[web-animations] font-variant-east-asian should support discrete animation
Created attachment 454257 [details] Patch
Comment on attachment 454257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454257&action=review > Source/WebCore/animation/CSSPropertyAnimation.cpp:2526 > + bool canInterpolate(const RenderStyle&, const RenderStyle&, CompositeOperation) const override { return false; } final > Source/WebCore/animation/CSSPropertyAnimation.cpp:2528 > + bool equals(const RenderStyle& a, const RenderStyle& b) const override final > Source/WebCore/animation/CSSPropertyAnimation.cpp:2537 > + void blend(RenderStyle& destination, const RenderStyle& from, const RenderStyle& to, const CSSPropertyBlendingContext& context) const override final > Source/WebCore/animation/CSSPropertyAnimation.cpp:2548 > + destination.setFontDescription(WTFMove(description)); > + destination.fontCascade().update(currentFontSelector); Other style settings on RenderStyle do the destination.setFontDescription() and fontCascade().update() thing internally. Can we be consistent?
(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 454257 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454257&action=review > > > Source/WebCore/animation/CSSPropertyAnimation.cpp:2548 > > + destination.setFontDescription(WTFMove(description)); > > + destination.fontCascade().update(currentFontSelector); > > Other style settings on RenderStyle do the destination.setFontDescription() > and fontCascade().update() thing internally. Can we be consistent? The difference here is that we need to set multiple members on the FontDescription rather than a single one, which I think is what the RenderStyle methods do. Since we need to set multiple fields for this property, this feels more efficient.
Can we make a helper on RenderStyle that everyone uses?
Comment on attachment 454257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454257&action=review > Source/WebCore/animation/CSSPropertyAnimation.cpp:2547 > + ASSERT(!context.progress || context.progress == 1.0); > + auto& sourceFontDescription = (context.progress ? to : from).fontDescription(); > + > + FontSelector* currentFontSelector = destination.fontCascade().fontSelector(); > + auto description = destination.fontDescription(); > + description.setVariantEastAsianVariant(sourceFontDescription.variantEastAsianVariant()); > + description.setVariantEastAsianWidth(sourceFontDescription.variantEastAsianWidth()); > + description.setVariantEastAsianRuby(sourceFontDescription.variantEastAsianRuby()); > + destination.setFontDescription(WTFMove(description)); I'm confused. https://drafts.csswg.org/css-fonts/#font-variant-east-asian-prop says Animation type: discrete. Why do we need a blend function here but not in https://bugs.webkit.org/show_bug.cgi?id=237662?
Comment on attachment 454257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454257&action=review >> Source/WebCore/animation/CSSPropertyAnimation.cpp:2547 >> + destination.setFontDescription(WTFMove(description)); > > I'm confused. https://drafts.csswg.org/css-fonts/#font-variant-east-asian-prop says Animation type: discrete. Why do we need a blend function here but not in https://bugs.webkit.org/show_bug.cgi?id=237662? Oh, I see, it's because animations work at the RenderStyle layer, not the CSS layer, and in RenderStyle (FontDescription, actually) we have 3 separate pieces of state for this, so you can't use the built-in discrete infrastructure because it doesn't know that the 3 values need to be set together.
r=me assuming you follow Simon's advice
(In reply to Simon Fraser (smfr) from comment #4) > Can we make a helper on RenderStyle that everyone uses? This is a good idea but I don't think it should block these patches.
Created attachment 454341 [details] Patch for landing
Committed r291109 (248271@main): <https://commits.webkit.org/248271@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454341 [details].
<rdar://problem/90098220>