Changeset 284084 in webkit

Timestamp:

Oct 13, 2021 12:59:14 AM (9 months ago)

Author:

Martin Robinson

Message:

Sticky element inside another sticky element does not redraw properly on scroll
​https://bugs.webkit.org/show_bug.cgi?id=199915
<rdar://problem/53375284>

Reviewed by Simon Fraser.

When calculating layer position for sticky nodes in the scrolling tree, take into
account situations where the sticky parents of sticky nodes don't stick (due to
sizing). This was handled properly in layout, but not in the scrolling tree, which
caused these kinds of nodes to jump around.

Source/WebCore:

Test: scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent.html

• page/scrolling/ScrollingStateStickyNode.cpp:

(WebCore::ScrollingStateStickyNode::computeLayerPosition const):
(WebCore::ScrollingStateStickyNode::scrollDeltaSinceLastCommit const):

• page/scrolling/ScrollingStateStickyNode.h:
• page/scrolling/cocoa/ScrollingTreeStickyNode.mm:

(WebCore::ScrollingTreeStickyNode::computeLayerPosition const):

• page/scrolling/nicosia/ScrollingTreeStickyNode.cpp:

(WebCore::ScrollingTreeStickyNode::computeLayerPosition const):
(WebCore::ScrollingTreeStickyNode::scrollDeltaSinceLastCommit const):

• page/scrolling/nicosia/ScrollingTreeStickyNode.h:

LayoutTests:

• scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent-expected.html: Added.
• scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent-expected.html (added)
• LayoutTests/scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp (modified) (4 diffs)
• Source/WebCore/page/scrolling/ScrollingStateStickyNode.h (modified) (1 diff)
• Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm (modified) (3 diffs)
• Source/WebCore/page/scrolling/nicosia/ScrollingTreeStickyNode.cpp (modified) (4 diffs)
• Source/WebCore/page/scrolling/nicosia/ScrollingTreeStickyNode.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-10-13  Martin Robinson  <mrobinson@webkit.org>
+
+        Sticky element inside another sticky element does not redraw properly on scroll
+        https://bugs.webkit.org/show_bug.cgi?id=199915
+        <rdar://problem/53375284>
+
+        Reviewed by Simon Fraser.
+
+        When calculating layer position for sticky nodes in the scrolling tree, take into
+        account situations where the sticky parents of sticky nodes don't stick (due to
+        sizing). This was handled properly in layout, but not in the scrolling tree, which
+        caused these kinds of nodes to jump around.
+
+        * scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent-expected.html: Added.
+        * scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent.html: Added.
+
 2021-10-12  Arcady Goldmints-Orlov  <agoldmints@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-10-13  Martin Robinson  <mrobinson@webkit.org>
+
+        Sticky element inside another sticky element does not redraw properly on scroll
+        https://bugs.webkit.org/show_bug.cgi?id=199915
+        <rdar://problem/53375284>
+
+        Reviewed by Simon Fraser.
+
+        When calculating layer position for sticky nodes in the scrolling tree, take into
+        account situations where the sticky parents of sticky nodes don't stick (due to
+        sizing). This was handled properly in layout, but not in the scrolling tree, which
+        caused these kinds of nodes to jump around.
+
+        Test: scrollingcoordinator/mac/nested-sticky-with-nonsticking-sticky-parent.html
+
+        * page/scrolling/ScrollingStateStickyNode.cpp:
+        (WebCore::ScrollingStateStickyNode::computeLayerPosition const):
+        (WebCore::ScrollingStateStickyNode::scrollDeltaSinceLastCommit const):
+        * page/scrolling/ScrollingStateStickyNode.h:
+        * page/scrolling/cocoa/ScrollingTreeStickyNode.mm:
+        (WebCore::ScrollingTreeStickyNode::computeLayerPosition const):
+        * page/scrolling/nicosia/ScrollingTreeStickyNode.cpp:
+        (WebCore::ScrollingTreeStickyNode::computeLayerPosition const):
+        (WebCore::ScrollingTreeStickyNode::scrollDeltaSinceLastCommit const):
+        * page/scrolling/nicosia/ScrollingTreeStickyNode.h:
+
 2021-10-12  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp

@@ -87 +87 @@
 {
     // This logic follows ScrollingTreeStickyNode::computeLayerPosition().
+    FloatSize offsetFromStickyAncestors;
     auto computeLayerPositionForScrollingNode = [&](ScrollingStateNode& scrollingStateNode) {
         FloatRect constrainingRect;
@@ -95 +96 @@
             constrainingRect = FloatRect(overflowScrollingNode.scrollPosition(), m_constraints.constrainingRectAtLastLayout().size());
         }
+        constrainingRect.move(offsetFromStickyAncestors);
         return m_constraints.layerPositionForConstrainingRect(constrainingRect);
     };
