Attachment 370603 Details for Bug 198237 – Patch

[patch] Patch

bug-198237-20190524162611.patch (text/plain), 15.66 KB, created by Simon Fraser (smfr) on 2019-05-24 16:26:11 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-05-24 16:26:11 PDT

Size: 15.66 KB

patch

obsolete

>Subversion Revision: 245751
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 572b81f7e6e7e17c52c71e8ba9c38fe7d763630e..b28f8abbc3c4dc14bf00ce76040c52960e3650c1 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-05-24  Simon Fraser  <simon.fraser@apple.com>
>+
>+        [Async overflow scrolling] Absolute positioned element inside async overflow scroll didn't get composited sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=198237
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The logic in requiresCompositingForIndirectReason() that decides if we need to do
>+        compositing for a layer that needs to move independently of its enclosing scroller
>+        was wrong.
>+
>+        Instead of asking question about the enclosing compositing layer, it needs to ask
>+        whether it has different positioning behavior from the layer that it would
>+        otherwise paint into, which is its paint-order parent.
>+
>+        "paintsIntoProvidedBacking" already does a containing-block check against the
>+        scroller, so we know we don't have to do the check in that case.
>+
>+        Test: scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller.html
>+
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::computeCompositingRequirements):
>+        (WebCore::enclosingCompositedScrollingLayer):
>+        (WebCore::isScrolledByOverflowScrollLayer):
>+        (WebCore::isNonScrolledLayerInsideScrolledCompositedAncestor):
>+        (WebCore::RenderLayerCompositor::requiresCompositingForIndirectReason const):
>+        * rendering/RenderLayerCompositor.h:
>+
> 2019-05-24  Youenn Fablet  <youenn@apple.com>
> 
>         Make sure completion handler is always called in SWServer::startSuspension
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index cae45f83982e7ad6d0d980307fe5d8307adc00a5..9a9d4870be57ea373b1f431f3159add2bde47df4 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -1009,7 +1009,7 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
>     // Now check for reasons to become composited that depend on the state of descendant layers.
>     IndirectCompositingReason indirectCompositingReason;
>     if (!willBeComposited && canBeComposited(layer)
>-        && requiresCompositingForIndirectReason(layer, compositingState.compositingAncestor, currentState.subtreeIsCompositing, anyDescendantHas3DTransform, layerPaintsIntoProvidedBacking, indirectCompositingReason)) {
>+        && requiresCompositingForIndirectReason(layer, currentState.subtreeIsCompositing, anyDescendantHas3DTransform, layerPaintsIntoProvidedBacking, indirectCompositingReason)) {
>         layer.setIndirectCompositingReason(indirectCompositingReason);
>         layerWillCompositePostDescendants();
>     }
>@@ -2916,8 +2916,45 @@ bool RenderLayerCompositor::requiresCompositingForOverflowScrolling(const Render
>     return layer.hasCompositedScrollableOverflow();
> }
> 
>+static RenderLayer* enclosingCompositedScrollingLayer(const RenderLayer& layer, const RenderLayer& intermediateLayer, bool& sawIntermediateLayer)
>+{
>+    const auto* currLayer = layer.parent();
>+    while (currLayer) {
>+        if (currLayer == &intermediateLayer)
>+            sawIntermediateLayer = true;
>+
>+        if (currLayer->hasCompositedScrollableOverflow())
>+            return const_cast<RenderLayer*>(currLayer);
>+
>+        currLayer = currLayer->parent();
>+    }
>+
>+    return nullptr;
>+}
>+
>+static bool isScrolledByOverflowScrollLayer(const RenderLayer& layer, const RenderLayer& overflowScrollLayer)
>+{
>+    bool scrolledByOverflowScroll = false;
>+    traverseAncestorLayers(layer, [&](const RenderLayer& ancestorLayer, bool inContainingBlockChain, bool) {
>+        if (&ancestorLayer == &overflowScrollLayer) {
>+            scrolledByOverflowScroll = inContainingBlockChain;
>+            return AncestorTraversal::Stop;
>+        }
>+        return AncestorTraversal::Continue;
>+    });
>+    return scrolledByOverflowScroll;
>+}
>+
>+static bool isNonScrolledLayerInsideScrolledCompositedAncestor(const RenderLayer& layer, const RenderLayer& compositedAncestor, const RenderLayer& scrollingAncestor)
>+{
>+    bool ancestorMovedByScroller = &compositedAncestor == &scrollingAncestor || isScrolledByOverflowScrollLayer(compositedAncestor, scrollingAncestor);
>+    bool layerMovedByScroller = isScrolledByOverflowScrollLayer(layer, scrollingAncestor);
>+
>+    return ancestorMovedByScroller && !layerMovedByScroller;
>+}
>+
> // FIXME: why doesn't this handle the clipping cases?
>-bool RenderLayerCompositor::requiresCompositingForIndirectReason(const RenderLayer& layer, const RenderLayer* compositingAncestor, bool hasCompositedDescendants, bool has3DTransformedDescendants, bool paintsIntoProvidedBacking, IndirectCompositingReason& reason) const
>+bool RenderLayerCompositor::requiresCompositingForIndirectReason(const RenderLayer& layer, bool hasCompositedDescendants, bool has3DTransformedDescendants, bool paintsIntoProvidedBacking, IndirectCompositingReason& reason) const
> {
>     // When a layer has composited descendants, some effects, like 2d transforms, filters, masks etc must be implemented
>     // via compositing so that they also apply to those composited descendants.
>@@ -2941,8 +2978,12 @@ bool RenderLayerCompositor::requiresCompositingForIndirectReason(const RenderLay
>         }
>     }
> 
>-    if (!paintsIntoProvidedBacking && renderer.isAbsolutelyPositioned() && compositingAncestor && layer.hasCompositedScrollingAncestor()) {
>-        if (layerContainingBlockCrossesCoordinatedScrollingBoundary(layer, *compositingAncestor)) {
>+    // If this layer scrolls independently from the layer that it would paint into, it needs to get composited.
>+    if (!paintsIntoProvidedBacking && layer.hasCompositedScrollingAncestor()) {
>+        auto* paintDestination = layer.paintOrderParent();
>+        bool paintDestinationIsScrolling = false;
>+        auto* scrollingAncestor = enclosingCompositedScrollingLayer(layer, *paintDestination, paintDestinationIsScrolling);
>+        if (isNonScrolledLayerInsideScrolledCompositedAncestor(layer, *paintDestination, *scrollingAncestor)) {
>             reason = IndirectCompositingReason::OverflowScrollPositioning;
>             return true;
>         }
>@@ -3045,43 +3086,6 @@ bool RenderLayerCompositor::useCoordinatedScrollingForLayer(const RenderLayer& l
>     return false;
> }
> 
>-static RenderLayer* enclosingCompositedScrollingLayer(const RenderLayer& layer, const RenderLayer& intermediateLayer, bool& sawIntermediateLayer)
>-{
>-    const auto* currLayer = layer.parent();
>-    while (currLayer) {
>-        if (currLayer == &intermediateLayer)
>-            sawIntermediateLayer = true;
>-
>-        if (currLayer->hasCompositedScrollableOverflow())
>-            return const_cast<RenderLayer*>(currLayer);
>-
>-        currLayer = currLayer->parent();
>-    }
>-
>-    return nullptr;
>-}
>-
>-static bool isScrolledByOverflowScrollLayer(const RenderLayer& layer, const RenderLayer& overflowScrollLayer)
>-{
>-    bool scrolledByOverflowScroll = false;
>-    traverseAncestorLayers(layer, [&](const RenderLayer& ancestorLayer, bool inContainingBlockChain, bool) {
>-        if (&ancestorLayer == &overflowScrollLayer) {
>-            scrolledByOverflowScroll = inContainingBlockChain;
>-            return AncestorTraversal::Stop;
>-        }
>-        return AncestorTraversal::Continue;
>-    });
>-    return scrolledByOverflowScroll;
>-}
>-
>-static bool isNonScrolledLayerInsideScrolledCompositedAncestor(const RenderLayer& layer, const RenderLayer& compositedAncestor, const RenderLayer& scrollingAncestor)
>-{
>-    bool ancestorMovedByScroller = &compositedAncestor == &scrollingAncestor || isScrolledByOverflowScrollLayer(compositedAncestor, scrollingAncestor);
>-    bool layerMovedByScroller = isScrolledByOverflowScrollLayer(layer, scrollingAncestor);
>-
>-    return ancestorMovedByScroller && !layerMovedByScroller;
>-}
>-
> bool RenderLayerCompositor::layerContainingBlockCrossesCoordinatedScrollingBoundary(const RenderLayer& layer, const RenderLayer& compositedAncestor)
> {
>     bool compositedAncestorIsInsideScroller = false;
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.h b/Source/WebCore/rendering/RenderLayerCompositor.h
>index 12c1353c4faeda4f30d836933f1d64b847ebad10..568ab4d3777f1fcd49b6e987599af1edacd190f2 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.h
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.h
>@@ -479,7 +479,7 @@ private:
>     bool requiresCompositingForPosition(RenderLayerModelObject&, const RenderLayer&, RequiresCompositingData&) const;
>     bool requiresCompositingForOverflowScrolling(const RenderLayer&, RequiresCompositingData&) const;
>     bool requiresCompositingForEditableImage(RenderLayerModelObject&) const;
>-    bool requiresCompositingForIndirectReason(const RenderLayer&, const RenderLayer* compositingAncestor, bool hasCompositedDescendants, bool has3DTransformedDescendants, bool paintsIntoProvidedBacking, IndirectCompositingReason&) const;
>+    bool requiresCompositingForIndirectReason(const RenderLayer&, bool hasCompositedDescendants, bool has3DTransformedDescendants, bool paintsIntoProvidedBacking, IndirectCompositingReason&) const;
> 
>     static bool layerContainingBlockCrossesCoordinatedScrollingBoundary(const RenderLayer&, const RenderLayer& compositedAncestor);
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 6078db09aea74e0cfb0f733bed2ac102a1e01540..915838419f4fbdf4b02f680bfbfabac37883ab1b 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-05-24  Simon Fraser  <simon.fraser@apple.com>
>+
>+        [Async overflow scrolling] Absolute positioned element inside async overflow scroll didn't get composited sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=198237
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/ios-wk2/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt: Added.
>+        * scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt: Added.
>+        * scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller.html: Added.
>+
> 2019-05-24  Shawn Roberts  <sroberts@apple.com>
> 
>         scrollingcoordinator/scrolling-tree/fixed-inside-frame.html is no longer flaky
>diff --git a/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt b/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..86cd129dafee0e10a1cfe9193c2d41f09284989f
>--- /dev/null
>+++ b/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt
>@@ -0,0 +1,44 @@
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (parent relative scrollable rect at (0,0) size 800x600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 1)
>+    (vertical scroll elasticity 1)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 2
>+    (Overflow scrolling node
>+      (scrollable area size 220 170)
>+      (contents size 220 1020)
>+      (parent relative scrollable rect at (19,11) size 220x170)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 1)
>+        (vertical scroll elasticity 1)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0)
>+        (has enabled vertical scrollbar 1))
>+    )
>+    (Positioned node
>+      (layout constraints 
>+        (layer-position-at-last-layout (10,10))
>+        (positioning-behavior moves))
>+      (related overflow nodes 1)
>+      (children 1
>+        (Positioned node
>+          (layout constraints 
>+            (layer-position-at-last-layout (71,22))
>+            (positioning-behavior stationary))
>+          (related overflow nodes 1)
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt b/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..227bdb174d06813042748aebd750c1dfde74aaf7
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller-expected.txt
>@@ -0,0 +1,44 @@
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (parent relative scrollable rect at (0,0) size 800x600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 2)
>+    (vertical scroll elasticity 2)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 2
>+    (Overflow scrolling node
>+      (scrollable area size 205 155)
>+      (contents size 205 1020)
>+      (parent relative scrollable rect at (19,11) size 205x155)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 0)
>+        (vertical scroll elasticity 0)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0)
>+        (has enabled vertical scrollbar 1))
>+    )
>+    (Positioned node
>+      (layout constraints 
>+        (layer-position-at-last-layout (10,10))
>+        (positioning-behavior moves))
>+      (related overflow nodes 1)
>+      (children 1
>+        (Positioned node
>+          (layout constraints 
>+            (layer-position-at-last-layout (71,22))
>+            (positioning-behavior stationary))
>+          (related overflow nodes 1)
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller.html b/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..2d97423803a175eb76b1c460ad64ddf2b3269560
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/scrolling-tree/absolute-inside-stacking-in-scroller.html
>@@ -0,0 +1,59 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+	<title>Tests that we make a compositing layer for an abspost element inside something that paints into the scroller</title>
>+    <style>
>+	    .scroller {
>+	        overflow: scroll;
>+	        margin: 10px;
>+	        background-color: silver;
>+	        border: 1px solid black;
>+	        padding: 10px;
>+	        width: 200px;
>+	        height: 150px;
>+	    }
>+
>+	    .stacking {
>+			border: 2px solid gray;
>+			padding: 20px;
>+	        opacity: 0.75;
>+	    }
>+
>+	    .box {
>+	        width: 100px;
>+	        height: 100px;
>+			background-color: blue;
>+	    }
>+	
>+	    .absolute {
>+	        position: absolute;
>+	        left: 100px;
>+	    }
>+
>+	    .scrolling-content {
>+	        height: 1000px;
>+	    }
>+    </style>
>+    <script>
>+	    if (window.testRunner)
>+	        testRunner.dumpAsText();
>+
>+	    window.addEventListener('load', () => {
>+	        if (window.internals)
>+	            document.getElementById('scrollingTree').innerText = window.internals.scrollingStateTreeAsText() + "\n";
>+	    }, false);
>+    </script>
>+</head>
>+<body>
>+    <div class="container">
>+        <div class="scroller">
>+            <div class="scrolling-content">
>+                <div class="stacking">
>+                    <div class="absolute stationary box"></div>
>+                </div>
>+            </div>
>+        </div>
>+    </div>
>+<pre id="scrollingTree"></pre>
>+</body>
>+</html>

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198237: 370602 | 370603