REGRESSION (r233268): contents of an animated element inside overflow:hidden disappear
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2018 19:05:43 +0000 (19:05 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2018 19:05:43 +0000 (19:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188655
rdar://problem/43382687

Reviewed by Antoine Quint.

Source/WebCore:

The logic that computes animation extent, used by backing store attachment code, failed
to account for the behavior where a keyframe animation with a missing 0% keyframe uses
the transform from the unanimated style. This resulted in the computed extent being wrong,
which caused us to remove the layer's backing store in some scenarios.

Fix both animation code paths to use the renderer style if the first keyframe doesn't
contain a transform.

Tests: compositing/backing/backing-store-attachment-empty-keyframe.html
       legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html

* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::computeExtentOfTransformAnimation const):
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::computeExtentOfTransformAnimation const):

LayoutTests:

* compositing/backing/backing-store-attachment-empty-keyframe-expected.txt: Added.
* compositing/backing/backing-store-attachment-empty-keyframe.html: Added.
* legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt: Added.
* legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt [new file with mode: 0644]
LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe.html [new file with mode: 0644]
LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt [new file with mode: 0644]
LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/page/animation/KeyframeAnimation.cpp

index e5c404a..b50c9eb 100644 (file)
@@ -1,3 +1,16 @@
+2018-12-14  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r233268): contents of an animated element inside overflow:hidden disappear
+        https://bugs.webkit.org/show_bug.cgi?id=188655
+        rdar://problem/43382687
+
+        Reviewed by Antoine Quint.
+
+        * compositing/backing/backing-store-attachment-empty-keyframe-expected.txt: Added.
+        * compositing/backing/backing-store-attachment-empty-keyframe.html: Added.
+        * legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt: Added.
+        * legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html: Added.
+
 2018-12-14  Zalan Bujtas  <zalan@apple.com>
 
         Unreviewed test gardening.
