Changeset 246197 in webkit

Timestamp:

Jun 7, 2019, 7:17:49 AM (7 years ago)

Author:

Antti Koivisto

Message:

position:fixed inside overflow positioning nodes is jumpy
​https://bugs.webkit.org/show_bug.cgi?id=198647
<rdar://problem/51514437>

Reviewed by Frédéric Wang.

Source/WebCore:

Tests: scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1.html

scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2.html
scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary.html

• page/scrolling/ScrollingTreeScrollingNode.h:
• page/scrolling/cocoa/ScrollingTreeFixedNode.mm:

(WebCore::ScrollingTreeFixedNode::applyLayerPositions):

Take deltas from positioning nodes into account.

• page/scrolling/cocoa/ScrollingTreePositionedNode.h:
• page/scrolling/cocoa/ScrollingTreePositionedNode.mm:

(WebCore::ScrollingTreePositionedNode::scrollDeltaSinceLastCommit const):

Rename since 'scrollOffset' has other meaning.

(WebCore::ScrollingTreePositionedNode::applyLayerPositions):
(WebCore::ScrollingTreePositionedNode::scrollOffsetSinceLastCommit const): Deleted.

• page/scrolling/cocoa/ScrollingTreeStickyNode.mm:

(WebCore::ScrollingTreeStickyNode::applyLayerPositions):

LayoutTests:

• scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1-expected.html: Added.
• scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1.html: Added.
• scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2-expected.html: Added.
• scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2.html: Added.
• scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary-expected.html: Added.
• scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1-expected.html (added)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1.html (added)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2-expected.html (added)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2.html (added)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary-expected.html (added)
• LayoutTests/scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h (modified) (1 diff)
• Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm (modified) (2 diffs)
• Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h (modified) (1 diff)
• Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm (modified) (2 diffs)
• Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-06-07  Antti Koivisto  <antti@apple.com>
+
+        position:fixed inside overflow positioning nodes is jumpy
+        https://bugs.webkit.org/show_bug.cgi?id=198647
+        <rdar://problem/51514437>
+
+        Reviewed by Frédéric Wang.
+
+        * scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1-expected.html: Added.
+        * scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1.html: Added.
+        * scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2-expected.html: Added.
+        * scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2.html: Added.
+        * scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary-expected.html: Added.
+        * scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary.html: Added.
+
 2019-06-07  Enrique Ocaña González  <eocanha@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-06-07  Antti Koivisto  <antti@apple.com>
+
+        position:fixed inside overflow positioning nodes is jumpy
+        https://bugs.webkit.org/show_bug.cgi?id=198647
+        <rdar://problem/51514437>
+
+        Reviewed by Frédéric Wang.
+
+        Tests: scrollingcoordinator/ios/fixed-overflow-no-stacking-context-1.html
+               scrollingcoordinator/ios/fixed-overflow-no-stacking-context-2.html
+               scrollingcoordinator/ios/fixed-overflow-stacking-context-stationary.html
+
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+        * page/scrolling/cocoa/ScrollingTreeFixedNode.mm:
+        (WebCore::ScrollingTreeFixedNode::applyLayerPositions):
+
+        Take deltas from positioning nodes into account.
+
+        * page/scrolling/cocoa/ScrollingTreePositionedNode.h:
+        * page/scrolling/cocoa/ScrollingTreePositionedNode.mm:
+        (WebCore::ScrollingTreePositionedNode::scrollDeltaSinceLastCommit const):
+
+        Rename since 'scrollOffset' has other meaning.
+
+        (WebCore::ScrollingTreePositionedNode::applyLayerPositions):
+        (WebCore::ScrollingTreePositionedNode::scrollOffsetSinceLastCommit const): Deleted.
+        * page/scrolling/cocoa/ScrollingTreeStickyNode.mm:
+        (WebCore::ScrollingTreeStickyNode::applyLayerPositions):
+
 2019-06-07  Enrique Ocaña González  <eocanha@igalia.com>
 

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

@@ -56 +56 @@
     FloatPoint currentScrollPosition() const { return m_currentScrollPosition; }
     FloatPoint lastCommittedScrollPosition() const { return m_lastCommittedScrollPosition; }
+    FloatSize scrollDeltaSinceLastCommit() const { return m_currentScrollPosition - m_lastCommittedScrollPosition; }
 
     // These are imperative; they adjust the scrolling layers.

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

@@ -34 +34 @@
 #import "ScrollingTreeFrameScrollingNode.h"
 #import "ScrollingTreeOverflowScrollingNode.h"
