[iOS] Dark flash when opening Google AMP pages
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Mar 2019 01:54:59 +0000 (01:54 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Mar 2019 01:54:59 +0000 (01:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195193
rdar://problem/48326442

Reviewed by Zalan Bujtas.

Source/WebCore:

After the incremental compositing updates changes, it was possible for a change in the size
of an overflow:hidden element to fail to update the "ancestor clipping layer" geometry on
a composited descendant that is not a descendant in z-order. When Google search results
create the <iframe> that contain AMP contents, we'd fail to update a zero-sized clipping layer,
leaving the #222 background of an intermediate element visible.

Fix by setting a flag in RenderLayer::updateLayerPosition() (which is called in containing block order)
that sets the "needs geometry update" dirty bit on containing-block-descendant layers. Currently
this flag affects all descendants; in future, we might be able to clear it for grand-children.

Tests: compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html
       compositing/geometry/ancestor-clip-change.html

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::updateLayerPositions):
(WebCore::RenderLayer::updateLayerPosition):
* rendering/RenderLayer.h:
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::updateAfterLayout):
* rendering/RenderLayerBacking.h:

LayoutTests:

Tests that change the size of a clipping layer with non-z-order composited descendant, with
a couple of layer tree configurations.

* compositing/geometry/ancestor-clip-change-expected.html: Added.
* compositing/geometry/ancestor-clip-change-interleaved-stacking-context-expected.html: Added.
* compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html: Added.
* compositing/geometry/ancestor-clip-change.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/compositing/geometry/ancestor-clip-change-expected.html [new file with mode: 0644]
LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context-expected.html [new file with mode: 0644]
LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html [new file with mode: 0644]
LayoutTests/compositing/geometry/ancestor-clip-change.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerBacking.h

index c7b4515..6897a21 100644 (file)
@@ -1,3 +1,19 @@
+2019-02-28  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS] Dark flash when opening Google AMP pages
+        https://bugs.webkit.org/show_bug.cgi?id=195193
+        rdar://problem/48326442
+
+        Reviewed by Zalan Bujtas.
+
+        Tests that change the size of a clipping layer with non-z-order composited descendant, with
+        a couple of layer tree configurations.
+
+        * compositing/geometry/ancestor-clip-change-expected.html: Added.
+        * compositing/geometry/ancestor-clip-change-interleaved-stacking-context-expected.html: Added.
+        * compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html: Added.
+        * compositing/geometry/ancestor-clip-change.html: Added.
+
 2019-02-28  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: View.removeSubview not removing the element properly when not parented
