Attachment 357283 Details for Bug 188655 – Patch

[patch] Patch

bug-188655-20181213185045.patch (text/plain), 14.18 KB, created by Simon Fraser (smfr) on 2018-12-13 18:50:46 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2018-12-13 18:50:46 PST

Size: 14.18 KB

patch

obsolete

>Subversion Revision: 239170
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c7c333bbc491e236532813334d0f41a531b75075..ee395b9b8c83ae10bba4de646a28179781698cde 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-12-13  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 NOBODY (OOPS!).
>+
>+        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-13  Eric Carlson  <eric.carlson@apple.com>
> 
>         [MediaStream] Calculate width or height when constraints contain only the other
>diff --git a/Source/WebCore/animation/KeyframeEffect.cpp b/Source/WebCore/animation/KeyframeEffect.cpp
>index 11c878aef6b20127829afb41a772c2c0cb77ec8d..ce1bb329a2e30402b5c4a1c27eef8ec9e2a4228b 100644
>--- a/Source/WebCore/animation/KeyframeEffect.cpp
>+++ b/Source/WebCore/animation/KeyframeEffect.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebCore/page/animation/KeyframeAnimation.cpp b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>index fefe0101150177b3c16e5db8c83804cb6063967e..f999ff643c50b7824cf9263a56431ea59d7d8b18 100644
>--- a/Source/WebCore/page/animation/KeyframeAnimation.cpp
>+++ b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>@@ -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;
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index fc660cd3292734e120aa41ed9f5baa9dbf919b19..0f458ede4ac214c246004f857eb582b957465364 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-12-13  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 NOBODY (OOPS!).
>+
>+        * 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-13  Eric Carlson  <eric.carlson@apple.com>
> 
>         [MediaStream] Calculate width or height when constraints contain only the other
>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
>index 0000000000000000000000000000000000000000..fc41ef0988ed4f1196352d49d500c08f556037bd
>--- /dev/null
>+++ b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>@@ -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)
>+              (anchor 0.50 0.50)
>+              (bounds 521.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
>index 0000000000000000000000000000000000000000..07ebba6d43fdf91b420c451e8aaea475b460451d
>--- /dev/null
>+++ b/LayoutTests/compositing/backing/backing-store-attachment-empty-keyframe.html
>@@ -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: 100%;
>+        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
>index 0000000000000000000000000000000000000000..9b46f232b9daf2a496c50c3f51ba03de980f0a24
>--- /dev/null
>+++ b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe-expected.txt
>@@ -0,0 +1,32 @@
>+(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)
>+              (anchor 0.50 0.50)
>+              (bounds 521.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
>index 0000000000000000000000000000000000000000..5d6b063eff16e90a9e6d4d4c6ad36d6568827555
>--- /dev/null
>+++ b/LayoutTests/legacy-animation-engine/compositing/backing/backing-store-attachment-empty-keyframe.html
>@@ -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: 100%;
>+        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>

Flags:

graouts: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 188655: 347266 | 354513 | 357283 | 357295