Attachment 359774 Details for Bug 193682 – Patch

[patch] Patch

bug-193682-20190122143023.patch (text/plain), 16.20 KB, created by Simon Fraser (smfr) on 2019-01-22 14:30:24 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-22 14:30:24 PST

Size: 16.20 KB

patch

obsolete

>Subversion Revision: 240297
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d9ef453f48665c98734ef9afb53cddd3db4fbab4..2b4989cddf3e0d49644767e9891e1116a0a8cfda 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-01-22  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Adding a child to a ScrollingStateNode needs to trigger a tree state commit
>+        https://bugs.webkit.org/show_bug.cgi?id=193682
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Scrolling tree mutations that re-arrange nodes (e.g. node reordering when z-index changes)
>+        need to trigger scrolling tree updates, and currently do not.
>+
>+        Fix by adding a "ChildNodes" dirty bit to ScrollingStateNode. There isn't any code that consults
>+        this flag when committing the scrolling tree because we always eagerly traverse children, but
>+        we could use it to optimize later. The important part is that we use it to trigger a tree update.
>+        
>+        Can't test via z-reordering until webkit.org/b/192529 is fixed.
>+
>+        Tests: scrollingcoordinator/gain-scrolling-node-parent.html
>+               scrollingcoordinator/lose-scrolling-node-parent.html
>+
>+        * page/scrolling/ScrollingStateNode.cpp:
>+        (WebCore::ScrollingStateNode::appendChild):
>+        (WebCore::ScrollingStateNode::insertChild):
>+        (WebCore::ScrollingStateNode::removeChildAtIndex):
>+        * page/scrolling/ScrollingStateNode.h:
>+        * page/scrolling/ScrollingStateTree.cpp:
>+        (WebCore::ScrollingStateTree::attachNode):
>+
> 2019-01-22  Alex Christensen  <achristensen@webkit.org>
> 
>         Fix more builds.
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>index 03b7ab61c5f7407cf0959cd2c12db20137159691..e10b21a333b8dd50649def53a26b5ba7445470eb 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>@@ -95,6 +95,7 @@ void ScrollingStateNode::appendChild(Ref<ScrollingStateNode>&& childNode)
>     if (!m_children)
>         m_children = std::make_unique<Vector<RefPtr<ScrollingStateNode>>>();
>     m_children->append(WTFMove(childNode));
>+    setPropertyChanged(ChildNodes);
> }
> 
> void ScrollingStateNode::insertChild(Ref<ScrollingStateNode>&& childNode, size_t index)
>@@ -107,6 +108,16 @@ void ScrollingStateNode::insertChild(Ref<ScrollingStateNode>&& childNode, size_t
>     }
> 
>     m_children->insert(index, WTFMove(childNode));
>+    setPropertyChanged(ChildNodes);
>+}
>+
>+void ScrollingStateNode::removeChildAtIndex(size_t index)
>+{
>+    ASSERT(m_children && index < m_children->size());
>+    if (m_children && index < m_children->size()) {
>+        m_children->remove(index);
>+        setPropertyChanged(ChildNodes);
>+    }
> }
> 
> size_t ScrollingStateNode::indexOfChild(ScrollingStateNode& childNode) const
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.h b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>index 27c2154931ba3aa9a23d113297898ba51b9cbced..a5a265ca45147019237f8e56fee0872efe88c75e 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>@@ -208,6 +208,7 @@ public:
> 
>     enum {
>         ScrollLayer = 0,
>+        ChildNodes,
>         NumStateNodeBits // This must remain at the last position.
>     };
>     typedef uint64_t ChangedProperties;
>@@ -238,6 +239,8 @@ public:
>     void appendChild(Ref<ScrollingStateNode>&&);
>     void insertChild(Ref<ScrollingStateNode>&&, size_t index);
> 
>+    void removeChildAtIndex(size_t index);
>+
>     size_t indexOfChild(ScrollingStateNode&) const;
> 
>     String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const;
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>index 2db453c14e44be9e359ee91536951086d6e22d37..b2bd266ebba9acf853bae1fdd54170edbeb696eb 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>@@ -106,7 +106,7 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
> 
>             ASSERT(currentIndex != notFound);
>             Ref<ScrollingStateNode> protectedNode(*node);
>-            parent->children()->remove(currentIndex);
>+            parent->removeChildAtIndex(currentIndex);
> 
>             if (childIndex == notFound)
>                 parent->appendChild(WTFMove(protectedNode));
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index c90ff2522694442acb58d914bdfd086cff8dc8ec..e0d067b975d29e2f1ec944658f6482c4bd408b7a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-22  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Adding a child to a ScrollingStateNode needs to trigger a tree state commit
>+        https://bugs.webkit.org/show_bug.cgi?id=193682
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/ios/scrollingcoordinator/gain-scrolling-node-parent-expected.txt: Added.
>+        * platform/ios/scrollingcoordinator/lose-scrolling-node-parent-expected.txt: Added.
>+        * scrollingcoordinator/gain-scrolling-node-parent-expected.txt: Added.
>+        * scrollingcoordinator/gain-scrolling-node-parent.html: Added.
>+        * scrollingcoordinator/lose-scrolling-node-parent-expected.txt: Added.
>+        * scrollingcoordinator/lose-scrolling-node-parent.html: Added.
>+
> 2019-01-22  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Unreviewed, skip all resource load statistics tests on GTK
>diff --git a/LayoutTests/platform/ios/scrollingcoordinator/gain-scrolling-node-parent-expected.txt b/LayoutTests/platform/ios/scrollingcoordinator/gain-scrolling-node-parent-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..0f5dc63b447deb66185498824c097d31b73721e6
>--- /dev/null
>+++ b/LayoutTests/platform/ios/scrollingcoordinator/gain-scrolling-node-parent-expected.txt
>@@ -0,0 +1,42 @@
>+Scrolling content
>+Middle scrolling content
>+Inner scrolling content
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 1)
>+    (vertical scroll elasticity 1)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (visual viewport enabled 1)
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 1
>+    (Overflow scrolling node
>+      (scrollable area size 420 320)
>+      (contents size 443 1041)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 1)
>+        (vertical scroll elasticity 1)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0))
>+      (children 1
>+        (Overflow scrolling node
>+          (scrollable area size 420 320)
>+          (contents size 420 1020)
>+          (scrollable area parameters 
>+            (horizontal scroll elasticity 1)
>+            (vertical scroll elasticity 1)
>+            (horizontal scrollbar mode 0)
>+            (vertical scrollbar mode 0))
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/platform/ios/scrollingcoordinator/lose-scrolling-node-parent-expected.txt b/LayoutTests/platform/ios/scrollingcoordinator/lose-scrolling-node-parent-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..0f5dc63b447deb66185498824c097d31b73721e6
>--- /dev/null
>+++ b/LayoutTests/platform/ios/scrollingcoordinator/lose-scrolling-node-parent-expected.txt
>@@ -0,0 +1,42 @@
>+Scrolling content
>+Middle scrolling content
>+Inner scrolling content
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 1)
>+    (vertical scroll elasticity 1)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (visual viewport enabled 1)
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 1
>+    (Overflow scrolling node
>+      (scrollable area size 420 320)
>+      (contents size 443 1041)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 1)
>+        (vertical scroll elasticity 1)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0))
>+      (children 1
>+        (Overflow scrolling node
>+          (scrollable area size 420 320)
>+          (contents size 420 1020)
>+          (scrollable area parameters 
>+            (horizontal scroll elasticity 1)
>+            (vertical scroll elasticity 1)
>+            (horizontal scrollbar mode 0)
>+            (vertical scrollbar mode 0))
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent-expected.txt b/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..1f123a182a749410966ae7e08e3173e2c4a2f88d
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent-expected.txt
>@@ -0,0 +1,42 @@
>+Scrolling content
>+Middle scrolling content
>+Inner scrolling content
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 2)
>+    (vertical scroll elasticity 2)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (visual viewport enabled 1)
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 1
>+    (Overflow scrolling node
>+      (scrollable area size 405 305)
>+      (contents size 443 1039)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 1)
>+        (vertical scroll elasticity 1)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0))
>+      (children 1
>+        (Overflow scrolling node
>+          (scrollable area size 405 305)
>+          (contents size 405 1020)
>+          (scrollable area parameters 
>+            (horizontal scroll elasticity 1)
>+            (vertical scroll elasticity 1)
>+            (horizontal scrollbar mode 0)
>+            (vertical scrollbar mode 0))
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent.html b/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..16bed94b768beab3d6786f17dd40525d5f8966dd
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/gain-scrolling-node-parent.html
>@@ -0,0 +1,64 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <title>Check that scrolling nodes get reparented when an ancestor is removed</title>
>+    <script>
>+        if (window.testRunner) {
>+            testRunner.dumpAsText();
>+            testRunner.waitUntilDone();
>+        }
>+
>+        if (window.internals)
>+            window.internals.settings.setAsyncOverflowScrollingEnabled(true);
>+
>+        function doTest() {
>+            requestAnimationFrame(() => {
>+                document.getElementById('target').classList.add('changed');
>+                if (window.internals)
>+                    document.getElementById('scrollingTree').innerText = window.internals.scrollingStateTreeAsText() + "\n";
>+
>+                if (window.testRunner)
>+                    testRunner.notifyDone();
>+            });
>+        }
>+
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+    <style>
>+        .scroller {
>+            background-color: silver;
>+            border: 1px solid black;
>+            padding: 10px;
>+            width: 400px;
>+            height: 300px;
>+            overflow: scroll;
>+        }
>+        
>+        #target.changed {
>+            overflow: visible;
>+        }
>+        
>+        .scrolling-content {
>+            height: 1000px;
>+        }
>+    </style>
>+</head>
>+<body>
>+    <div class="scroller">
>+        <div class="scrolling-content">
>+            Scrolling content
>+            <div class="intermediate scroller" id="target">
>+                <div class="scrolling-content">
>+                    Middle scrolling content
>+                    <div class="inner scroller">
>+                        <div class="scrolling-content">
>+                        Inner scrolling content
>+                        </div>
>+                    </div>
>+                </div>
>+            </div>
>+        </div>
>+    </div>
>+    <pre id="scrollingTree"></pre>
>+</body>
>+</html>
>diff --git a/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent-expected.txt b/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..1f123a182a749410966ae7e08e3173e2c4a2f88d
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent-expected.txt
>@@ -0,0 +1,42 @@
>+Scrolling content
>+Middle scrolling content
>+Inner scrolling content
>+
>+(Frame scrolling node
>+  (scrollable area size 800 600)
>+  (contents size 800 600)
>+  (scrollable area parameters 
>+    (horizontal scroll elasticity 2)
>+    (vertical scroll elasticity 2)
>+    (horizontal scrollbar mode 0)
>+    (vertical scrollbar mode 0))
>+  (visual viewport enabled 1)
>+  (layout viewport at (0,0) size 800x600)
>+  (min layout viewport origin (0,0))
>+  (max layout viewport origin (0,0))
>+  (behavior for fixed 0)
>+  (children 1
>+    (Overflow scrolling node
>+      (scrollable area size 405 305)
>+      (contents size 443 1039)
>+      (scrollable area parameters 
>+        (horizontal scroll elasticity 1)
>+        (vertical scroll elasticity 1)
>+        (horizontal scrollbar mode 0)
>+        (vertical scrollbar mode 0))
>+      (children 1
>+        (Overflow scrolling node
>+          (scrollable area size 405 305)
>+          (contents size 405 1020)
>+          (scrollable area parameters 
>+            (horizontal scroll elasticity 1)
>+            (vertical scroll elasticity 1)
>+            (horizontal scrollbar mode 0)
>+            (vertical scrollbar mode 0))
>+        )
>+      )
>+    )
>+  )
>+)
>+
>+
>diff --git a/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent.html b/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..16bed94b768beab3d6786f17dd40525d5f8966dd
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/lose-scrolling-node-parent.html
>@@ -0,0 +1,64 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <title>Check that scrolling nodes get reparented when an ancestor is removed</title>
>+    <script>
>+        if (window.testRunner) {
>+            testRunner.dumpAsText();
>+            testRunner.waitUntilDone();
>+        }
>+
>+        if (window.internals)
>+            window.internals.settings.setAsyncOverflowScrollingEnabled(true);
>+
>+        function doTest() {
>+            requestAnimationFrame(() => {
>+                document.getElementById('target').classList.add('changed');
>+                if (window.internals)
>+                    document.getElementById('scrollingTree').innerText = window.internals.scrollingStateTreeAsText() + "\n";
>+
>+                if (window.testRunner)
>+                    testRunner.notifyDone();
>+            });
>+        }
>+
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+    <style>
>+        .scroller {
>+            background-color: silver;
>+            border: 1px solid black;
>+            padding: 10px;
>+            width: 400px;
>+            height: 300px;
>+            overflow: scroll;
>+        }
>+        
>+        #target.changed {
>+            overflow: visible;
>+        }
>+        
>+        .scrolling-content {
>+            height: 1000px;
>+        }
>+    </style>
>+</head>
>+<body>
>+    <div class="scroller">
>+        <div class="scrolling-content">
>+            Scrolling content
>+            <div class="intermediate scroller" id="target">
>+                <div class="scrolling-content">
>+                    Middle scrolling content
>+                    <div class="inner scroller">
>+                        <div class="scrolling-content">
>+                        Inner scrolling content
>+                        </div>
>+                    </div>
>+                </div>
>+            </div>
>+        </div>
>+    </div>
>+    <pre id="scrollingTree"></pre>
>+</body>
>+</html>

Flags:

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

Actions: View | Formatted Diff | Diff

Attachments on bug 193682: 359771 | 359774 | 359784 | 359801 | 359824