diff --git a/LayoutTests/compositing/geometry/ancestor-clip-change-expected.html b/LayoutTests/compositing/geometry/ancestor-clip-change-expected.html
new file mode 100644 (file)
index 0000000..7d2c15f
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body {
+            height: 1000px;
+        }
+        .clipper {
+            margin: 20px;
+            padding: 10px;
+            width: 300px;
+            height: 300px;
+            overflow: hidden;
+            background-color: gray;
+        }
+        .content {
+            transform: translateZ(0);
+            background-color: green;
+            height: 300px;
+        }
+    </style>
+</head>
+<body>
+    <div class="clipper">
+        <div class="content">
+            &nbsp;
+        </div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context-expected.html b/LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context-expected.html
new file mode 100644 (file)
index 0000000..bdd0e49
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body {
+            height: 1000px;
+        }
+        .outer-clipper {
+            position: relative;
+            width: 360px;
+            height: 360px;
+            border: 2px solid orange;
+        }
+        .clipper {
+            margin: 20px;
+            padding: 10px;
+            width: 300px;
+            height: 300px;
+            overflow: hidden;
+            background-color: gray;
+        }
+        .content {
+            transform: translateZ(0);
+            background-color: green;
+            height: 300px;
+        }
+    </style>
+</head>
+<body>
+        <div class="outer-clipper">
+            <div class="clipper">
+                <div class="content">
+                    &nbsp;
+                </div>
+            </div>
+        </div>
+</body>
+</html>
diff --git a/LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html b/LayoutTests/compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html
new file mode 100644 (file)
index 0000000..734256e
--- /dev/null
@@ -0,0 +1,54 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body {
+            height: 1000px;
+        }
+        .outer-clipper {
+            position: relative;
+            width: 360px;
+            height: 360px;
+            border: 2px solid orange;
+        }
+        .clipper {
+            margin: 20px;
+            padding: 10px;
+            width: 300px;
+            height: 100px;
+            overflow: hidden;
+            background-color: gray;
+        }
+        .changed .clipper {
+            height: 300px;
+        }
+        .content {
+            transform: translateZ(0);
+            background-color: green;
+            height: 300px;
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        window.addEventListener('load', () => {
+            requestAnimationFrame(() => {
+                document.body.classList.add('changed');
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            });
+        }, false);
+    </script>
+</head>
+<body>
+        <div class="outer-clipper">
+            <div class="clipper">
+                <div class="content">
+                    &nbsp;
+                </div>
+            </div>
+        </div>
+</body>
+</html>
diff --git a/LayoutTests/compositing/geometry/ancestor-clip-change.html b/LayoutTests/compositing/geometry/ancestor-clip-change.html
new file mode 100644 (file)
index 0000000..90c7d27
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body {
+            height: 1000px;
+        }
+        .clipper {
+            margin: 20px;
+            padding: 10px;
+            width: 300px;
+            height: 100px;
+            overflow: hidden;
+            background-color: gray;
+        }
+        .changed .clipper {
+            height: 300px;
+        }
+        .content {
+            transform: translateZ(0);
+            background-color: green;
+            height: 300px;
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        window.addEventListener('load', () => {
+            requestAnimationFrame(() => {
+                document.body.classList.add('changed');
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            });
+        }, false);
+    </script>
+</head>
+<body>
+    <div class="clipper">
+        <div class="content">
+            &nbsp;
+        </div>
+    </div>
+</body>
+</html>
index 2e78a2b..534dadf 100644 (file)
@@ -1,3 +1,32 @@
+2019-02-28  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS] Dark flash when opening Google AMP pages
+        https://bugs.webkit.org/show_bug.cgi?id=195193
+        rdar://problem/48326442
+
+        Reviewed by Zalan Bujtas.
+
+        After the incremental compositing updates changes, it was possible for a change in the size
+        of an overflow:hidden element to fail to update the "ancestor clipping layer" geometry on
+        a composited descendant that is not a descendant in z-order. When Google search results
+        create the <iframe> that contain AMP contents, we'd fail to update a zero-sized clipping layer,
+        leaving the #222 background of an intermediate element visible.
+
+        Fix by setting a flag in RenderLayer::updateLayerPosition() (which is called in containing block order)
+        that sets the "needs geometry update" dirty bit on containing-block-descendant layers. Currently
+        this flag affects all descendants; in future, we might be able to clear it for grand-children.
+
+        Tests: compositing/geometry/ancestor-clip-change-interleaved-stacking-context.html
+               compositing/geometry/ancestor-clip-change.html
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::updateLayerPositions):
+        (WebCore::RenderLayer::updateLayerPosition):
+        * rendering/RenderLayer.h:
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::updateAfterLayout):
+        * rendering/RenderLayerBacking.h:
+
 2019-02-28  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Use-after-move in RenderCombineText::combineTextIfNeeded()
