Bug 218655

Summary: Reversed transform animation not applied alongside other transform animations
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220653
https://bugs.webkit.org/show_bug.cgi?id=224062
Attachments:
Description Flags
Test
none
Patch
none
Patch
none
Patch none

Antoine Quint
Reported 2020-11-06 05:05:56 PST
Created attachment 413425 [details] Test Consider this example: div { width: 100px; height: 100px; margin-left: 100px; margin-top: 100px; background-color: black; animation: scale 2s forwards, rotate 500ms 1s reverse; } @keyframes scale { 50% { scale: 2 } } @keyframes rotate { 100% { rotate: 90deg } } In Safari, the rotate animation won't be performed because it's reversed and we disable accelerated animations when the playback rate is not 1 (see bug 211839). We should ensure that we only run accelerated transform animations if all of the accelerated transform animations can be accelerated.
Attachments
Test (305 bytes, text/html)
2020-11-06 05:05 PST, Antoine Quint 		no flags
Details
Patch (26.91 KB, patch)
2021-01-14 08:55 PST, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (28.19 KB, patch)
2021-01-15 00:14 PST, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (29.14 KB, patch)
2021-01-15 02:38 PST, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-11-06 05:06:16 PST
<rdar://problem/71116284>
Antoine Quint
Comment 2 2020-11-06 05:16:57 PST
We should also test we switch to software mode if already accelerated and a non-accelerated animation is added.
Antoine Quint
Comment 3 2021-01-14 07:22:14 PST
I have the case where we try to add an animation that we think should be accelerated but fails to run accelerated due to the internals of GraphicsLayerCA working fine. I also need to handle the case where we determine the animation cannot be run accelerated before even involving GraphicsLayerCA, such as a steps() timing function.
Antoine Quint
Comment 4 2021-01-14 08:55:46 PST
Created attachment 417627 [details] Patch
Simon Fraser (smfr)
Comment 5 2021-01-14 09:29:20 PST
Comment on attachment 417627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417627&action=review > Source/WebCore/animation/DocumentTimeline.cpp:443 > + m_isApplyingPendingAcceleratedAnimations = true; SetForScope<> > Source/WebCore/animation/DocumentTimeline.cpp:466 > + if (keyframeEffect.failedToRunAcceleratedTransformRelatedAnimation()) { It's a bit weird to have the very specific failedToRunAcceleratedTransformRelatedAnimation() function. Could applyPendingAcceleratedActions() instead return an OptionSet<> of what it did? > Source/WebCore/animation/KeyframeEffect.cpp:1773 > + if (downcast<DocumentTimeline>(animation()->timeline())->isApplyingPendingAcceleratedAnimations()) { Not a fan of code like this that needs to know what's on its call stack.
Antoine Quint
Comment 6 2021-01-14 12:53:39 PST
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 417627 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417627&action=review > > > Source/WebCore/animation/DocumentTimeline.cpp:466 > > + if (keyframeEffect.failedToRunAcceleratedTransformRelatedAnimation()) { > > It's a bit weird to have the very specific > failedToRunAcceleratedTransformRelatedAnimation() function. Could > applyPendingAcceleratedActions() instead return an OptionSet<> of what it > did? Yes, I'll do this. > > Source/WebCore/animation/KeyframeEffect.cpp:1773 > > + if (downcast<DocumentTimeline>(animation()->timeline())->isApplyingPendingAcceleratedAnimations()) { > > Not a fan of code like this that needs to know what's on its call stack. Me neither actually, but was a bit lazy there I confess. I'll make it so that a parameter is passed down to effectFailedToRunAcceleratedTransformRelatedAnimation() to indicate whether the change should be made synchronously or as part of an accelerated action.
Antoine Quint
Comment 7 2021-01-15 00:14:12 PST
Created attachment 417682 [details] Patch
Antoine Quint
Comment 8 2021-01-15 02:38:54 PST
Created attachment 417690 [details] Patch
EWS
Comment 9 2021-01-15 09:47:23 PST
Committed r271524: <https://trac.webkit.org/changeset/271524> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417690 [details].
Note You need to log in before you can comment on or make changes to this bug.