@@ -111 +113 @@
             return computeLayerPositionForScrollingNode(*ancestor);
 
-        if (is<ScrollingStateFixedNode>(*ancestor) || is<ScrollingStateStickyNode>(*ancestor)) {
+        if (is<ScrollingStateStickyNode>(*ancestor))
+            offsetFromStickyAncestors += downcast<ScrollingStateStickyNode>(*ancestor).scrollDeltaSinceLastCommit(viewportRect);
+
+        if (is<ScrollingStateFixedNode>(*ancestor)) {
             // FIXME: Do we need scrolling tree nodes at all for nested cases?
             return m_constraints.layerPositionAtLastLayout();
@@ -142 +147 @@
         }
     }
+}
+
+FloatSize ScrollingStateStickyNode::scrollDeltaSinceLastCommit(const LayoutRect& viewportRect) const
+{
+    auto layerPosition = computeLayerPosition(viewportRect);
+    return layerPosition - m_constraints.layerPositionAtLastLayout();
 }
 

trunk/Source/WebCore/page/scrolling/ScrollingStateStickyNode.h

@@ -54 +54 @@
     FloatPoint computeLayerPosition(const LayoutRect& viewportRect) const;
     void reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction) final;
+    FloatSize scrollDeltaSinceLastCommit(const LayoutRect& viewportRect) const;
 
     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const final;

trunk/Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm

@@ -70 +70 @@
 FloatPoint ScrollingTreeStickyNode::computeLayerPosition() const
 {
+    FloatSize offsetFromStickyAncestors;
     auto computeLayerPositionForScrollingNode = [&](ScrollingTreeNode& scrollingNode) {
         FloatRect constrainingRect;
@@ -80 +81 @@
             constrainingRect.move(overflowScrollingNode.scrollDeltaSinceLastCommit());
         }
+        constrainingRect.move(-offsetFromStickyAncestors);
         return m_constraints.layerPositionForConstrainingRect(constrainingRect);
     };
@@ -96 +98 @@
             return computeLayerPositionForScrollingNode(*ancestor);
 
-        if (is<ScrollingTreeFixedNode>(*ancestor) || is<ScrollingTreeStickyNode>(*ancestor)) {
+        if (is<ScrollingTreeStickyNode>(*ancestor))
+            offsetFromStickyAncestors += downcast<ScrollingTreeStickyNode>(*ancestor).scrollDeltaSinceLastCommit();
+
+        if (is<ScrollingTreeFixedNode>(*ancestor)) {
             // FIXME: Do we need scrolling tree nodes at all for nested cases?
             return m_constraints.layerPositionAtLastLayout();

trunk/Source/WebCore/page/scrolling/nicosia/ScrollingTreeStickyNode.cpp

@@ -112 +112 @@
 FloatPoint ScrollingTreeStickyNode::computeLayerPosition() const
 {
+    FloatSize offsetFromStickyAncestors;
     auto computeLayerPositionForScrollingNode = [&](ScrollingTreeNode& scrollingNode) {
         FloatRect constrainingRect;
@@ -122 +123 @@
             constrainingRect.move(overflowScrollingNode.scrollDeltaSinceLastCommit());
         }
+        constrainingRect.move(-offsetFromStickyAncestors);
         return m_constraints.layerPositionForConstrainingRect(constrainingRect);
     };
@@ -138 +140 @@
             return computeLayerPositionForScrollingNode(*ancestor);
 
-        if (is<ScrollingTreeFixedNode>(*ancestor) || is<ScrollingTreeStickyNode>(*ancestor)) {
+        if (is<ScrollingTreeStickyNode>(*ancestor))
+            offsetFromStickyAncestors += downcast<ScrollingTreeStickyNode>(*ancestor).scrollDeltaSinceLastCommit();
+
+        if (is<ScrollingTreeFixedNode>(*ancestor)) {
             // FIXME: Do we need scrolling tree nodes at all for nested cases?
             return m_constraints.layerPositionAtLastLayout();
@@ -147 +152 @@
 }
 
+FloatSize ScrollingTreeStickyNode::scrollDeltaSinceLastCommit() const
+{
+    auto layerPosition = computeLayerPosition();
+    return layerPosition - m_constraints.layerPositionAtLastLayout();
+}
+
 } // namespace WebCore
 

trunk/Source/WebCore/page/scrolling/nicosia/ScrollingTreeStickyNode.h

@@ -55 +55 @@
 
     FloatPoint computeLayerPosition() const;
+    FloatSize scrollDeltaSinceLastCommit() const;
 
     StickyPositionViewportConstraints m_constraints;