Bug 229437 – Redefining @keyframes does not work

• Component: Animations
• Status: RESOLVED FIXED

[Mu-An Chiou]

Created attachment 436262 [details]
HTML file illustrating the problem.

After a named @keyframes is defined, re-defining it by setting the styles again via script does not work.

In the attachment, clicking on "change color" will replace the CSS with an alternate animation color. This works in Chrome 92 as well as Firefox 91.

The demo opt to replace the style tag instead of resetting the style tag content because of https://bugs.webkit.org/show_bug.cgi?id=191265. 

This problem might be related to https://bugs.webkit.org/show_bug.cgi?id=201736 where CSS variables changes are found ineffective when used in keyframes.

[Radar WebKit Bug Importer]

<rdar://problem/82563372>

[Antoine Quint]

I did some work in this area recently, specifically these:

https://bugs.webkit.org/show_bug.cgi?id=234895
https://bugs.webkit.org/show_bug.cgi?id=234955

However, fixing those bugs did not fix this particular bug.

[Antoine Quint]

Created attachment 450519 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Antoine Quint]

Created attachment 450521 [details]
Patch

[Antti Koivisto]

Comment on attachment 450521 [details]
Patch

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

> Source/WebCore/style/StyleResolver.cpp:199
> +    m_keyframesRuleMap.set(animationName.impl(), WTFMove(rule));

Why does this map use AtomStringImpl* as a key instead of AtomString?

[Antoine Quint]

Created attachment 450522 [details]
Patch for landing

[Antti Koivisto]

Comment on attachment 450521 [details]
Patch

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

> Source/WebCore/style/StyleResolver.cpp:198
> +    AtomString animationName(rule->name());

This is probably not needed, rule->name() is an AtomString

[Antoine Quint]

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 450521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450521&action=review
> 
> > Source/WebCore/style/StyleResolver.cpp:198
> > +    AtomString animationName(rule->name());
> 
> This is probably not needed, rule->name() is an AtomString

I have no idea, but I'll look into changing that in a followup patch.

[EWS]

Committed r288882 (246634@main): <https://commits.webkit.org/246634@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450522 [details].

[Antoine Quint]

Will address refactor comments in bug 235962. The new WPT test landed in https://github.com/web-platform-tests/wpt/pull/32641.