git://git.webkit.org / WebKit-https.git / commitdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 207d844)
REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2018 08:27:54 +0000 (08:27 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2018 08:27:54 +0000 (08:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185299
<rdar://problem/39630230>

Reviewed by Simon Fraser.

Source/WebCore:

In r230574, the fix for webkit.org/b/184518, we changed the processing order in GraphicsLayerCA::updateAnimations() to first
process m_uncomittedAnimations and then m_animationsToProcess, so we are guaranteed animations exist before we attempt to pause
or seek them. This broke interrupting and resuming hardware animations (such as an interrupted CSS Transition or an animation
running in a non-visible tab) since a pause operation recorded _before_ an animation was added would be paused anyway since
the animation was now first added, and then paused. The fix is simply to clear any pending AnimationProcessingAction for a
newly-uncommitted animation.

Test: transitions/interrupted-transition-hardware.html

* platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::createAnimationFromKeyframes):
(WebCore::GraphicsLayerCA::appendToUncommittedAnimations):
(WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
* platform/graphics/ca/GraphicsLayerCA.h:
(WebCore::GraphicsLayerCA::LayerPropertyAnimation::LayerPropertyAnimation):

LayoutTests:

Add a new test where we interrupt a transition and check that upon returning to the original value,
an animated value is still used and not the initial value. This test fails prior to this patch.

* transitions/interrupted-transition-hardware-expected.html: Added.
* transitions/interrupted-transition-hardware.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231840 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog patch | blob | history
LayoutTests/transitions/interrupted-transition-hardware-expected.html [new file with mode: 0644] patch | blob
LayoutTests/transitions/interrupted-transition-hardware.html [new file with mode: 0644] patch | blob
Source/WebCore/ChangeLog patch | blob | history
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp patch | blob | history
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h patch | blob | history

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 161e28e..a22f5e7 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2018-05-16  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
+        https://bugs.webkit.org/show_bug.cgi?id=185299
+        <rdar://problem/39630230>
+
+        Reviewed by Simon Fraser.
+
+        Add a new test where we interrupt a transition and check that upon returning to the original value,
+        an animated value is still used and not the initial value. This test fails prior to this patch.
+
+        * transitions/interrupted-transition-hardware-expected.html: Added.
+        * transitions/interrupted-transition-hardware.html: Added.
+
 2018-05-15  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r231765.
 2018-05-15  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r231765.
diff --git a/LayoutTests/transitions/interrupted-transition-hardware-expected.html b/LayoutTests/transitions/interrupted-transition-hardware-expected.html
new file mode 100644 (file)
index 0000000..ff73df0
--- /dev/null
+++ b/LayoutTests/transitions/interrupted-transition-hardware-expected.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+
+        div {
+            position: absolute;
+            left: 0;
+            top: 0;
+            height: 100px;
+            width: 100px;
+            transform: translateX(25px);
+            background-color: green;
+        }
+
+    </style>
+</head>
+<body>
+    <div id="cover"></div>
+</body>
+</html>
diff --git a/LayoutTests/transitions/interrupted-transition-hardware.html b/LayoutTests/transitions/interrupted-transition-hardware.html
new file mode 100644 (file)
index 0000000..b6e77a5
--- /dev/null
+++ b/LayoutTests/transitions/interrupted-transition-hardware.html
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+
+        div {
+            position: absolute;
+            left: 0;
+            top: 0;
+            height: 100px;
+            width: 100px;
+        }
+        
+        #test {
+            background-color: red;
+            transition: transform calc(24 * 60 * 60s) linear;
+            transition-delay: calc(-12 * 60 * 60s);
+            transform: none;
+        }
+
+        #test.transitions {
+            transform: translateX(100px);
+        }
+
+        #cover {
+            transform: translateX(25px);
+            background-color: green;
+        }
+
+    </style>
+</head>
+<body>
+
+    <div id="test"></div>
+    <div id="cover"></div>
+
+    <script type="text/javascript">
+
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        const test = document.getElementById("test");
+
+        requestAnimationFrame(() => {
+            test.classList.add("transitions");
+            requestAnimationFrame(() => {
+                test.classList.remove("transitions");
+                requestAnimationFrame(() => {
+                    requestAnimationFrame(() => {
+                        if (window.testRunner)
+                            testRunner.notifyDone();
+                    });
+                });
+            });
+        });
+        
+    </script>
+
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 9cc5dfd..2af26a5 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,27 @@
+2018-05-16  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
+        https://bugs.webkit.org/show_bug.cgi?id=185299
+        <rdar://problem/39630230>
+
+        Reviewed by Simon Fraser.
+
+        In r230574, the fix for webkit.org/b/184518, we changed the processing order in GraphicsLayerCA::updateAnimations() to first
+        process m_uncomittedAnimations and then m_animationsToProcess, so we are guaranteed animations exist before we attempt to pause
+        or seek them. This broke interrupting and resuming hardware animations (such as an interrupted CSS Transition or an animation
+        running in a non-visible tab) since a pause operation recorded _before_ an animation was added would be paused anyway since
+        the animation was now first added, and then paused. The fix is simply to clear any pending AnimationProcessingAction for a
+        newly-uncommitted animation.
+
+        Test: transitions/interrupted-transition-hardware.html
+
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayerCA::createAnimationFromKeyframes):
+        (WebCore::GraphicsLayerCA::appendToUncommittedAnimations):
+        (WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
+        * platform/graphics/ca/GraphicsLayerCA.h:
+        (WebCore::GraphicsLayerCA::LayerPropertyAnimation::LayerPropertyAnimation):
+
 2018-05-15  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Check TypeInfo first before calling getCallData when we would like to check whether given object is a function
 2018-05-15  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Check TypeInfo first before calling getCallData when we would like to check whether given object is a function
diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
index 2b35a97..0a02a35 100644 (file)
--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
@@ -3015,7 +3015,7 @@ bool GraphicsLayerCA::createAnimationFromKeyframes(const KeyframeValueList& valu
     if (!valuesOK)
         return false;
 
     if (!valuesOK)
         return false;
 
-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
 
     return true;
 }
 
     return true;
 }
