Bug 237665 – [web-animations] font-variant-east-asian should support discrete animation

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

[web-animations] font-variant-east-asian should support discrete animation

[Antoine Quint]

Created attachment 454257 [details]
Patch

[Simon Fraser (smfr)]

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?

[Antoine Quint]

(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.

[Simon Fraser (smfr)]

Can we make a helper on RenderStyle that everyone uses?

[Myles C. Maxfield]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

r=me assuming you follow Simon's advice

[Myles C. Maxfield]

(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.

[Antoine Quint]

Created attachment 454341 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/90098220>