Bug 235014

Summary: Implicit keyframe for a CSS Animation should always use the underlying style
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: 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 Flags
Patch
koivisto: review+, ews-feeder: commit-queue-
Patch for landing ews-feeder: commit-queue-

Antoine Quint
Reported 2022-01-09 05:44:52 PST
Implicit keyframe for a CSS Animation should always use the underlying style
Attachments
Patch (10.50 KB, patch)
2022-01-09 05:49 PST, Antoine Quint 		koivisto: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (11.18 KB, patch)
2022-01-09 12:23 PST, Antoine Quint 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Antoine Quint
Comment 1 2022-01-09 05:49:28 PST
Created attachment 448698 [details] Patch
Antoine Quint
Comment 2 2022-01-09 05:49:57 PST
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32301
EWS Watchlist
Comment 3 2022-01-09 05:50:38 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
Antti Koivisto
Comment 4 2022-01-09 06:45:51 PST
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?
Antoine Quint
Comment 5 2022-01-09 10:07:48 PST
The test failure is due to bug 235019. I'll re-run EWS once that other bug lands.
Antoine Quint
Comment 6 2022-01-09 10:27:20 PST
(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.
Antoine Quint
Comment 7 2022-01-09 10:56:50 PST
(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.
Antoine Quint
Comment 8 2022-01-09 12:23:56 PST
Created attachment 448717 [details] Patch for landing
EWS
Comment 9 2022-01-09 13:20:54 PST
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].
Radar WebKit Bug Importer
Comment 10 2022-01-09 13:21:22 PST
<rdar://problem/87314805>
Note You need to log in before you can comment on or make changes to this bug.