@@ -3042,7 +3042,7 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val
     if (!validMatrices)
         return false;
 
     if (!validMatrices)
         return false;
 
-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
     return true;
 }
 
     return true;
 }
 
@@ -3104,12 +3104,21 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val
         
         ASSERT(valuesOK);
 
         
         ASSERT(valuesOK);
 
-        m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
+        appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
     }
 
     return true;
 }
 
     }
 
     return true;
 }
 
+void GraphicsLayerCA::appendToUncommittedAnimations(LayerPropertyAnimation&& animation)
+{
+    // Since we're adding a new animation, make sure we clear any pending AnimationProcessingAction for this animation
+    // as these are applied after we've committed new animations.
+    m_animationsToProcess.remove(animation.m_name);
+
+    m_uncomittedAnimations.append(WTFMove(animation));
+}
+
 bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, Seconds timeOffset)
 {
 #if ENABLE(FILTERS_LEVEL_2)
 bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, Seconds timeOffset)
 {
 #if ENABLE(FILTERS_LEVEL_2)
diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
index 5011ccd..ee41cd4 100644 (file)
--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
@@ -459,8 +459,29 @@ private:
         moveOrCopyAnimations(Copy, fromLayer, toLayer);
     }
 
         moveOrCopyAnimations(Copy, fromLayer, toLayer);
     }
 
+    // This represents the animation of a single property. There may be multiple transform animations for
+    // a single transition or keyframe animation, so index is used to distinguish these.
+    struct LayerPropertyAnimation {
+        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
+            : m_animation(WTFMove(caAnimation))
+            , m_name(animationName)
+            , m_property(property)
+            , m_index(index)
+            , m_subIndex(subIndex)
+            , m_timeOffset(timeOffset)
+        { }
+
+        RefPtr<PlatformCAAnimation> m_animation;
+        String m_name;
+        AnimatedPropertyID m_property;
+        int m_index;
+        int m_subIndex;
+        Seconds m_timeOffset;
+    };
+
     bool appendToUncommittedAnimations(const KeyframeValueList&, const TransformOperations*, const Animation*, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool isMatrixAnimation);
     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
     bool appendToUncommittedAnimations(const KeyframeValueList&, const TransformOperations*, const Animation*, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool isMatrixAnimation);
     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
+    void appendToUncommittedAnimations(LayerPropertyAnimation&&);
 
     enum LayerChange : uint64_t {
         NoChange                                = 0,
 
     enum LayerChange : uint64_t {
         NoChange                                = 0,
@@ -572,26 +593,6 @@ private:
     RetainPtr<CGImageRef> m_uncorrectedContentsImage;
     RetainPtr<CGImageRef> m_pendingContentsImage;
     
     RetainPtr<CGImageRef> m_uncorrectedContentsImage;
     RetainPtr<CGImageRef> m_pendingContentsImage;
     
-    // This represents the animation of a single property. There may be multiple transform animations for
-    // a single transition or keyframe animation, so index is used to distinguish these.
-    struct LayerPropertyAnimation {
-        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
-            : m_animation(WTFMove(caAnimation))
-            , m_name(animationName)
-            , m_property(property)
-            , m_index(index)
-            , m_subIndex(subIndex)
-            , m_timeOffset(timeOffset)
-        { }
-
-        RefPtr<PlatformCAAnimation> m_animation;
-        String m_name;
-        AnimatedPropertyID m_property;
-        int m_index;
-        int m_subIndex;
-        Seconds m_timeOffset;
-    };
-    
     // Uncommitted transitions and animations.
     Vector<LayerPropertyAnimation> m_uncomittedAnimations;
     
     // Uncommitted transitions and animations.
     Vector<LayerPropertyAnimation> m_uncomittedAnimations;
     
Mirror of the WebKit open source project from svn.webkit.org.