diff --git a/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
new file mode 100644 (file)
index 0000000..e44d701
--- /dev/null
@@ -0,0 +1,30 @@
+(GraphicsLayer
+  (anchor 0.00 0.00)
+  (bounds 800.00 600.00)
+  (backingStoreAttached 1)
+  (children 1
+    (GraphicsLayer
+      (bounds 800.00 600.00)
+      (contentsOpaque 1)
+      (backingStoreAttached 1)
+      (children 1
+        (GraphicsLayer
+          (offsetFromRenderer width=-501 height=0)
+          (position 9.00 101.00)
+          (bounds 501.00 150.00)
+          (backingStoreAttached 1)
+          (children 1
+            (GraphicsLayer
+              (position 501.00 0.00)
+              (bounds 520.00 170.00)
+              (contentsOpaque 1)
+              (drawsContent 1)
+              (backingStoreAttached 1)
+            )
+          )
+        )
+      )
+    )
+  )
+)
+Some text here to force backing store.
diff --git a/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe.html b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe.html
new file mode 100644 (file)
index 0000000..212131c
--- /dev/null
@@ -0,0 +1,73 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+    .container {
+        position: absolute;
+        top: 100px;
+        overflow: hidden;
+        width: 500px;
+        height: 150px;
+        border: 1px solid black;
+    }
+
+    .mover {
+        position: absolute;
+        width: 500px;
+        height: 100%;
+        left: 100%;
+        transform: translateX(-100%);
+        background-color: silver;
+        padding: 10px;
+    }
+    
+    .mover.animating {
+        animation: slide 1s linear forwards;
+    }
+
+    @keyframes slide {
+        100% { transform: translateX(0) }
+    }
+</style>
+<script>
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    function dumpLayerTree()
+    {
+        if (!window.internals)
+            return;
+
+        var out = document.getElementById('out');
+        out.innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
+    }
+
+    function dumpLayersSoon()
+    {
+        requestAnimationFrame(function() {
+            // Trigger layer flush
+            document.querySelector('.container').style.width = '501px';
+            dumpLayerTree();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        });
+    }
+
+    window.addEventListener('load', () => {
+        let box = document.getElementById('box');
+        box.addEventListener('animationstart', dumpLayersSoon, false);
+        box.classList.add('animating');
+    }, false);
+</script>
+</head>
+<body>
+<pre id="out"></pre>
+<div class="container">
+    <div id="box" class="mover">
+        Some text here to force backing store.
+    </div>
+</div>
+</body>
+</html>
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
new file mode 100644 (file)
index 0000000..840c4fe
--- /dev/null
@@ -0,0 +1,31 @@
+(GraphicsLayer
+  (anchor 0.00 0.00)
+  (bounds 800.00 600.00)
+  (backingStoreAttached 1)
+  (children 1
+    (GraphicsLayer
+      (bounds 800.00 600.00)
+      (contentsOpaque 1)
+      (backingStoreAttached 1)
+      (children 1
+        (GraphicsLayer
+          (offsetFromRenderer width=-501 height=0)
+          (position 9.00 101.00)
+          (bounds 501.00 150.00)
+          (backingStoreAttached 1)
+          (children 1
+            (GraphicsLayer
+              (position 501.00 0.00)
+              (bounds 520.00 170.00)
+              (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])
+            )
+          )
+        )
+      )
+    )
+  )
+)
+Some text here to force backing store.
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
new file mode 100644 (file)
index 0000000..281f09c
--- /dev/null
@@ -0,0 +1,73 @@
+<!DOCTYPE html><!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=false ] -->
+<html>
+<head>
+<style>
+    .container {
+        position: absolute;
+        top: 100px;
+        overflow: hidden;
+        width: 500px;
+        height: 150px;
+        border: 1px solid black;
+    }
+
+    .mover {
+        position: absolute;
+        width: 500px;
+        height: 100%;
+        left: 100%;
+        transform: translateX(-100%);
+        background-color: silver;
+        padding: 10px;
+    }
+    
+    .mover.animating {
+        animation: slide 1s linear forwards;
+    }
+
+    @keyframes slide {
+        100% { transform: translateX(0) }
+    }
+</style>
+<script>
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    function dumpLayerTree()
+    {
+        if (!window.internals)
+            return;
+
+        var out = document.getElementById('out');
+        out.innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
+    }
+
+    function dumpLayersSoon()
+    {
+        requestAnimationFrame(function() {
+            // Trigger layer flush
+            document.querySelector('.container').style.width = '501px';
+            dumpLayerTree();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        });
+    }
+
+    window.addEventListener('load', () => {
+        let box = document.getElementById('box');
+        box.addEventListener('animationstart', dumpLayersSoon, false);
+        box.classList.add('animating');
+    }, false);
+</script>
+</head>
+<body>
+<pre id="out"></pre>
+<div class="container">
+    <div id="box" class="mover">
+        Some text here to force backing store.
+    </div>
+</div>
+</body>
+</html>
index 4fce43c..854ee33 100644 (file)
@@ -1,3 +1,27 @@
+2018-12-14  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r233268): contents of an animated element inside overflow:hidden disappear
+        https://bugs.webkit.org/show_bug.cgi?id=188655
+        rdar://problem/43382687
+
+        Reviewed by Antoine Quint.
+
+        The logic that computes animation extent, used by backing store attachment code, failed
+        to account for the behavior where a keyframe animation with a missing 0% keyframe uses
+        the transform from the unanimated style. This resulted in the computed extent being wrong,
+        which caused us to remove the layer's backing store in some scenarios.
+
+        Fix both animation code paths to use the renderer style if the first keyframe doesn't
+        contain a transform.
+
+        Tests: compositing/backing/backing-store-attachment-empty-keyframe.html
+               legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::computeExtentOfTransformAnimation const):
+        * page/animation/KeyframeAnimation.cpp:
+        (WebCore::KeyframeAnimation::computeExtentOfTransformAnimation const):
+
 2018-12-14  Chris Dumez  <cdumez@apple.com>
 
         [PSON] Stop exposing PolicyAction::Suspend to WebCore
index 11c878a..ce1bb32 100644 (file)
@@ -1408,23 +1408,30 @@ bool KeyframeEffect::computeExtentOfTransformAnimation(LayoutRect& bounds) const
     if (!is<RenderBox>(renderer()))
         return true; // Non-boxes don't get transformed;
 
