Bug 237665 - [web-animations] font-variant-east-asian should support discrete animation
Summary: [web-animations] font-variant-east-asian should support discrete animation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar, WebExposed, WPTImpact
Depends on:
Blocks:
 
Reported: 2022-03-09 10:07 PST by Antoine Quint
Modified: 2022-03-10 07:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2022-03-09 10:07 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (12.00 KB, patch)
2022-03-10 05:35 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2022-03-09 10:07:08 PST
[web-animations] font-variant-east-asian should support discrete animation
Comment 1 Antoine Quint 2022-03-09 10:07:37 PST
Created attachment 454257 [details]
Patch
Comment 2 Simon Fraser (smfr) 2022-03-09 11:31:32 PST
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?
Comment 3 Antoine Quint 2022-03-09 11:42:02 PST
(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.
Comment 4 Simon Fraser (smfr) 2022-03-09 11:48:00 PST
Can we make a helper on RenderStyle that everyone uses?
Comment 5 Myles C. Maxfield 2022-03-09 16:55:17 PST
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 6 Myles C. Maxfield 2022-03-09 16:57:30 PST
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.
Comment 7 Myles C. Maxfield 2022-03-09 16:59:05 PST
r=me assuming you follow Simon's advice
Comment 8 Myles C. Maxfield 2022-03-09 17:00:19 PST
(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.
Comment 9 Antoine Quint 2022-03-10 05:35:28 PST
Created attachment 454341 [details]
Patch for landing
Comment 10 EWS 2022-03-10 07:58:36 PST
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].
Comment 11 Radar WebKit Bug Importer 2022-03-10 07:59:16 PST
<rdar://problem/90098220>