Bug 170784

<rdar://problem/31599333>

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*.

Created attachment 340560 [details] Testcase Testing more, it seems unrelated to whether -webkit-overflow-scrolling is set ; just specific to iOS.

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?

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.

Created attachment 341453 [details] WIP Patch

Created attachment 341454 [details] Alternative patch Initial attempt mentioned in comment 4

Created attachment 341646 [details] Another testcase Compare linear VS steps

Created attachment 341650 [details] Patch

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

Created attachment 342245 [details] Patch

Created attachment 342414 [details] Patch

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

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 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 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 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.

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

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.

Created attachment 344170 [details] Patch for landing

(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 on attachment 344170 [details] Patch for landing Clearing flags on attachment: 344170 Committed r233460: <https://trac.webkit.org/changeset/233460>

All reviewed patches have been landed. Closing bug.