Attachment 368531 Details for Bug 197401 – Patch

[patch] Patch

bug-197401-20190429194936.patch (text/plain), 18.83 KB, created by Simon Fraser (smfr) on 2019-04-29 19:49:37 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-04-29 19:49:37 PDT

Size: 18.83 KB

patch

obsolete

>Subversion Revision: 244752
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5dfdad8ac0c1937e4bc178424d1b4254fa01b428..695d1ec163f69c8284c72b2881a598c9da090252 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-04-29  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Transform is sometimes left in a bad state after an animation
>+        https://bugs.webkit.org/show_bug.cgi?id=197401
>+        rdar://problem/48179186
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        In some more complex compositing scenarios, at the end of an animation we'd
>+        fail to push a new transform onto a layer, because updateGeometry() would
>+        think there's an animation running (which there is, but in the "Ending" state).
>+
>+        It's simpler in this code to just always push transform and opacity to the layer;
>+        they will get overridden by the animation while it's running. The current code
>+        dates from the first landing of the file, and the reason for the if (!isRunningAcceleratedTransformAnimation)
>+        check is lost in the sands of time.
>+
>+        I was not able to get a reliable ref or layer tree test for this, because the next compositing update
>+        fixes it, and WTR seems to trigger one.  But the added test does show the bug
>+        in Safari, and is a good test to have.
>+
>+        Test: compositing/animation/transform-after-animation.html
>+
>+        * rendering/RenderLayerBacking.cpp:
>+        (WebCore::RenderLayerBacking::updateGeometry):
>+
> 2019-04-29  Youenn Fablet  <youenn@apple.com>
> 
>         getDisplayMedia should be called on user gesture
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
>index b384339ebb8e66a96acf9d5d61ca748776d2c5b0..1921292fb7d28603ecd0f8e8c9e53610c35dba6b 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
>+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
>@@ -1001,26 +1001,14 @@ void RenderLayerBacking::updateGeometry()
>     const RenderStyle& style = renderer().style();
> 
>     bool isRunningAcceleratedTransformAnimation = false;
>-    bool isRunningAcceleratedOpacityAnimation = false;
>     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
>-        if (auto* timeline = renderer().documentTimeline()) {
>+        if (auto* timeline = renderer().documentTimeline())
>             isRunningAcceleratedTransformAnimation = timeline->isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyTransform);
>-            isRunningAcceleratedOpacityAnimation = timeline->isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyOpacity);
>-        }
>-    } else {
>+    } else
>         isRunningAcceleratedTransformAnimation = renderer().animation().isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyTransform);
>-        isRunningAcceleratedOpacityAnimation = renderer().animation().isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyOpacity);
>-    }
>-
>-    // Set transform property, if it is not animating. We have to do this here because the transform
>-    // is affected by the layer dimensions.
>-    if (!isRunningAcceleratedTransformAnimation)
>-        updateTransform(style);
>-
>-    // Set opacity, if it is not animating.
>-    if (!isRunningAcceleratedOpacityAnimation)
>-        updateOpacity(style);
> 
>+    updateTransform(style);
>+    updateOpacity(style);
>     updateFilters(style);
> #if ENABLE(FILTERS_LEVEL_2)
>     updateBackdropFilters(style);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 08c590b25d9864ec82c3a550e370dbf348145dd4..93a4047ff3bd4f11cd6fbdf5a395f99e40979471 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-04-29  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Transform is sometimes left in a bad state after an animation
>+        https://bugs.webkit.org/show_bug.cgi?id=197401
>+        rdar://problem/48179186
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        In tests that dump layer trees at times that can snapshot animating transforms, filter
>+        the transform out of the layer tree dump (it's not the interesting part).
>+
>+        * compositing/animation/transform-after-animation-expected.html: Added.
>+        * compositing/animation/transform-after-animation.html: Added.
>+        * compositing/backing/backing-store-attachment-empty-keyframe-expected.txt:
>+        * legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt:
>+        * legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html:
>+        * legacy-animation-engine/compositing/backing/transform-transition-from-outside-view-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/mismatched-rotated-transform-transition-overlap-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/mismatched-transform-transition-overlap-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap.html:
>+        * legacy-animation-engine/compositing/layer-creation/translate-scale-transition-overlap-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/translate-transition-overlap-expected.txt:
>+        * legacy-animation-engine/compositing/layer-creation/translate-transition-overlap.html:
>+
> 2019-04-29  Javier Fernandez  <jfernandez@igalia.com>
> 
>         line should not be broken before the first space after a word
>diff --git a/LayoutTests/compositing/animation/transform-after-animation-expected.html b/LayoutTests/compositing/animation/transform-after-animation-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..4cadef71eb42d645fe1c3861392a3c210fea6b24
>--- /dev/null
>+++ b/LayoutTests/compositing/animation/transform-after-animation-expected.html
>@@ -0,0 +1,26 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        .stage {
>+            margin: 20px;
>+            width: 300px; height: 250px;
>+            border: 2px solid black;
>+            transform-style: preserve-3d;
>+        }
>+        
>+        .slide {
>+            width: 100%;
>+            height: 100%;
>+            -webkit-backface-visibility: hidden;
>+            background-color: blue;
>+            transform: translateX(0px);
>+        }
>+    </style>
>+</head>
>+<body>
>+    <div class="stage">
>+        <div id="target" class="slide" onanimationend="animationEnded()"></div>
>+    </div>
>+</body>
>+</html>
>diff --git a/LayoutTests/compositing/animation/transform-after-animation.html b/LayoutTests/compositing/animation/transform-after-animation.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..fe51391867fd61cc0e201543bb66c257348c198e
>--- /dev/null
>+++ b/LayoutTests/compositing/animation/transform-after-animation.html
>@@ -0,0 +1,51 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        .stage {
>+            margin: 20px;
>+            width: 300px; height: 250px;
>+            border: 2px solid black;
>+            transform-style: preserve-3d;
>+        }
>+        
>+        .slide {
>+            width: 100%;
>+            height: 100%;
>+            animation-duration: 20ms;
>+            animation-fill-mode: forwards;
>+            -webkit-backface-visibility: hidden;
>+            background-color: blue;
>+        }
>+        
>+        .animating {
>+            animation-name: slide;
>+        }
>+        
>+        @keyframes slide {
>+            0% {   transform-origin: 50% 50%; transform: translateX(400px); } 
>+            50% {  transform-origin: 50% 50%; transform: translateX(0px); } 
>+            100% { transform-origin: 50% 50%; transform: translateX(0px); }             
>+        }
>+    </style>
>+    <script>
>+        if (window.testRunner)
>+            testRunner.waitUntilDone();
>+
>+        function animationEnded()
>+        {
>+            if (window.testRunner)
>+                testRunner.notifyDone();
>+        }
>+
>+        window.addEventListener('load', () => {
>+            document.getElementById('target').classList.add('animating');
>+        }, false);
>+    </script>
>+</head>
>+<body>
>+    <div class="stage">
>+        <div id="target" class="slide" onanimationend="animationEnded()"></div>
>+    </div>
>+</body>
>+</html>
>diff --git a/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>index e44d70180742c06fc5111b396e7099d97790e881..840c4fed5b85717602d8ebbeda116a9ce98a02db 100644
>--- a/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>+++ b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>@@ -20,6 +20,7 @@
>               (contentsOpaque 1)
>               (drawsContent 1)
>               (backingStoreAttached 1)
>+              (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [-520.00 0.00 0.00 1.00])
>             )
>           )
>         )
>diff --git a/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>index 840c4fed5b85717602d8ebbeda116a9ce98a02db..e44d70180742c06fc5111b396e7099d97790e881 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>@@ -20,7 +20,6 @@
>               (contentsOpaque 1)
>               (drawsContent 1)
>               (backingStoreAttached 1)
>-              (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [-520.00 0.00 0.00 1.00])
>             )
>           )
>         )
>diff --git a/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html
>index 281f09c51c528ea4ef86db2a0a36adda47ea40a7..650237254cafe330c6faab5f7edf12226ce978d2 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html
>+++ b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html
>@@ -34,14 +34,20 @@
>         testRunner.dumpAsText();
>         testRunner.waitUntilDone();
>     }
>+    
>+    function layerTreeWithoutTransforms()
>+    {
>+        var layerTreeText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
>+        var filteredLines = layerTreeText.split("\n").filter(line => line.indexOf('transform') == -1);
>+        return filteredLines.join('\n');
>+    }
> 
>     function dumpLayerTree()
>     {
>         if (!window.internals)
>             return;
> 
>-        var out = document.getElementById('out');
>-        out.innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
>+        document.getElementById('out').textContent = layerTreeWithoutTransforms();
>     }
> 
>     function dumpLayersSoon()
>diff --git a/LayoutTests/legacy-animation-engine/compositing/backing/transform-transition-from-outside-view-expected.txt b/LayoutTests/legacy-animation-engine/compositing/backing/transform-transition-from-outside-view-expected.txt
>index 85cdf2d23bf31176a0ff4e6e1dd5931be3816d4b..90fd7e6929b46f566a7f4343580f71b04f53299e 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/backing/transform-transition-from-outside-view-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/backing/transform-transition-from-outside-view-expected.txt
>@@ -14,6 +14,7 @@
>           (bounds 148.00 128.00)
>           (drawsContent 1)
>           (backingStoreAttached 1)
>+          (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [-400.00 0.00 0.00 1.00])
>           (children 1
>             (GraphicsLayer
>               (offsetFromRenderer width=-14 height=-14)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-rotated-transform-transition-overlap-expected.txt b/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-rotated-transform-transition-overlap-expected.txt
>index 7a8bd3ddf39374c1b5fd05965b31c7d1429454f1..d3c156c51f3beecf73b8f3e66dd4aa9366388923 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-rotated-transform-transition-overlap-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-rotated-transform-transition-overlap-expected.txt
>@@ -12,7 +12,7 @@
>           (anchor 0.74 0.27)
>           (bounds 148.00 128.00)
>           (drawsContent 1)
>-          (transform [1.30 0.02 0.00 0.00] [-0.02 1.30 0.00 0.00] [0.00 0.00 1.00 0.00] [0.00 0.00 0.00 1.00])
>+          (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [10.00 0.00 0.00 1.00])
>         )
>         (GraphicsLayer
>           (bounds 4.00 4.00)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-transform-transition-overlap-expected.txt b/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-transform-transition-overlap-expected.txt
>index 9c995bd2e06cd37719ac6831f350e008391513f6..2c3bb1166ebfa801671d8ba399a9cec7c34feef4 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-transform-transition-overlap-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/mismatched-transform-transition-overlap-expected.txt
>@@ -12,7 +12,7 @@
>           (anchor 0.74 0.27)
>           (bounds 148.00 128.00)
>           (drawsContent 1)
>-          (transform [1.30 0.00 0.00 0.00] [0.00 1.30 0.00 0.00] [0.00 0.00 1.00 0.00] [0.00 0.00 0.00 1.00])
>+          (transform [1.00 -0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [10.00 0.00 0.00 1.00])
>         )
>         (GraphicsLayer
>           (position 60.00 80.00)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap-expected.txt b/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap-expected.txt
>index 2067abdf0d8eddac875a1394faaee519129aff10..68fc7b5f22f6b77eacd293d7f4e5db240781cfee 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap-expected.txt
>@@ -12,7 +12,6 @@
>           (anchor 0.74 0.27)
>           (bounds 148.00 128.00)
>           (drawsContent 1)
>-          (transform [0.92 0.92 0.00 0.00] [-0.92 0.92 0.00 0.00] [0.00 0.00 1.00 0.00] [0.00 0.00 0.00 1.00])
>         )
>         (GraphicsLayer
>           (position 20.00 10.00)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap.html b/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap.html
>index f4b9ad0b8c0c4c3dc61fc0409fef2d3a707453b6..5d08902a5799823f0fed39c19cd0e8105d3140a0 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap.html
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/scale-rotation-transition-overlap.html
>@@ -33,10 +33,17 @@
>             testRunner.waitUntilDone();
>         }
> 
>+        function layerTreeWithoutTransforms()
>+        {
>+            var layerTreeText = internals.layerTreeAsText(document);
>+            var filteredLines = layerTreeText.split("\n").filter(line => line.indexOf('transform') == -1);
>+            return filteredLines.join('\n');
>+        }
>+
>         function dumpLayers()
>         {
>             if (window.testRunner) {
>-                document.getElementById('layers').innerText = window.internals.layerTreeAsText(document);
>+                document.getElementById('layers').innerText = layerTreeWithoutTransforms();
>                 testRunner.notifyDone();
>             }
>         }
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-scale-transition-overlap-expected.txt b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-scale-transition-overlap-expected.txt
>index 55b4e27b0ead78a47335e9734389e2ac8514e0d0..fccb59f8c8e9a31745f4da334f0b51e3008d9bbf 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-scale-transition-overlap-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-scale-transition-overlap-expected.txt
>@@ -12,7 +12,6 @@
>           (anchor 0.11 0.89)
>           (bounds 228.00 128.00)
>           (drawsContent 1)
>-          (transform [1.30 0.00 0.00 0.00] [0.00 1.30 0.00 0.00] [0.00 0.00 1.00 0.00] [100.00 0.00 0.00 1.00])
>         )
>         (GraphicsLayer
>           (position 30.00 30.00)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap-expected.txt b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap-expected.txt
>index dc57596f788ee515d26bd5eb26c6c4dbcf01d5ae..204b75a2b97e1d7d9c302364b2a1cf0861ee0f2d 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap-expected.txt
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap-expected.txt
>@@ -11,7 +11,6 @@
>           (position 24.00 38.00)
>           (bounds 228.00 128.00)
>           (drawsContent 1)
>-          (transform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 0.00] [100.00 0.00 0.00 1.00])
>         )
>         (GraphicsLayer
>           (position 30.00 40.00)
>diff --git a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap.html b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap.html
>index af820741833c3390c999c33b0602feabf5ca4574..7a89ad07511bea064ce5176703266bf323e09f60 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap.html
>+++ b/LayoutTests/legacy-animation-engine/compositing/layer-creation/translate-transition-overlap.html
>@@ -33,10 +33,17 @@
>             testRunner.waitUntilDone();
>         }
> 
>+        function layerTreeWithoutTransforms()
>+        {
>+            var layerTreeText = internals.layerTreeAsText(document);
>+            var filteredLines = layerTreeText.split("\n").filter(line => line.indexOf('transform') == -1);
>+            return filteredLines.join('\n');
>+        }
>+
>         function dumpLayers()
>         {
>             if (window.testRunner) {
>-                document.getElementById('layers').innerText = window.internals.layerTreeAsText(document);
>+                document.getElementById('layers').innerText = layerTreeWithoutTransforms();
>                 testRunner.notifyDone();
>             }
>         }

Actions: View | Formatted Diff | Diff

Attachments on bug 197401: 368531 | 368535 | 368541 | 368575 | 368583