Bug 222939

Summary: [Web Animations] setKeyframes does not preserve animation's current offset
Product: WebKit Reporter: Liam DeBeasi <ldebeasi>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, graouts, graouts, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Code reproduction
none
Simplified testcase
none
Patch none

Description Liam DeBeasi 2021-03-08 13:37:41 PST
Created attachment 422613 [details]
Code reproduction

When using setKeyframes() on an animation, WebKit does not preserve the animation's current offset. For example, if an animation is at 100% progress and setKeyframes() is called, WebKit will reset the animation's progress to 0%.

Steps to reproduce:

1. Open attached code reproduction in either Safari for iOS or macOS.
2. Observe that a blue square translates from left to right and then stops.
3. Press "Update Keyframes". You should notice that the square jumps back to the left/starting position.

Expected Behavior:

I would expect that after pressing "Update Keyframes" that the square stays at its ending position.

Actual Behavior:

The square is reset back to its starting position.

Other Information:

- I can reproduce this in STP 121 and iOS 14.5, though I can reproduce this on stable releases as well.
- Both Chrome and Firefox preserve the animation's current offset as expected.
- This is only an issue if "fill: 'both'" is set on the animation.
Comment 1 Radar WebKit Bug Importer 2021-03-09 01:11:34 PST
<rdar://problem/75207793>
Comment 2 Antoine Quint 2021-03-09 01:22:53 PST
Created attachment 422677 [details]
Simplified testcase

Simplified the test case to not involve accelerated animations and not require interaction.
Comment 3 Antoine Quint 2021-03-09 01:24:53 PST
Interestingly, the output of animation.effect.getComputedTiming() shows the expected values, so I think this is purely about presentation and not the animation model.
Comment 4 Antoine Quint 2021-03-09 07:26:36 PST
What happens is that calling seyKeyframes() empties m_blendingKeyframes and we rely on that to have KeyframeEffect::animatedProperties() return the effect's properties inside of DocumentTimeline::animationCanBeRemoved(). Because we are not aware of any animated properties, the animation is marked as removable and retired from the keyframe effect stack, and thus it is no longer invalidated during the ensuing style update.

I think all this needs is for KeyframeEffect::animatedProperties() to return properties from the parsed keyframes until the next style update when m_blendingKeyframes will be reset.
Comment 5 Antoine Quint 2021-03-09 08:23:56 PST
Created attachment 422700 [details]
Patch
Comment 6 EWS 2021-03-09 12:09:54 PST
Committed r274165: <https://commits.webkit.org/r274165>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422700 [details].
Comment 7 Liam DeBeasi 2021-03-10 07:41:27 PST
That was fast, thank you! I will keep an eye out for this fix whenever it lands in STP.
Comment 8 Antoine Quint 2021-03-10 07:52:27 PST
(In reply to Liam DeBeasi from comment #7)
> That was fast, thank you! I will keep an eye out for this fix whenever it
> lands in STP.

Thanks for filing with a great reduction, it really helps. It won't be in the immediate next STP release but in the one after that.