RequestedScrollPosition shouldn't be applied after node reattach
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jun 2019 17:21:11 +0000 (17:21 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jun 2019 17:21:11 +0000 (17:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198994
<rdar://problem/51439685>

Reviewed by Simon Fraser.

Source/WebCore:

Test: scrollingcoordinator/ios/scroll-position-after-reattach.html

If a scrolling node gets reattached, its scroll position resets to (0,0) or whatever the previous
requestedScrollPosition was, and the current position is lost.

* page/scrolling/ScrollingStateFixedNode.cpp:
(WebCore::ScrollingStateFixedNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStateFixedNode::setAllPropertiesChanged): Deleted.

Rename to better reflect what this is for.

* page/scrolling/ScrollingStateFixedNode.h:
* page/scrolling/ScrollingStateFrameHostingNode.cpp:
(WebCore::ScrollingStateFrameHostingNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStateFrameHostingNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStateFrameHostingNode.h:
* page/scrolling/ScrollingStateFrameScrollingNode.cpp:
(WebCore::ScrollingStateFrameScrollingNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStateFrameScrollingNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStateFrameScrollingNode.h:
* page/scrolling/ScrollingStateNode.cpp:
(WebCore::ScrollingStateNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStateNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStateNode.h:
* page/scrolling/ScrollingStatePositionedNode.cpp:
(WebCore::ScrollingStatePositionedNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStatePositionedNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStatePositionedNode.h:
* page/scrolling/ScrollingStateScrollingNode.cpp:
(WebCore::ScrollingStateScrollingNode::setPropertyChangedBitsAfterReattach):

Don't set RequestedScrollPosition. It is a special property that is applied only once on request
and shouldn't get reapplied. Nodes should keep their existing scroll position on reattach.

(WebCore::ScrollingStateScrollingNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStateScrollingNode.h:
* page/scrolling/ScrollingStateStickyNode.cpp:
(WebCore::ScrollingStateStickyNode::setPropertyChangedBitsAfterReattach):
(WebCore::ScrollingStateStickyNode::setAllPropertiesChanged): Deleted.
* page/scrolling/ScrollingStateStickyNode.h:
* page/scrolling/ScrollingStateTree.cpp:
(WebCore::ScrollingStateTree::nodeWasReattachedRecursive):

LayoutTests:

* scrollingcoordinator/ios/scroll-position-after-reattach-expected.html: Added.
* scrollingcoordinator/ios/scroll-position-after-reattach.html: Added.

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

19 files changed:
LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach-expected.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp
Source/WebCore/page/scrolling/ScrollingStateFixedNode.h
Source/WebCore/page/scrolling/ScrollingStateFrameHostingNode.cpp
Source/WebCore/page/scrolling/ScrollingStateFrameHostingNode.h
Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h
Source/WebCore/page/scrolling/ScrollingStateNode.cpp
Source/WebCore/page/scrolling/ScrollingStateNode.h
Source/WebCore/page/scrolling/ScrollingStatePositionedNode.cpp
Source/WebCore/page/scrolling/ScrollingStatePositionedNode.h
Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h
Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp
Source/WebCore/page/scrolling/ScrollingStateStickyNode.h
Source/WebCore/page/scrolling/ScrollingStateTree.cpp

index f24d2e4..9121661 100644 (file)
@@ -1,3 +1,14 @@
+2019-06-19  Antti Koivisto  <antti@apple.com>
+
+        RequestedScrollPosition shouldn't be applied after node reattach
+        https://bugs.webkit.org/show_bug.cgi?id=198994
+        <rdar://problem/51439685>
+
+        Reviewed by Simon Fraser.
+
+        * scrollingcoordinator/ios/scroll-position-after-reattach-expected.html: Added.
+        * scrollingcoordinator/ios/scroll-position-after-reattach.html: Added.
+
 2019-06-19  Truitt Savell  <tsavell@apple.com>
 
         REGRESSION: ( r246394 ) webgpu/whlsl-buffer-fragment.html and webgpu/whlsl-buffer-vertex.html are failing
diff --git a/LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach-expected.html b/LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach-expected.html
new file mode 100644 (file)
index 0000000..3a434ab
--- /dev/null
@@ -0,0 +1,47 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true internal:AsyncFrameScrollingEnabled=true ] -->
+<style>
+#outer {
+    overflow: scroll;
+    width: 400px;
+    height: 400px;
+    border: 2px solid yellow;
+}
+#inner {
+    overflow: auto;
+    width: 300px;
+    height: 300px;
+    border: 2px solid blue;
+}
+#innercontent {
+    width: 1000px;
+    height: 1000px;
+    border: 10px solid green;
+}
+#outercontent {
+    width: 1000px;
+    height: 1000px;
+    border: 10px solid red;
+}
+</style>
+<script src="../../resources/ui-helper.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+async function doTest() {
+    if (!window.testRunner)
+        return;
+    inner.scrollTo(0, 200);
+    testRunner.notifyDone();
+}
+
+window.addEventListener('load', doTest, false);
+</script>\ 1
+
+<div id=outer>
+    <div id=inner>
+        <div id=innercontent>This should be scrollable</div>
+    </div>
+    <div id=outercontent class=content></div>
+</div>
+
diff --git a/LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach.html b/LayoutTests/scrollingcoordinator/ios/scroll-position-after-reattach.html
new file mode 100644 (file)
index 0000000..0b5eccd
--- /dev/null
@@ -0,0 +1,56 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true internal:AsyncFrameScrollingEnabled=true ] -->
+<style>
+#outer {
+    overflow: scroll;
+    width: 400px;
+    height: 400px;
+    border: 2px solid yellow;
+}
+#inner {
+    overflow: auto;
+    width: 300px;
+    height: 300px;
+    border: 2px solid blue;
+}
+#innercontent {
+    width: 1000px;
+    height: 1000px;
+    border: 10px solid green;
+}
+#outercontent {
+    width: 1000px;
+    height: 1000px;
+    border: 10px solid red;
+}
+</style>
+<script src="../../resources/ui-helper.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+function scroll() {
+    let removed = outercontent;
+    outercontent.remove();
+    document.offsetLeft;
+    setTimeout(() => outer.appendChild(removed));
+}
+
+async function doTest() {
+    if (!window.testRunner)
+        return;
+    inner.addEventListener("scroll", scroll);
+
+    await UIHelper.immediateScrollElementAtContentPointToOffset(50, 50, 0, 200);
+    testRunner.notifyDone();
+}
+
+window.addEventListener('load', doTest, false);
+</script>\ 1
+
+<div id=outer>
+    <div id=inner>
+        <div id=innercontent>This should be scrollable</div>
+    </div>
+    <div id=outercontent class=content></div>
+</div>
+
index c571b58..6b76399 100644 (file)
@@ -1,3 +1,54 @@
+2019-06-19  Antti Koivisto  <antti@apple.com>
+
+        RequestedScrollPosition shouldn't be applied after node reattach
+        https://bugs.webkit.org/show_bug.cgi?id=198994
+        <rdar://problem/51439685>
+
+        Reviewed by Simon Fraser.
+
+        Test: scrollingcoordinator/ios/scroll-position-after-reattach.html
+
+        If a scrolling node gets reattached, its scroll position resets to (0,0) or whatever the previous
+        requestedScrollPosition was, and the current position is lost.
+
+        * page/scrolling/ScrollingStateFixedNode.cpp:
+        (WebCore::ScrollingStateFixedNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStateFixedNode::setAllPropertiesChanged): Deleted.
+
+        Rename to better reflect what this is for.
+
+        * page/scrolling/ScrollingStateFixedNode.h:
+        * page/scrolling/ScrollingStateFrameHostingNode.cpp:
+        (WebCore::ScrollingStateFrameHostingNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStateFrameHostingNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStateFrameHostingNode.h:
+        * page/scrolling/ScrollingStateFrameScrollingNode.cpp:
+        (WebCore::ScrollingStateFrameScrollingNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStateFrameScrollingNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStateFrameScrollingNode.h:
+        * page/scrolling/ScrollingStateNode.cpp:
+        (WebCore::ScrollingStateNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStateNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStateNode.h:
+        * page/scrolling/ScrollingStatePositionedNode.cpp:
+        (WebCore::ScrollingStatePositionedNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStatePositionedNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStatePositionedNode.h:
+        * page/scrolling/ScrollingStateScrollingNode.cpp:
+        (WebCore::ScrollingStateScrollingNode::setPropertyChangedBitsAfterReattach):
+
+        Don't set RequestedScrollPosition. It is a special property that is applied only once on request
+        and shouldn't get reapplied. Nodes should keep their existing scroll position on reattach.
+
+        (WebCore::ScrollingStateScrollingNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStateScrollingNode.h:
+        * page/scrolling/ScrollingStateStickyNode.cpp:
+        (WebCore::ScrollingStateStickyNode::setPropertyChangedBitsAfterReattach):
+        (WebCore::ScrollingStateStickyNode::setAllPropertiesChanged): Deleted.
+        * page/scrolling/ScrollingStateStickyNode.h:
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::nodeWasReattachedRecursive):
+
 2019-06-18  Saam Barati  <sbarati@apple.com>
 
         [WHLSL] Support matrices
index ca6439d..908dd0c 100644 (file)
@@ -58,10 +58,10 @@ Ref<ScrollingStateNode> ScrollingStateFixedNode::clone(ScrollingStateTree& adopt
     return adoptRef(*new ScrollingStateFixedNode(*this, adoptiveTree));
 }
 
-void ScrollingStateFixedNode::setAllPropertiesChanged()
+void ScrollingStateFixedNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(ViewportConstraints);
-    ScrollingStateNode::setAllPropertiesChanged();
+    ScrollingStateNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStateFixedNode::updateConstraints(const FixedPositionViewportConstraints& constraints)
index ac3c2bf..6db722b 100644 (file)
@@ -55,7 +55,7 @@ private:
     ScrollingStateFixedNode(ScrollingStateTree&, ScrollingNodeID);
     ScrollingStateFixedNode(const ScrollingStateFixedNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     void reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction) override;
 
index 9503032..847419c 100644 (file)
@@ -57,11 +57,11 @@ Ref<ScrollingStateNode> ScrollingStateFrameHostingNode::clone(ScrollingStateTree
     return adoptRef(*new ScrollingStateFrameHostingNode(*this, adoptiveTree));
 }
 
-void ScrollingStateFrameHostingNode::setAllPropertiesChanged()
+void ScrollingStateFrameHostingNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(ParentRelativeScrollableRect);
 
-    ScrollingStateNode::setAllPropertiesChanged();
+    ScrollingStateNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStateFrameHostingNode::setParentRelativeScrollableRect(const LayoutRect& parentRelativeScrollableRect)
index 0568cd2..416cd43 100644 (file)
@@ -53,7 +53,7 @@ private:
     ScrollingStateFrameHostingNode(ScrollingStateTree&, ScrollingNodeID);
     ScrollingStateFrameHostingNode(const ScrollingStateFrameHostingNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     LayoutRect m_parentRelativeScrollableRect;
 };
index 46ef674..781cdf9 100644 (file)
@@ -85,7 +85,7 @@ Ref<ScrollingStateNode> ScrollingStateFrameScrollingNode::clone(ScrollingStateTr
     return adoptRef(*new ScrollingStateFrameScrollingNode(*this, adoptiveTree));
 }
 
-void ScrollingStateFrameScrollingNode::setAllPropertiesChanged()
+void ScrollingStateFrameScrollingNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(FrameScaleFactor);
     setPropertyChangedBit(EventTrackingRegion);
@@ -107,7 +107,7 @@ void ScrollingStateFrameScrollingNode::setAllPropertiesChanged()
     setPropertyChangedBit(MinLayoutViewportOrigin);
     setPropertyChangedBit(MaxLayoutViewportOrigin);
 
-    ScrollingStateScrollingNode::setAllPropertiesChanged();
+    ScrollingStateScrollingNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStateFrameScrollingNode::setFrameScaleFactor(float scaleFactor)
index 13c650f..94751d1 100644 (file)
@@ -135,7 +135,7 @@ private:
     ScrollingStateFrameScrollingNode(ScrollingStateTree&, ScrollingNodeType, ScrollingNodeID);
     ScrollingStateFrameScrollingNode(const ScrollingStateFrameScrollingNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     LayerRepresentation m_rootContentsLayer;
     LayerRepresentation m_counterScrollingLayer;
index 91eea44..882d733 100644 (file)
@@ -67,7 +67,7 @@ void ScrollingStateNode::setPropertyChanged(unsigned propertyBit)
     m_scrollingStateTree.setHasChangedProperties();
 }
 
-void ScrollingStateNode::setAllPropertiesChanged()
+void ScrollingStateNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(Layer);
     setPropertyChangedBit(ChildNodes);
index 6cb6e81..0f87ea1 100644 (file)
@@ -212,7 +212,7 @@ public:
     bool hasChangedProperty(unsigned propertyBit) const { return m_changedProperties & (static_cast<ChangedProperties>(1) << propertyBit); }
     void resetChangedProperties() { m_changedProperties = 0; }
     void setPropertyChanged(unsigned propertyBit);
-    virtual void setAllPropertiesChanged();
+    virtual void setPropertyChangedBitsAfterReattach();
 
     ChangedProperties changedProperties() const { return m_changedProperties; }
     void setChangedProperties(ChangedProperties changedProperties) { m_changedProperties = changedProperties; }
index 704d505..6777be2 100644 (file)
@@ -59,11 +59,11 @@ Ref<ScrollingStateNode> ScrollingStatePositionedNode::clone(ScrollingStateTree&
     return adoptRef(*new ScrollingStatePositionedNode(*this, adoptiveTree));
 }
 
-void ScrollingStatePositionedNode::setAllPropertiesChanged()
+void ScrollingStatePositionedNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(RelatedOverflowScrollingNodes);
     setPropertyChangedBit(LayoutConstraintData);
-    ScrollingStateNode::setAllPropertiesChanged();
+    ScrollingStateNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStatePositionedNode::setRelatedOverflowScrollingNodes(Vector<ScrollingNodeID>&& nodes)
index cb1beac..566828b 100644 (file)
@@ -61,7 +61,7 @@ private:
     ScrollingStatePositionedNode(ScrollingStateTree&, ScrollingNodeID);
     ScrollingStatePositionedNode(const ScrollingStatePositionedNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
 
index 6acc985..2d99ac5 100644 (file)
@@ -73,7 +73,7 @@ ScrollingStateScrollingNode::ScrollingStateScrollingNode(const ScrollingStateScr
 
 ScrollingStateScrollingNode::~ScrollingStateScrollingNode() = default;
 
-void ScrollingStateScrollingNode::setAllPropertiesChanged()
+void ScrollingStateScrollingNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(ScrollableAreaSize);
     setPropertyChangedBit(TotalContentsSize);
@@ -82,7 +82,6 @@ void ScrollingStateScrollingNode::setAllPropertiesChanged()
     setPropertyChangedBit(ScrollPosition);
     setPropertyChangedBit(ScrollOrigin);
     setPropertyChangedBit(ScrollableAreaParams);
-    setPropertyChangedBit(RequestedScrollPosition);
 #if ENABLE(CSS_SCROLL_SNAP)
     setPropertyChangedBit(HorizontalSnapOffsets);
     setPropertyChangedBit(VerticalSnapOffsets);
@@ -98,7 +97,7 @@ void ScrollingStateScrollingNode::setAllPropertiesChanged()
     setPropertyChangedBit(VerticalScrollbarLayer);
     setPropertyChangedBit(PainterForScrollbar);
 
-    ScrollingStateNode::setAllPropertiesChanged();
+    ScrollingStateNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStateScrollingNode::setScrollableAreaSize(const FloatSize& size)
index b4581cf..3c2de1d 100644 (file)
@@ -139,7 +139,7 @@ protected:
     ScrollingStateScrollingNode(ScrollingStateTree&, ScrollingNodeType, ScrollingNodeID);
     ScrollingStateScrollingNode(const ScrollingStateScrollingNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
 
index 9466659..0a3d5f0 100644 (file)
@@ -58,10 +58,10 @@ Ref<ScrollingStateNode> ScrollingStateStickyNode::clone(ScrollingStateTree& adop
     return adoptRef(*new ScrollingStateStickyNode(*this, adoptiveTree));
 }
 
-void ScrollingStateStickyNode::setAllPropertiesChanged()
+void ScrollingStateStickyNode::setPropertyChangedBitsAfterReattach()
 {
     setPropertyChangedBit(ViewportConstraints);
-    ScrollingStateNode::setAllPropertiesChanged();
+    ScrollingStateNode::setPropertyChangedBitsAfterReattach();
 }
 
 void ScrollingStateStickyNode::updateConstraints(const StickyPositionViewportConstraints& constraints)
index 8f075f6..c09f581 100644 (file)
@@ -55,7 +55,7 @@ private:
     ScrollingStateStickyNode(ScrollingStateTree&, ScrollingNodeID);
     ScrollingStateStickyNode(const ScrollingStateStickyNode&, ScrollingStateTree&);
 
-    void setAllPropertiesChanged() override;
+    void setPropertyChangedBitsAfterReattach() override;
 
     void reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction) override;
 
index 138e6da..f26cded 100644 (file)
@@ -274,7 +274,7 @@ void ScrollingStateTree::clear()
 void ScrollingStateTree::nodeWasReattachedRecursive(ScrollingStateNode& node)
 {
     // When a node is re-attached, the ScrollingTree is recreating the ScrollingNode from scratch, so we need to set all the dirty bits.
-    node.setAllPropertiesChanged();
+    node.setPropertyChangedBitsAfterReattach();
 
     if (auto* children = node.children()) {
         for (auto& child : *children)