Title: [231765] trunk
Revision
231765
Author
grao...@webkit.org
Date
2018-05-14 11:15:57 -0700 (Mon, 14 May 2018)

Log Message

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.

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.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (231764 => 231765)


--- trunk/LayoutTests/ChangeLog	2018-05-14 17:39:16 UTC (rev 231764)
+++ trunk/LayoutTests/ChangeLog	2018-05-14 18:15:57 UTC (rev 231765)
@@ -1,3 +1,17 @@
+2018-05-14  Antoine Quint  <grao...@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-14  Jeremy Jones  <jere...@apple.com>
 
         NSEvent event trackers don't work from WebKitTestRunner

Added: trunk/LayoutTests/transitions/interrupted-transition-hardware-expected.html (0 => 231765)


--- trunk/LayoutTests/transitions/interrupted-transition-hardware-expected.html	                        (rev 0)
+++ trunk/LayoutTests/transitions/interrupted-transition-hardware-expected.html	2018-05-14 18:15:57 UTC (rev 231765)
@@ -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>

Added: trunk/LayoutTests/transitions/interrupted-transition-hardware.html (0 => 231765)


--- trunk/LayoutTests/transitions/interrupted-transition-hardware.html	                        (rev 0)
+++ trunk/LayoutTests/transitions/interrupted-transition-hardware.html	2018-05-14 18:15:57 UTC (rev 231765)
@@ -0,0 +1,59 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+
+        div {
+            position: absolute;
+            left: 0;
+            top: 0;
+            height: 100px;
+            width: 100px;
+        }
+        
+        #test {
+            background-color: red;
+            transition: transform 4000s linear;
+            transition-delay: -2000s;
+            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(() => {
+                    if (window.testRunner)
+                        testRunner.notifyDone();
+                });
+            });
+        });
+        
+    </script>
+
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (231764 => 231765)


--- trunk/Source/WebCore/ChangeLog	2018-05-14 17:39:16 UTC (rev 231764)
+++ trunk/Source/WebCore/ChangeLog	2018-05-14 18:15:57 UTC (rev 231765)
@@ -1,3 +1,27 @@
+2018-05-14  Antoine Quint  <grao...@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-14  Thibault Saunier  <tsaun...@igalia.com>
 
         [GStreamer] Fix style issue in MediaPlayerPrivateGStreamerBase

Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (231764 => 231765)


--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp	2018-05-14 17:39:16 UTC (rev 231764)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp	2018-05-14 18:15:57 UTC (rev 231765)
@@ -3015,7 +3015,7 @@
     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;
 }
@@ -3042,7 +3042,7 @@
     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;
 }
 
@@ -3104,12 +3104,21 @@
         
         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;
 }
 
+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)

Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h (231764 => 231765)


--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h	2018-05-14 17:39:16 UTC (rev 231764)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h	2018-05-14 18:15:57 UTC (rev 231765)
@@ -459,8 +459,29 @@
         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);
+    void appendToUncommittedAnimations(LayerPropertyAnimation&&);
 
     enum LayerChange : uint64_t {
         NoChange                                = 0,
@@ -572,26 +593,6 @@
     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;
     
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to