Bug 229437

Summary:

Redefining @keyframes does not work

Product:

WebKit

Reporter:

Mu-An Chiou <me>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

clopez, dino, ews-watchlist, graouts, graouts, koivisto, simon.fraser, webkit-bug-importer, youennf

Priority:

Keywords:

BrowserCompat, InRadar

Version:

Safari 14

Hardware:

All

OS:

Unspecified

Bug Depends on:

Bug Blocks:

235962

Attachments:

Description	Flags
HTML file illustrating the problem.	none
Patch	none
Patch	koivisto: review+
Patch for landing	none

Description Mu-An Chiou 2021-08-23 21:05:07 PDT

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.

Comment 1 Radar WebKit Bug Importer 2021-08-30 21:06:21 PDT

<rdar://problem/82563372>

Comment 2 Antoine Quint 2022-02-01 02:47:49 PST

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.

Comment 3 Antoine Quint 2022-02-01 06:29:34 PST

Created attachment 450519 [details]
Patch

Comment 4 EWS Watchlist 2022-02-01 06:31:09 PST

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

Comment 5 Antoine Quint 2022-02-01 06:37:15 PST

Created attachment 450521 [details]
Patch

Comment 6 Antti Koivisto 2022-02-01 06:54:16 PST

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?

Comment 7 Antoine Quint 2022-02-01 06:54:41 PST

Created attachment 450522 [details]
Patch for landing

Comment 8 Antti Koivisto 2022-02-01 06:56:36 PST

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

Comment 9 Antoine Quint 2022-02-01 06:57:55 PST

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

Comment 10 EWS 2022-02-01 08:54:36 PST

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

Comment 11 Antoine Quint 2022-02-01 10:21:07 PST

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