-    RenderBox& box = downcast<RenderBox>(*renderer());
-    FloatRect rendererBox = snapRectToDevicePixels(box.borderBoxRect(), box.document().deviceScaleFactor());
+    auto& box = downcast<RenderBox>(*renderer());
+    auto rendererBox = snapRectToDevicePixels(box.borderBoxRect(), box.document().deviceScaleFactor());
 
-    FloatRect cumulativeBounds = bounds;
+    auto cumulativeBounds = bounds;
 
     for (const auto& keyframe : m_blendingKeyframes.keyframes()) {
+        const auto* keyframeStyle = keyframe.style();
+
         // FIXME: maybe for declarative animations we always say it's true for the first and last keyframe.
-        if (!keyframe.containsProperty(CSSPropertyTransform))
-            continue;
+        if (!keyframe.containsProperty(CSSPropertyTransform)) {
+            // If the first keyframe is missing transform style, use the current style.
+            if (!keyframe.key())
+                keyframeStyle = &box.style();
+            else
+                continue;
+        }
 
-        LayoutRect keyframeBounds = bounds;
+        auto keyframeBounds = bounds;
 
         bool canCompute;
         if (transformFunctionListsMatch())
-            canCompute = computeTransformedExtentViaTransformList(rendererBox, *keyframe.style(), keyframeBounds);
+            canCompute = computeTransformedExtentViaTransformList(rendererBox, *keyframeStyle, keyframeBounds);
         else
-            canCompute = computeTransformedExtentViaMatrix(rendererBox, *keyframe.style(), keyframeBounds);
+            canCompute = computeTransformedExtentViaMatrix(rendererBox, *keyframeStyle, keyframeBounds);
 
         if (!canCompute)
             return false;
@@ -1432,7 +1439,7 @@ bool KeyframeEffect::computeExtentOfTransformAnimation(LayoutRect& bounds) const
         cumulativeBounds.unite(keyframeBounds);
     }
 
-    bounds = LayoutRect(cumulativeBounds);
+    bounds = cumulativeBounds;
     return true;
 }
 
index fefe010..f999ff6 100644 (file)
@@ -245,22 +245,29 @@ bool KeyframeAnimation::computeExtentOfTransformAnimation(LayoutRect& bounds) co
     if (!is<RenderBox>(renderer()))
         return true; // Non-boxes don't get transformed;
 
-    RenderBox& box = downcast<RenderBox>(*renderer());
-    FloatRect rendererBox = snapRectToDevicePixels(box.borderBoxRect(), box.document().deviceScaleFactor());
+    auto& box = downcast<RenderBox>(*renderer());
+    auto rendererBox = snapRectToDevicePixels(box.borderBoxRect(), box.document().deviceScaleFactor());
 
-    FloatRect cumulativeBounds = bounds;
+    auto cumulativeBounds = bounds;
 
     for (auto& keyframe : m_keyframes.keyframes()) {
-        if (!keyframe.containsProperty(CSSPropertyTransform))
-            continue;
+        const RenderStyle* keyframeStyle = keyframe.style();
+
+        if (!keyframe.containsProperty(CSSPropertyTransform)) {
+            // If the first keyframe is missing transform style, use the current style.
+            if (!keyframe.key())
+                keyframeStyle = &box.style();
+            else
+                continue;
+        }
 
-        LayoutRect keyframeBounds = bounds;
+        auto keyframeBounds = bounds;
         
         bool canCompute;
         if (transformFunctionListsMatch())
-            canCompute = computeTransformedExtentViaTransformList(rendererBox, *keyframe.style(), keyframeBounds);
+            canCompute = computeTransformedExtentViaTransformList(rendererBox, *keyframeStyle, keyframeBounds);
         else
-            canCompute = computeTransformedExtentViaMatrix(rendererBox, *keyframe.style(), keyframeBounds);
+            canCompute = computeTransformedExtentViaMatrix(rendererBox, *keyframeStyle, keyframeBounds);
         
         if (!canCompute)
             return false;
@@ -268,7 +275,7 @@ bool KeyframeAnimation::computeExtentOfTransformAnimation(LayoutRect& bounds) co
         cumulativeBounds.unite(keyframeBounds);
     }
 
-    bounds = LayoutRect(cumulativeBounds);
+    bounds = cumulativeBounds;
     return true;
 }