Merge r235843 - [Web Animations] Interrupting an accelerated CSS tran…

…sition on a composited element in flight fails https://bugs.webkit.org/show_bug.cgi?id=189405 <rdar://problem/43342639> Reviewed by Simon Fraser. Source/WebCore: Test: webanimations/accelerated-transition-interrupted-on-composited-element.html If we interrupt an animation on an element that is composited also outside of the duration of the animation, the "stop" accelerated action would fail to be performed because we no longer had a resolved current time and the accelerated animation applied to the layer would never be removed. However, having a resolved current time is not necessary to stop an animation, only for the other types of actions (play, pause and seek). So we now simply default to a 0s time for an unresolved current time for a simple fix to this issue. * animation/KeyframeEffectReadOnly.cpp: (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions): LayoutTests: Add a new test that checks that interrupting a CSS transition targeting an accelerated property for an element that is composited outside the duration of the transition correctly interrupts the animation and jumps straight to the target value. * platform/win/TestExpectations: * webanimations/accelerated-transition-interrupted-on-composited-element-expected.html: Added. * webanimations/accelerated-transition-interrupted-on-composited-element.html: Added.
WebKit · Sep 19, 2018 · 4e93695 · 4e93695
1 parent 833ca1d
commit 4e93695
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 5 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+2018-09-10  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Interrupting an accelerated CSS transition on a composited element in flight fails
+        https://bugs.webkit.org/show_bug.cgi?id=189405
+        <rdar://problem/43342639>
+
+        Reviewed by Simon Fraser.
+
+        Add a new test that checks that interrupting a CSS transition targeting an accelerated property for an element
+        that is composited outside the duration of the transition correctly interrupts the animation and jumps straight
+        to the target value.
+
+        * platform/win/TestExpectations:
+        * webanimations/accelerated-transition-interrupted-on-composited-element-expected.html: Added.
+        * webanimations/accelerated-transition-interrupted-on-composited-element.html: Added.
+
 2018-09-05  Brent Fulgham  <bfulgham@apple.com>
 
         The width of a nullptr TextRun should be zero

diff --git a/LayoutTests/platform/win/TestExpectations b/LayoutTests/platform/win/TestExpectations
@@ -4156,6 +4156,8 @@ webkit.org/b/188166 webanimations/accessing-current-time-after-clearing-css-anim
 webkit.org/b/188166 webanimations/setting-css-animation-none-after-clearing-effect.html [ Failure ]
 webkit.org/b/188166 webanimations/setting-css-animation-timing-property-via-style-after-clearing-effect.html [ Failure ]
 
+webkit.org/b/189468 webanimations/accelerated-transition-interrupted-on-composited-element.html [ Failure ]
+
 webkit.org/b/188167 fast/repaint/canvas-object-fit.html [ Failure ]
 
 webkit.org/b/188168 fast/text/complex-first-glyph-with-initial-advance.html [ ImageOnlyFailure ]

diff --git a/...ests/webanimations/accelerated-transition-interrupted-on-composited-element-expected.html b/...ests/webanimations/accelerated-transition-interrupted-on-composited-element-expected.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<body>
+<style>
+div {
+    width: 100px;
+    height: 100px;
+    background-color: black;
+    will-change: transform;
+    transform: translateX(100px);
+}
+</style>
+<div></div>
+</body>
diff --git a/LayoutTests/webanimations/accelerated-transition-interrupted-on-composited-element.html b/LayoutTests/webanimations/accelerated-transition-interrupted-on-composited-element.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<body>
+<style>
+div {
+    width: 100px;
+    height: 100px;
+    background-color: black;
+    will-change: transform;
+}
+</style>
+<script>
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+const div = document.body.appendChild(document.createElement("div"));
+
+// Wait for a transition to start and abort it.
+div.addEventListener("transitionstart", event => {
+    div.style.transition = "none";
+    if (window.testRunner)
+        requestAnimationFrame(() => testRunner.notifyDone());
+});
+
+// Initiate a transform transition.
+setTimeout(() => {
+    div.style.transition = "transform 2s";
+    div.style.transform = "translateX(100px)";
+});
+
+</script>
+</body>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
+2018-09-10  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Interrupting an accelerated CSS transition on a composited element in flight fails
+        https://bugs.webkit.org/show_bug.cgi?id=189405
+        <rdar://problem/43342639>
+
+        Reviewed by Simon Fraser.
+
+        Test: webanimations/accelerated-transition-interrupted-on-composited-element.html
+
+        If we interrupt an animation on an element that is composited also outside of the duration of the animation,
+        the "stop" accelerated action would fail to be performed because we no longer had a resolved current time and
+        the accelerated animation applied to the layer would never be removed.
+
+        However, having a resolved current time is not necessary to stop an animation, only for the other types of
+        actions (play, pause and seek). So we now simply default to a 0s time for an unresolved current time for a
+        simple fix to this issue.
+
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
+
 2018-09-08  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [CSSJIT] Use lshiftPtr instead of mul32

diff --git a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
@@ -1278,11 +1278,8 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
 
     auto* compositedRenderer = downcast<RenderBoxModelObject>(renderer);
 
-    auto currentTime = animation()->currentTime();
-    if (!currentTime)
-        return;
-
-    auto timeOffset = currentTime->seconds();
+    // To simplify the code we use a default of 0s for an unresolved current time since for a Stop action that is acceptable.
+    auto timeOffset = animation()->currentTime().value_or(0_s).seconds();
     if (timing()->delay() < 0_s)
         timeOffset = -timing()->delay().seconds();