Bug 234895 - [Web Animations] inserting a rule within a @keyframes rule should update animations
Summary: [Web Animations] inserting a rule within a @keyframes rule should update anim...
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
Depends on:
Blocks: 235138
  Show dependency treegraph
 
Reported: 2022-01-05 13:21 PST by Antoine Quint
Modified: 2022-01-12 12:38 PST (History)
14 users (show)

See Also:


Attachments
Patch (20.31 KB, patch)
2022-01-05 13:34 PST, Antoine Quint
darin: review+
Details | Formatted Diff | Diff
Patch for landing (24.09 KB, patch)
2022-01-06 03:50 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (23.26 KB, patch)
2022-01-06 08:18 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-01-05 13:21:52 PST
[Web Animations] inserting a rule within a @keyframes rule should update animations
Comment 1 Antoine Quint 2022-01-05 13:34:56 PST
Created attachment 448426 [details]
Patch
Comment 2 Darin Adler 2022-01-05 15:17:43 PST
Comment on attachment 448426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448426&action=review

> Source/WebCore/animation/CSSAnimation.cpp:249
> +    auto owningElement = this->owningElement();

Should this be RefPtr instead of auto?

> Source/WebCore/css/CSSKeyframesRule.cpp:168
> +    if (CSSStyleSheet* parent = parentStyleSheet()) {
> +        if (Document* ownerDocument = parent->ownerDocument())
> +            ownerDocument->cssKeyframesRuleDidChange(name());
> +    }

Would be nice to not have this be pasted twice; a shared function perhaps. Separately, I suggest using auto more here.

> Source/WebCore/dom/Document.h:1541
> +    void cssKeyframesRuleDidChange(String);

I am not sure that the "css" here is needed; is there any other kind of keyframes rule other than a CSS one?

It’s not obvious that the string argument here is the name of the rule, so there should be an argument name "name" here.

I suggest either StringView (preferred) or const String& as the type so we don’t do unnecessary reference count churn.

> Source/WebCore/dom/Element.cpp:4067
> +    if (auto* animationData = animationRareData(pseudoId))
> +        return animationData->isPendingKeyframesUpdate();
> +    return false;

How I would write this:

    auto data = animationRareData(pseudoId);
    return data && data->isPendingKeyframesUpdate();
Comment 3 Antti Koivisto 2022-01-05 22:49:34 PST
Comment on attachment 448426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448426&action=review

>> Source/WebCore/css/CSSKeyframesRule.cpp:168
>> +    if (CSSStyleSheet* parent = parentStyleSheet()) {
>> +        if (Document* ownerDocument = parent->ownerDocument())
>> +            ownerDocument->cssKeyframesRuleDidChange(name());
>> +    }
> 
> Would be nice to not have this be pasted twice; a shared function perhaps. Separately, I suggest using auto more here.

The shared function exist already. This should be done in CSSStyleSheet::didMutateRules. There should probably be a new RuleMutationType for it.
Comment 4 Antoine Quint 2022-01-06 03:50:10 PST
Created attachment 448484 [details]
Patch for landing
Comment 5 Antoine Quint 2022-01-06 04:13:52 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 448426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448426&action=review
> 
> > Source/WebCore/animation/CSSAnimation.cpp:249
> > +    auto owningElement = this->owningElement();
> 
> Should this be RefPtr instead of auto?

I started a thread about this on Slack: https://webkit.slack.com/archives/C01ARTA5TDM/p1641467768004500.

> > Source/WebCore/css/CSSKeyframesRule.cpp:168
> > +    if (CSSStyleSheet* parent = parentStyleSheet()) {
> > +        if (Document* ownerDocument = parent->ownerDocument())
> > +            ownerDocument->cssKeyframesRuleDidChange(name());
> > +    }
> 
> Would be nice to not have this be pasted twice; a shared function perhaps.
> Separately, I suggest using auto more here.
> 
> > Source/WebCore/dom/Document.h:1541
> > +    void cssKeyframesRuleDidChange(String);
> 
> I am not sure that the "css" here is needed; is there any other kind of
> keyframes rule other than a CSS one?
> 
> It’s not obvious that the string argument here is the name of the rule, so
> there should be an argument name "name" here.
> 
> I suggest either StringView (preferred) or const String& as the type so we
> don’t do unnecessary reference count churn.

Changed to `void keyframesRuleDidChange(const String& name);`

> > Source/WebCore/dom/Element.cpp:4067
> > +    if (auto* animationData = animationRareData(pseudoId))
> > +        return animationData->isPendingKeyframesUpdate();
> > +    return false;
> 
> How I would write this:
> 
>     auto data = animationRareData(pseudoId);
>     return data && data->isPendingKeyframesUpdate();