+#import "ScrollingTreePositionedNode.h"
 #import "WebCoreCALayerExtras.h"
 #import <wtf/text/TextStream.h>
@@ -70 +71 @@
     auto computeLayerPosition = [&] {
         FloatSize overflowScrollDelta;
-        // FIXME: This code is wrong in complex cases where the fixed element is inside a positioned node as
-        //        the scroll container order does not match the scrolling tree ancestor order.
-        for (auto* node = parent(); node; node = node->parent()) {
-            if (is<ScrollingTreeFrameScrollingNode>(*node)) {
+        for (auto* ancestor = parent(); ancestor; ancestor = ancestor->parent()) {
+            if (is<ScrollingTreePositionedNode>(*ancestor)) {
+                auto& positioningAncestor = downcast<ScrollingTreePositionedNode>(*ancestor);
+                if (positioningAncestor.layer() != m_layer)
+                    overflowScrollDelta -= positioningAncestor.scrollDeltaSinceLastCommit();
+            }
+
+            if (is<ScrollingTreeFrameScrollingNode>(*ancestor)) {
                 // Fixed nodes are positioned relative to the containing frame scrolling node.
                 // We bail out after finding one.
-                auto layoutViewport = downcast<ScrollingTreeFrameScrollingNode>(*node).layoutViewport();
+                auto layoutViewport = downcast<ScrollingTreeFrameScrollingNode>(*ancestor).layoutViewport();
                 return m_constraints.layerPositionForViewportRect(layoutViewport) - overflowScrollDelta;
             }
 
-            if (is<ScrollingTreeOverflowScrollingNode>(*node)) {
+            if (is<ScrollingTreeOverflowScrollingNode>(*ancestor)) {
                 // To keep the layer still during async scrolling we adjust by how much the position has changed since layout.
-                auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*node);
-                auto localDelta = overflowNode.lastCommittedScrollPosition() - overflowNode.currentScrollPosition();
-                overflowScrollDelta += localDelta;
+                auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*ancestor);
+                overflowScrollDelta -= overflowNode.scrollDeltaSinceLastCommit();
             }
         }

trunk/Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h

@@ -47 +47 @@
     const Vector<ScrollingNodeID>& relatedOverflowScrollingNodes() const { return m_relatedOverflowScrollingNodes; }
 
-    FloatSize scrollOffsetSinceLastCommit() const;
+    FloatSize scrollDeltaSinceLastCommit() const;
 
 private:

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

@@ -77 +77 @@
 }
 
-FloatSize ScrollingTreePositionedNode::scrollOffsetSinceLastCommit() const
+FloatSize ScrollingTreePositionedNode::scrollDeltaSinceLastCommit() const
 {
-    FloatSize offset;
+    FloatSize delta;
     for (auto nodeID : m_relatedOverflowScrollingNodes) {
         if (auto* node = scrollingTree().nodeForID(nodeID)) {
             if (is<ScrollingTreeOverflowScrollingNode>(node)) {
                 auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*node);
-                offset += overflowNode.currentScrollPosition() - overflowNode.lastCommittedScrollPosition();
+                delta += overflowNode.scrollDeltaSinceLastCommit();
             }
         }
@@ -90 +90 @@
     if (m_constraints.scrollPositioningBehavior() == ScrollPositioningBehavior::Stationary) {
         // Stationary nodes move in the opposite direction.
-        return -offset;
+        return -delta;
     }
 
-    return offset;
+    return delta;
 }
 
 void ScrollingTreePositionedNode::applyLayerPositions()
 {
-    auto offset = scrollOffsetSinceLastCommit();
-    auto layerPosition = m_constraints.layerPositionAtLastLayout() - offset;
+    auto delta = scrollDeltaSinceLastCommit();
+    auto layerPosition = m_constraints.layerPositionAtLastLayout() - delta;
 
-    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreePositionedNode " << scrollingNodeID() << " applyLayerPositions: overflow delta " << offset << " moving layer to " << layerPosition);
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreePositionedNode " << scrollingNodeID() << " applyLayerPositions: overflow delta " << delta << " moving layer to " << layerPosition);
 
     [m_layer _web_setLayerTopLeftPosition:layerPosition - m_constraints.alignmentOffset()];

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

@@ -98 +98 @@
                     if (positioningAncestor.layer() == m_layer) {
                         // We'll also do the adjustment the positioning node would do.
-                        position -= positioningAncestor.scrollOffsetSinceLastCommit();
+                        position -= positioningAncestor.scrollDeltaSinceLastCommit();
                     }