Attachment 360748 Details for Bug 191816 – Patch for landing

[patch] Patch for landing

bug-191816-20190131205607.patch (text/plain), 9.16 KB, created by Frédéric Wang (:fredw) on 2019-01-31 11:56:09 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2019-01-31 11:56:09 PST

Size: 9.16 KB

patch

obsolete

>Subversion Revision: 240799
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 817faad080da778ac15ec7188d1f69e28a0964dc..8332405982824a1fa2b90e45769943696ba45d2d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-01-31  Frederic Wang  <fwang@igalia.com>
>+
>+        [css-scroll-snap] scroll-snap-align not honored on child with non-visible overflow
>+        https://bugs.webkit.org/show_bug.cgi?id=191816
>+
>+        Reviewed by Wenson Hsieh.
>+
>+        This patch fixes a bug that prevents children of a scroll container to create snap positions
>+        when they have non-visible overflow. This happens because for such a child, the function
>+        RenderBox::findEnclosingScrollableContainer() will return the child itself rather than the
>+        scroll container. To address that issue, we introduce a new
>+        RenderObject::enclosingScrollableContainerForSnapping() helper function that ensures that
>+        a real RenderBox ancestor is returned.
>+
>+        Test: css3/scroll-snap/scroll-snap-children-with-overflow.html
>+
>+        * page/scrolling/AxisScrollSnapOffsets.cpp:
>+        (WebCore::updateSnapOffsetsForScrollableArea): Use enclosingScrollableContainerForSnapping()
>+        so that we don't skip children with non-visible overflow.
>+        * rendering/RenderLayerModelObject.cpp:
>+        (WebCore::RenderLayerModelObject::styleDidChange): Ditto. The new function calls
>+        enclosingBox().
>+        * rendering/RenderObject.cpp:
>+        (WebCore::RenderObject::enclosingScrollableContainerForSnapping const): Return
>+        the scrollable container of the enclosing box. If it is actually the render object itself
>+        then start the search from the parent box instead.
>+        * rendering/RenderObject.h: Declare enclosingScrollableContainerForSnapping(). 
>+
> 2019-01-31  Keith Rollin  <krollin@apple.com>
> 
>         GCGamepad is deprecated
>diff --git a/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp b/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp
>index 020753847a0d84171930e2e6f3e5e4d9d48c7af3..8c14ccc9d1d774dca170007ec0f9083aa773a8da 100644
>--- a/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp
>+++ b/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp
>@@ -223,7 +223,7 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, HTMLElem
>     LOG(Scrolling, "Computing scroll snap offsets in snap port: %s", snapPortOrAreaToString(scrollSnapPort).utf8().data());
> #endif
>     for (auto* child : scrollContainer->view().boxesWithScrollSnapPositions()) {
>-        if (child->findEnclosingScrollableContainer() != scrollContainer)
>+        if (child->enclosingScrollableContainerForSnapping() != scrollContainer)
>             continue;
> 
>         // The bounds of the child element's snap area, where the top left of the scrolling container's border box is the origin.
>diff --git a/Source/WebCore/rendering/RenderLayerModelObject.cpp b/Source/WebCore/rendering/RenderLayerModelObject.cpp
>index 06fccde877916f96203765ba8e69cf8dc20b76da..0ce0bb48bde86bfe719653bfe7716d1cf0819d53 100644
>--- a/Source/WebCore/rendering/RenderLayerModelObject.cpp
>+++ b/Source/WebCore/rendering/RenderLayerModelObject.cpp
>@@ -219,7 +219,7 @@ void RenderLayerModelObject::styleDidChange(StyleDifference diff, const RenderSt
>         }
>     }
>     if (oldStyle && oldStyle->scrollSnapArea() != newStyle.scrollSnapArea()) {
>-        const RenderBox* scrollSnapBox = enclosingBox().findEnclosingScrollableContainer();
>+        auto* scrollSnapBox = enclosingScrollableContainerForSnapping();
>         if (scrollSnapBox && scrollSnapBox->layer()) {
>             const RenderStyle& style = scrollSnapBox->style();
>             if (style.scrollSnapType().strictness != ScrollSnapStrictness::None) {
>diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
>index 676017c4c6c77a07b22739f0d567315f276be064..df4f7378da9b98a06f1501b6f2f91787fe6684c5 100644
>--- a/Source/WebCore/rendering/RenderObject.cpp
>+++ b/Source/WebCore/rendering/RenderObject.cpp
>@@ -438,6 +438,20 @@ RenderBoxModelObject& RenderObject::enclosingBoxModelObject() const
>     return *lineageOfType<RenderBoxModelObject>(const_cast<RenderObject&>(*this)).first();
> }
> 
>+const RenderBox* RenderObject::enclosingScrollableContainerForSnapping() const
>+{
>+    auto& renderBox = enclosingBox();
>+    if (auto* scrollableContainer = renderBox.findEnclosingScrollableContainer()) {
>+        // The scrollable container for snapping cannot be the node itself.
>+        if (scrollableContainer == this) {
>+            ASSERT(renderBox.parentBox());
>+            return renderBox.parentBox()->findEnclosingScrollableContainer();
>+        }
>+        return scrollableContainer;
>+    }
>+    return nullptr;
>+}
>+
> RenderBlock* RenderObject::firstLineBlock() const
> {
>     return nullptr;
>diff --git a/Source/WebCore/rendering/RenderObject.h b/Source/WebCore/rendering/RenderObject.h
>index 97cc0faf9c62ce87d0c38e3cc728d5780499a38e..d08c1db0a2e2f6193620f3a752947e4b98152a77 100644
>--- a/Source/WebCore/rendering/RenderObject.h
>+++ b/Source/WebCore/rendering/RenderObject.h
>@@ -165,6 +165,7 @@ public:
>     // Convenience function for getting to the nearest enclosing box of a RenderObject.
>     WEBCORE_EXPORT RenderBox& enclosingBox() const;
>     RenderBoxModelObject& enclosingBoxModelObject() const;
>+    const RenderBox* enclosingScrollableContainerForSnapping() const;
> 
>     // Function to return our enclosing flow thread if we are contained inside one. This
>     // function follows the containing block chain.
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index b0be8865e22218951b2b718d8419c6b0e2351fb0..671e0d9777a581d80ddcd93fc6bf9d2830483a9a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-31  Frederic Wang  <fwang@igalia.com>
>+
>+        [css-scroll-snap] scroll-snap-align not honored on child with non-visible overflow
>+        https://bugs.webkit.org/show_bug.cgi?id=191816
>+
>+        Reviewed by Wenson Hsieh.
>+
>+        Add a test to verify that children with non-visible overflow create snap offsets.
>+
>+        * css3/scroll-snap/scroll-snap-children-with-overflow-expected.txt: Added.
>+        * css3/scroll-snap/scroll-snap-children-with-overflow.html: Added.
>+
> 2019-01-31  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] Use the used margin values in outOfFlowReplacedHorizontalGeometry consistently
>diff --git a/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow-expected.txt b/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..7fcb5c0d63a236f79ff31bcc3ef8930e0cceca54
>--- /dev/null
>+++ b/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow-expected.txt
>@@ -0,0 +1,2 @@
>+Scroll-snap offsets are: vertical = { 0, 720, 1440, 2160, 2880, 3600 }
>+
>diff --git a/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow.html b/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..7af007939c05e5b7faeb881d95c422714aec0ce9
>--- /dev/null
>+++ b/LayoutTests/css3/scroll-snap/scroll-snap-children-with-overflow.html
>@@ -0,0 +1,61 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        #container {
>+            position: absolute;
>+            width: 400px;
>+            height: 400px;
>+            top: 0;
>+            left: 0;
>+            scroll-snap-type: y mandatory;
>+            overflow-x: hidden;
>+            overflow-y: scroll;
>+            -webkit-overflow-scrolling: touch;
>+            border: 1px black dashed;
>+            opacity: 0.75;
>+            background-color: #EEE;
>+        }
>+
>+        .child {
>+            width: 400px;
>+            height: 400px;
>+            scroll-snap-align: end;
>+            position: absolute;
>+            left: 0;
>+        }
>+    </style>
>+    <script>
>+    let write = s => output.innerHTML += s + "<br>";
>+    if (window.testRunner) {
>+        testRunner.dumpAsText();
>+        testRunner.waitUntilDone();
>+    }
>+
>+    function run()
>+    {
>+        if (!window.testRunner || !window.internals) {
>+            write(`To manually test, verify that scrolling in the container snaps such that the bottom tip of each
>+                colored box aligns with the bottom of the scrolling container.`);
>+            return;
>+        }
>+
>+         setTimeout(() => {
>+            write("Scroll-snap offsets are: " + internals.scrollSnapOffsets(container));
>+            testRunner.notifyDone();
>+        }, 0);
>+    }
>+    </script>
>+</head>
>+<body onload=run()>
>+    <div id="container">
>+        <div class="child" style="background-color: red;     top: 0px; overflow: visible;"></div>
>+        <div class="child" style="background-color: green;   top: 720px;  overflow: hidden;"></div>
>+        <div class="child" style="background-color: blue;    top: 1440px; overflow: scroll;"></div>
>+        <div class="child" style="background-color: aqua;    top: 2160px; overflow: auto;"></div>
>+        <div class="child" style="background-color: yellow;  top: 2880px; overflow: overlay;"></div>
>+        <div class="child" style="background-color: fuchsia; top: 3600px;"></div>
>+    </div>
>+    <div id="output"></div>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 191816: 359159 | 360715 | 360748 | 361053