index 47a9e4d..69cb493 100644 (file)
@@ -848,10 +848,7 @@ void RenderLayer::updateLayerPositionsAfterLayout(const RenderLayer* rootLayer,
 
 void RenderLayer::updateLayerPositions(RenderGeometryMap* geometryMap, OptionSet<UpdateLayerPositionsFlag> flags)
 {
-    updateLayerPosition(); // For relpositioned layers or non-positioned layers,
-                           // we need to keep in sync, since we may have shifted relative
-                           // to our parent layer.
-
+    updateLayerPosition(&flags);
     applyPostLayoutScrollPositionIfNeeded();
 
     if (geometryMap)
@@ -952,7 +949,7 @@ void RenderLayer::updateLayerPositions(RenderGeometryMap* geometryMap, OptionSet
     }
 
     if (isComposited())
-        backing()->updateAfterLayout(flags.contains(NeedsFullRepaintInBacking));
+        backing()->updateAfterLayout(flags.contains(ContainingClippingLayerChangedSize), flags.contains(NeedsFullRepaintInBacking));
 
     if (geometryMap)
         geometryMap->popMappingsToAncestor(parent());
@@ -1498,7 +1495,7 @@ bool RenderLayer::update3DTransformedDescendantStatus()
     return has3DTransform();
 }
 
-bool RenderLayer::updateLayerPosition()
+bool RenderLayer::updateLayerPosition(OptionSet<UpdateLayerPositionsFlag>* flags)
 {
     LayoutPoint localPoint;
     LayoutSize inlineBoundingBoxOffset; // We don't put this into the RenderLayer x/y for inlines, so we need to subtract it out when done.
@@ -1516,6 +1513,10 @@ bool RenderLayer::updateLayerPosition()
                 // Trigger RenderLayerCompositor::requiresCompositingForFrame() which depends on the contentBoxRect size.
                 setNeedsPostLayoutCompositingUpdate();
             }
+
+            if (flags && renderer().hasOverflowClip())
+                flags->add(ContainingClippingLayerChangedSize);
+
             setSize(newSize);
         }
         
index 88b3255..ccfcc4e 100644 (file)
@@ -505,11 +505,12 @@ public:
     bool canRender3DTransforms() const;
 
     enum UpdateLayerPositionsFlag {
-        CheckForRepaint                 = 1 << 0,
-        NeedsFullRepaintInBacking       = 1 << 1,
-        UpdatePagination                = 1 << 2,
-        SeenTransformedLayer            = 1 << 3,
-        Seen3DTransformedLayer          = 1 << 4,
+        CheckForRepaint                     = 1 << 0,
+        NeedsFullRepaintInBacking           = 1 << 1,
+        ContainingClippingLayerChangedSize  = 1 << 2,
+        UpdatePagination                    = 1 << 3,
+        SeenTransformedLayer                = 1 << 4,
+        Seen3DTransformedLayer              = 1 << 5,
     };
     static constexpr OptionSet<UpdateLayerPositionsFlag> updateLayerPositionsDefaultFlags() { return { CheckForRepaint }; }
 
@@ -931,7 +932,7 @@ private:
     void updateScrollbarsAfterLayout();
 
     // Returns true if the position changed.
-    bool updateLayerPosition();
+    bool updateLayerPosition(OptionSet<UpdateLayerPositionsFlag>* = nullptr);
 
     void updateLayerPositions(RenderGeometryMap* = nullptr, OptionSet<UpdateLayerPositionsFlag> = updateLayerPositionsDefaultFlags());
 
index 5465716..938a9bc 100644 (file)
@@ -651,7 +651,7 @@ void RenderLayerBacking::updateAfterWidgetResize()
     }
 }
 
-void RenderLayerBacking::updateAfterLayout(bool needsFullRepaint)
+void RenderLayerBacking::updateAfterLayout(bool needsClippingUpdate, bool needsFullRepaint)
 {
     LOG(Compositing, "RenderLayerBacking %p updateAfterLayout (layer %p)", this, &m_owningLayer);
 
@@ -662,7 +662,8 @@ void RenderLayerBacking::updateAfterLayout(bool needsFullRepaint)
         m_owningLayer.setNeedsCompositingGeometryUpdate();
         // This layer's geometry affects those of its children.
         m_owningLayer.setChildrenNeedCompositingGeometryUpdate();
-    }
+    } else if (needsClippingUpdate)
+        m_owningLayer.setNeedsCompositingGeometryUpdate();
     
     if (needsFullRepaint && canIssueSetNeedsDisplay())
         setContentsNeedDisplay();
index 71ba37f..7af36e0 100644 (file)
@@ -83,7 +83,7 @@ public:
     // Update contents and clipping structure.
     void updateDrawsContent();
     
-    void updateAfterLayout(bool needsFullRepaint);
+    void updateAfterLayout(bool needsClippingUpdate, bool needsFullRepaint);
     
     GraphicsLayer* graphicsLayer() const { return m_graphicsLayer.get(); }