Summary: | Implicit keyframe for a CSS Animation should always use the underlying style | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | changseok, clopez, dino, esprehn+autocc, ews-watchlist, glenn, graouts, koivisto, kondapallykalyan, pdr, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://github.com/web-platform-tests/wpt/pull/32301 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 235138 | ||||||||
Attachments: |
|
Description
Antoine Quint
2022-01-09 05:44:52 PST
Created attachment 448698 [details]
Patch
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32301 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 on attachment 448698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448698&action=review > Source/WebCore/rendering/style/KeyframeList.cpp:92 > KeyframeValue keyframeValue(keyframe.key(), RenderStyle::clonePtr(*keyframe.style())); > for (auto propertyId : keyframe.properties()) > keyframeValue.addProperty(propertyId); > + keyframeValue.setTimingFunction(keyframe.timingFunction()); > + keyframeValue.setCompositeOperation(keyframe.compositeOperation()); can we just add a copy operator/function to KeyframeValue? The test failure is due to bug 235019. I'll re-run EWS once that other bug lands. (In reply to Antti Koivisto from comment #4) > Comment on attachment 448698 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448698&action=review > > > Source/WebCore/rendering/style/KeyframeList.cpp:92 > > KeyframeValue keyframeValue(keyframe.key(), RenderStyle::clonePtr(*keyframe.style())); > > for (auto propertyId : keyframe.properties()) > > keyframeValue.addProperty(propertyId); > > + keyframeValue.setTimingFunction(keyframe.timingFunction()); > > + keyframeValue.setCompositeOperation(keyframe.compositeOperation()); > > can we just add a copy operator/function to KeyframeValue? Actually, the only two call sites for fillImplicitKeyframes() are places where we make the copies as well, so I think we can wrap this all up into a single function. (In reply to Antoine Quint from comment #6) > (In reply to Antti Koivisto from comment #4) > > Comment on attachment 448698 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=448698&action=review > > > > > Source/WebCore/rendering/style/KeyframeList.cpp:92 > > > KeyframeValue keyframeValue(keyframe.key(), RenderStyle::clonePtr(*keyframe.style())); > > > for (auto propertyId : keyframe.properties()) > > > keyframeValue.addProperty(propertyId); > > > + keyframeValue.setTimingFunction(keyframe.timingFunction()); > > > + keyframeValue.setCompositeOperation(keyframe.compositeOperation()); > > > > can we just add a copy operator/function to KeyframeValue? > > Actually, the only two call sites for fillImplicitKeyframes() are places > where we make the copies as well, so I think we can wrap this all up into a > single function. Actually, that's not true. I'll attempt to refactor this in a separate patch. Created attachment 448717 [details]
Patch for landing
Committed r287827 (245879@main): <https://commits.webkit.org/245879@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448717 [details]. |