Bug 170784 – [iOS] Animations with Bézier timing function not suspended on UI process when animation-play-state is set to "paused"

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Alan Orozco]

See example: <http://codepen.io/anon/pen/bWGobG>

Intended behavior is that an element should pause its animation when animation-play-state is set to "paused".

Linked example works on desktop Safari. (Safari 10.0.3, macOS 10.12.3)

[Radar WebKit Bug Importer]

<rdar://problem/31599333>

[Frédéric Wang (:fredw)]

Created attachment 340559 [details]
Testcase

This is a testcase based on the one provided by the reporter. I seems animation is not paused if animation-play-state is set *dynamically*.

[Frédéric Wang (:fredw)]

Created attachment 340560 [details]
Testcase

Testing more, it seems unrelated to whether -webkit-overflow-scrolling is set ; just specific to iOS.

[Frédéric Wang (:fredw)]

I debugged this a bit today. Pausing seems to behave normally for WebCore's animation-related objects but then the code arrives in GraphicsLayerCA::pauseCAAnimationOnLayer where the running animation is replace with a zero-speed copy:

    // Animations on the layer are immutable, so we have to clone and modify.   
    RefPtr<PlatformCAAnimation> newAnim = curAnim->copy();

    newAnim->setSpeed(0);
    newAnim->setTimeOffset(timeOffset.seconds());

    layer->addAnimationForKey(animationID, *newAnim); // This will replace the running animation.                                                              

Here for macOS vs iOS we have "Cocoa" vs "Remote" layer-related objects, which I guess behave differently. I have not tried to debug more for now but removing the animation key before adding the zero-speed copy does stop the animation  (but unfortunately seems to remove the current transform). So I guess it's an issue in how "Remote" objects handle this.

I also wonder what is this bug7311367Workaround function in GraphicsLayerCA  and whether it is relevant here?

[Frédéric Wang (:fredw)]

I checked this again this morning, indeed if the animation already exists and the addition was already committed to the UI Process then PlatformCALayerRemote::addAnimationForKey does nothing (since m_properties.addedAnimations was cleared). In order to inform the UI process to replace the animation, one solution is to add it to m_properties.addedAnimations and m_properties.keyPathsOfAnimationsToRemove (the latter does not seem necessary in the current implementation though). Alternatively, calling layer->removeAnimationForKey as I suggested in comment 4 does more or less the same.

For the record, I've been struggling to write a test, the CSS value obtained via getComputedStyle() don't change after the pause, it's only the animation on the UI process that continues. Also the bug does not show up with a steps(.., ..) timing function so it's difficult to know the exact expected CSS value at pause time. I expect to upload something today though.

(In reply to Frédéric Wang (:fredw) from comment #4)
> (but unfortunately seems to remove the current transform).

This seems to be a separate bug that happens only for the rotation transform.

[Frédéric Wang (:fredw)]

Created attachment 341453 [details]
WIP Patch

[Frédéric Wang (:fredw)]

Created attachment 341454 [details]
Alternative patch

Initial attempt mentioned in comment 4

[Frédéric Wang (:fredw)]

Created attachment 341646 [details]
Another testcase

Compare linear VS steps

[Frédéric Wang (:fredw)]

Created attachment 341650 [details]
Patch

[Frédéric Wang (:fredw)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/11266

[Frédéric Wang (:fredw)]

Created attachment 342245 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 342414 [details]
Patch

[EWS Watchlist]

Comment on attachment 342414 [details]
Patch

Attachment 342414 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8126644

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 342422 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ali Juma]

Comment on attachment 342414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342414&action=review

> LayoutTests/http/wpt/css/css-animations/set-animation-play-state-to-paused-001.html:74
> +        // Stop the reftest after the time when the animations would have stop.

It's a bit unfortunate that this test will always take 2 seconds. Maybe try a shorter animation duration? Or does that make the test too flaky?

[Frédéric Wang (:fredw)]

Comment on attachment 342414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342414&action=review

>> LayoutTests/http/wpt/css/css-animations/set-animation-play-state-to-paused-001.html:74
>> +        // Stop the reftest after the time when the animations would have stop.
> 
> It's a bit unfortunate that this test will always take 2 seconds. Maybe try a shorter animation duration? Or does that make the test too flaky?

That won't make the test flacky. The thing is that we want to wait a bit in order so that the we are sure the red squares would be visible without the patch. In previous patch, I just tried to wait a small amount of time. I think that amount can be calculated from the square speed and current position + a small delta.

[Frédéric Wang (:fredw)]

Comment on attachment 342414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342414&action=review

>>> LayoutTests/http/wpt/css/css-animations/set-animation-play-state-to-paused-001.html:74
>>> +        // Stop the reftest after the time when the animations would have stop.
>> 
>> It's a bit unfortunate that this test will always take 2 seconds. Maybe try a shorter animation duration? Or does that make the test too flaky?
> 
> That won't make the test flacky. The thing is that we want to wait a bit in order so that the we are sure the red squares would be visible without the patch. In previous patch, I just tried to wait a small amount of time. I think that amount can be calculated from the square speed and current position + a small delta.

Thinking again, I think this would add more code and make the test less readable. So if this is a concern, I think we can just reduce the time before testRunner.notifyDone() (e.g. 900ms instead of the whole animation duration). Reducing the animation duration increases the speed of the square and may make the test flacky.

[Frédéric Wang (:fredw)]

@Antoine Quint: Can you please take a look at the patch?

[Antoine Quint]

Comment on attachment 342414 [details]
Patch

You might want to rebase and check your test still passes before landing as there have been some changes related to when we apply pending accelerated animations.

[Frédéric Wang (:fredw)]

Created attachment 344170 [details]
Patch for landing

[Frédéric Wang (:fredw)]

(In reply to Antoine Quint from comment #19)
> Comment on attachment 342414 [details]
> Patch
> 
> You might want to rebase and check your test still passes before landing as
> there have been some changes related to when we apply pending accelerated
> animations.

Right, I just re-tested locally, patch still works as expected and test passes.  Anyway, I'll "land safely".

[WebKit Commit Bot]

Comment on attachment 344170 [details]
Patch for landing

Clearing flags on attachment: 344170

Committed r233460: <https://trac.webkit.org/changeset/233460>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.