Bug 170784

Summary: [iOS] Animations with Bézier timing function not suspended on UI process when animation-play-state is set to "paused"
Product: WebKit Reporter: Alan Orozco <orozcoalan>
Component: AnimationsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: ajuma, commit-queue, dino, ews, fred.wang, graouts, rwlbuis, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 10   
Hardware: iPhone / iPad   
OS: iOS 10   
See Also: https://github.com/web-platform-tests/wpt/pull/11266
Bug Depends on: 186045, 186046, 186129    
Bug Blocks: 186127    
Attachments:
Description Flags
Testcase
none
Testcase
none
WIP Patch
none
Alternative patch
none
Another testcase
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch for landing none

Description Alan Orozco 2017-04-12 12:31:14 PDT
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)
Comment 1 Radar WebKit Bug Importer 2017-04-12 22:44:00 PDT
<rdar://problem/31599333>
Comment 2 Frédéric Wang (:fredw) 2018-05-17 00:57:37 PDT
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*.
Comment 3 Frédéric Wang (:fredw) 2018-05-17 01:06:03 PDT
Created attachment 340560 [details]
Testcase

Testing more, it seems unrelated to whether -webkit-overflow-scrolling is set ; just specific to iOS.
Comment 4 Frédéric Wang (:fredw) 2018-05-17 08:04:29 PDT
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?
Comment 5 Frédéric Wang (:fredw) 2018-05-28 05:03:26 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2018-05-28 08:48:19 PDT
Created attachment 341453 [details]
WIP Patch
Comment 7 Frédéric Wang (:fredw) 2018-05-28 09:02:02 PDT
Created attachment 341454 [details]
Alternative patch

Initial attempt mentioned in comment 4
Comment 8 Frédéric Wang (:fredw) 2018-05-31 00:04:09 PDT
Created attachment 341646 [details]
Another testcase

Compare linear VS steps
Comment 9 Frédéric Wang (:fredw) 2018-05-31 01:21:51 PDT
Created attachment 341650 [details]
Patch
Comment 10 Frédéric Wang (:fredw) 2018-05-31 02:55:05 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/11266
Comment 11 Frédéric Wang (:fredw) 2018-06-08 01:33:04 PDT
Created attachment 342245 [details]
Patch
Comment 12 Frédéric Wang (:fredw) 2018-06-11 02:31:09 PDT
Created attachment 342414 [details]
Patch
Comment 13 Build Bot 2018-06-11 06:38:10 PDT
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
Comment 14 Build Bot 2018-06-11 06:38:21 PDT
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
Comment 15 Ali Juma 2018-06-13 13:43:14 PDT
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?
Comment 16 Frédéric Wang (:fredw) 2018-06-13 14:04:06 PDT
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.
Comment 17 Frédéric Wang (:fredw) 2018-06-14 04:45:31 PDT
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.
Comment 18 Frédéric Wang (:fredw) 2018-07-02 15:27:28 PDT
@Antoine Quint: Can you please take a look at the patch?
Comment 19 Antoine Quint 2018-07-03 01:56:16 PDT
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.
Comment 20 Frédéric Wang (:fredw) 2018-07-03 02:43:44 PDT
Created attachment 344170 [details]
Patch for landing
Comment 21 Frédéric Wang (:fredw) 2018-07-03 02:44:38 PDT
(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".
Comment 22 WebKit Commit Bot 2018-07-03 03:23:01 PDT
Comment on attachment 344170 [details]
Patch for landing

Clearing flags on attachment: 344170

Committed r233460: <https://trac.webkit.org/changeset/233460>
Comment 23 WebKit Commit Bot 2018-07-03 03:23:03 PDT
All reviewed patches have been landed.  Closing bug.