Fixed.
Comment 6 Antoine Quint 2022-01-06 04:14:14 PST
(In reply to Antti Koivisto from comment #3)
> Comment on attachment 448426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448426&action=review
> 
> >> Source/WebCore/css/CSSKeyframesRule.cpp:168
> >> +    if (CSSStyleSheet* parent = parentStyleSheet()) {
> >> +        if (Document* ownerDocument = parent->ownerDocument())
> >> +            ownerDocument->cssKeyframesRuleDidChange(name());
> >> +    }
> > 
> > Would be nice to not have this be pasted twice; a shared function perhaps. Separately, I suggest using auto more here.
> 
> The shared function exist already. This should be done in
> CSSStyleSheet::didMutateRules. There should probably be a new
> RuleMutationType for it.

This is addressed in the patch for landing.
Comment 7 Antti Koivisto 2022-01-06 07:09:19 PST
Comment on attachment 448484 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=448484&action=review

> Source/WebCore/css/CSSStyleSheet.h:105
> -        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = RuleMutationType::OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
>          RuleMutationScope(CSSRule*);

I think it would be better to add RuleMutationType argument to the second constructor version and use it for CSSKeyframesRule mutations (or just test for CSSKeyframesRule in the constructor to set the appropriate mutation type). The idea here is that the first constructor is used when stylesheet is itself mutated while the seconds is used when some rule mutates.
Comment 8 Antoine Quint 2022-01-06 08:10:30 PST
(In reply to Antti Koivisto from comment #7)
> Comment on attachment 448484 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448484&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.h:105
> > -        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> > +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = RuleMutationType::OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> >          RuleMutationScope(CSSRule*);
> 
> I think it would be better to add RuleMutationType argument to the second
> constructor version and use it for CSSKeyframesRule mutations (or just test
> for CSSKeyframesRule in the constructor to set the appropriate mutation
> type). The idea here is that the first constructor is used when stylesheet
> is itself mutated while the seconds is used when some rule mutates.

OK, I'll infer the mutation type from the CSSRule type and add a `String m_modifiedKeyframesRuleName` member to `RuleMutationScope` to track the modified @keyframes rule name.
Comment 9 Antoine Quint 2022-01-06 08:18:05 PST
Created attachment 448504 [details]
Patch for landing
Comment 10 Darin Adler 2022-01-06 09:36:15 PST
Two other thoughts for refinement (tiny nits), neither urgent:

I think that "hasPendingKeyframesUpdate" would be better than "isPendingKeyframesUpdate".

One place here we have "is<CSSAnimation>(animation)" but it should be "is<CSSAnimation>(*animation)" since fact that the set uses pointers is an implementation detail and the pointers can never be null.
Comment 11 Antoine Quint 2022-01-06 11:57:46 PST
(In reply to Darin Adler from comment #10)
> Two other thoughts for refinement (tiny nits), neither urgent:
> 
> I think that "hasPendingKeyframesUpdate" would be better than
> "isPendingKeyframesUpdate".

I went back and forth between the two while writing the patch, but this tips me over to the other side. I've made that change to the commit.

> One place here we have "is<CSSAnimation>(animation)" but it should be
> "is<CSSAnimation>(*animation)" since fact that the set uses pointers is an
> implementation detail and the pointers can never be null.

Are you saying that the HashSet iterator skips over null values?
Comment 12 Antoine Quint 2022-01-06 11:57:52 PST
Committed r287707 (245793@trunk): <https://commits.webkit.org/245793@trunk>
Comment 13 Radar WebKit Bug Importer 2022-01-06 11:59:40 PST
<rdar://problem/87212387>
Comment 14 Darin Adler 2022-01-06 13:07:16 PST
(In reply to Antoine Quint from comment #11)
> (In reply to Darin Adler from comment #10)
> > I think that "hasPendingKeyframesUpdate" would be better than
> > "isPendingKeyframesUpdate".
> 
> I went back and forth between the two while writing the patch, but this tips
> me over to the other side. I've made that change to the commit.

Great!

> Are you saying that the HashSet iterator skips over null values?

Sort of. You can’t put a nullptr into a HashSet of pointers, no values in the set can be nullptr. And yes the iterator does skip the null values, technically they are empty buckets.
Comment 15 Antti Koivisto 2022-01-06 21:12:40 PST
Comment on attachment 448504 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=448504&action=review

> Source/WebCore/css/CSSStyleSheet.cpp:181
> +    if (mutationType == RuleInsertion && !contentsWereClonedForMutation && !scope->activeStyleSheetsContains(this)) {
> +        if (insertedKeyframesRule) {
> +            if (auto* resolver = scope->resolverIfExists())
> +                resolver->addKeyframeStyle(*insertedKeyframesRule);
> +            return;
> +        }
> +        scope->didChangeActiveStyleSheetCandidates();
> +        return;
> +    }

We don't need two copies if this section of code!
Comment 16 Antoine Quint 2022-01-07 00:29:16 PST
(In reply to Antti Koivisto from comment #15)
> Comment on attachment 448504 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448504&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.cpp:181
> > +    if (mutationType == RuleInsertion && !contentsWereClonedForMutation && !scope->activeStyleSheetsContains(this)) {
> > +        if (insertedKeyframesRule) {
> > +            if (auto* resolver = scope->resolverIfExists())
> > +                resolver->addKeyframeStyle(*insertedKeyframesRule);
> > +            return;
> > +        }
> > +        scope->didChangeActiveStyleSheetCandidates();
> > +        return;
> > +    }
> 
> We don't need two copies if this section of code!

Oh no! Fixed in r287741.
Comment 17 Antoine Quint 2022-01-07 00:44:15 PST
(In reply to Darin Adler from comment #14)
> (In reply to Antoine Quint from comment #11)
> > Are you saying that the HashSet iterator skips over null values?
> 
> Sort of. You can’t put a nullptr into a HashSet of pointers, no values in
> the set can be nullptr. And yes the iterator does skip the null values,
> technically they are empty buckets.

Filed a patch to remove the null checks in bug 234948.