Changeset 243539 in webkit

Timestamp:

Mar 27, 2019, 8:22:03 AM (7 years ago)

Author:

Simon Fraser

Message:

[iOS WK2] Fixed elements in frames can be misplaced sometimes
​https://bugs.webkit.org/show_bug.cgi?id=196290

Reviewed by Frédéric Wang.

Source/WebCore:

In a page containing position:fixed inside an async-scrolling iframe, if the
main page is scrolled down, and you reload, then the fixed element in the iframe can
get misplaced or disappear.

The bug was that the reconcileViewportConstrainedLayerPositions() recursive state node
walk would cross frame boundaries, hitting subframe ScrollingStateFixedNodes with a viewport rect
for the main page.

Fix by giving ScrollingStateTree the responsibility for the recursive tree walk, and
have it bail at at frame boundaries.

Test: scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position.html

• page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions):

• page/scrolling/ScrollingStateFixedNode.cpp:

(WebCore::ScrollingStateFixedNode::reconcileLayerPositionForViewportRect):

• page/scrolling/ScrollingStateNode.cpp:

(WebCore::ScrollingStateNode::reconcileLayerPositionForViewportRect): Deleted.

• page/scrolling/ScrollingStateNode.h:

(WebCore::ScrollingStateNode::reconcileLayerPositionForViewportRect):

• page/scrolling/ScrollingStateStickyNode.cpp:

(WebCore::ScrollingStateStickyNode::reconcileLayerPositionForViewportRect):

• page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::reconcileLayerPositionsRecursive):
(WebCore::ScrollingStateTree::reconcileViewportConstrainedLayerPositions):

• page/scrolling/ScrollingStateTree.h:

LayoutTests:

• scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position-expected.txt: Added.
• scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position-expected.txt (added)
• LayoutTests/scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateNode.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateNode.h (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateTree.cpp (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateTree.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-03-26  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] Fixed elements in frames can be misplaced sometimes
+        https://bugs.webkit.org/show_bug.cgi?id=196290
+
+        Reviewed by Frédéric Wang.
+
+        * scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position-expected.txt: Added.
+        * scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position.html: Added.
+
 2019-03-26  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-26  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] Fixed elements in frames can be misplaced sometimes
+        https://bugs.webkit.org/show_bug.cgi?id=196290
+
+        Reviewed by Frédéric Wang.
+
+        In a page containing position:fixed inside an async-scrolling iframe, if the 
+        main page is scrolled down, and you reload, then the fixed element in the iframe can
+        get misplaced or disappear.
+
+        The bug was that the reconcileViewportConstrainedLayerPositions() recursive state node
+        walk would cross frame boundaries, hitting subframe ScrollingStateFixedNodes with a viewport rect
+        for the main page.
+
+        Fix by giving ScrollingStateTree the responsibility for the recursive tree walk, and
+        have it bail at at frame boundaries.
+
+        Test: scrollingcoordinator/ios/fixed-in-frame-layer-reconcile-layer-position.html
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions):
+        * page/scrolling/ScrollingStateFixedNode.cpp:
+        (WebCore::ScrollingStateFixedNode::reconcileLayerPositionForViewportRect):
+        * page/scrolling/ScrollingStateNode.cpp:
+        (WebCore::ScrollingStateNode::reconcileLayerPositionForViewportRect): Deleted.
+        * page/scrolling/ScrollingStateNode.h:
+        (WebCore::ScrollingStateNode::reconcileLayerPositionForViewportRect):
+        * page/scrolling/ScrollingStateStickyNode.cpp:
+        (WebCore::ScrollingStateStickyNode::reconcileLayerPositionForViewportRect):
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::reconcileLayerPositionsRecursive):
+        (WebCore::ScrollingStateTree::reconcileViewportConstrainedLayerPositions):
+        * page/scrolling/ScrollingStateTree.h:
+
 2019-03-27  Philippe Normand  <pnormand@igalia.com>
 

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

@@ -540 +540 @@
 void AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions(ScrollingNodeID scrollingNodeID, const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
 {
-    auto* scrollingNode = m_scrollingStateTree->stateNodeForID(scrollingNodeID);
-    if (!scrollingNode)
-        return;
-
     LOG_WITH_STREAM(Scrolling, stream << getCurrentProcessID() << " AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions for viewport rect " << viewportRect << " and node " << scrollingNodeID);
 
-    scrollingNode->reconcileLayerPositionForViewportRect(viewportRect, action);
+    m_scrollingStateTree->reconcileViewportConstrainedLayerPositions(scrollingNodeID, viewportRect, action);
 }
 

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

@@ -78 +78 @@
 void ScrollingStateFixedNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
 {
-    ScrollingStateNode::reconcileLayerPositionForViewportRect(viewportRect, action);
-
     FloatPoint position = m_constraints.layerPositionForViewportRect(viewportRect);
     if (layer().representsGraphicsLayer()) {

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

@@ -152 +152 @@
 }
 
-void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
-{
-    if (!m_children)
-        return;
-
-    for (auto& child : *m_children)
-        child->reconcileLayerPositionForViewportRect(viewportRect, action);
-}
-
 void ScrollingStateNode::setLayer(const LayerRepresentation& layerRepresentation)
 {

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

@@ -218 +218 @@
     void setChangedProperties(ChangedProperties changedProperties) { m_changedProperties = changedProperties; }
     
-    virtual void reconcileLayerPositionForViewportRect(const LayoutRect& /*viewportRect*/, ScrollingLayerPositionAction);
+    virtual void reconcileLayerPositionForViewportRect(const LayoutRect& /*viewportRect*/, ScrollingLayerPositionAction) { }
 
     const LayerRepresentation& layer() const { return m_layer; }

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

@@ -78 +78 @@
 void ScrollingStateStickyNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
 {
-    ScrollingStateNode::reconcileLayerPositionForViewportRect(viewportRect, action);
-
     FloatPoint position = m_constraints.layerPositionForConstrainingRect(viewportRect);
     if (layer().representsGraphicsLayer()) {

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

@@ -365 +365 @@
 }
 
+void ScrollingStateTree::reconcileLayerPositionsRecursive(ScrollingStateNode& currNode, const LayoutRect& layoutViewport, ScrollingLayerPositionAction action)
+{
+    currNode.reconcileLayerPositionForViewportRect(layoutViewport, action);
+
+    if (!currNode.children())
+        return;
+    
+    for (auto& child : *currNode.children()) {
+        // Never need to cross frame boundaries, since viewport rect reconciliation is per frame.
+        if (is<ScrollingStateFrameScrollingNode>(child))
+            continue;
+
+        reconcileLayerPositionsRecursive(*child, layoutViewport, action);
+    }
+}
+
+void ScrollingStateTree::reconcileViewportConstrainedLayerPositions(ScrollingNodeID scrollingNodeID, const LayoutRect& layoutViewport, ScrollingLayerPositionAction action)
+{
+    auto* scrollingNode = stateNodeForID(scrollingNodeID);
+    if (!scrollingNode)
+        return;
+    
+    reconcileLayerPositionsRecursive(*scrollingNode, layoutViewport, action);
+}
+
 } // namespace WebCore
 

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

@@ -76 +76 @@
     void setPreferredLayerRepresentation(LayerRepresentation::Type representation) { m_preferredLayerRepresentation = representation; }
 
+    void reconcileViewportConstrainedLayerPositions(ScrollingNodeID, const LayoutRect& viewportRect, ScrollingLayerPositionAction);
+
 private:
     void setRootStateNode(Ref<ScrollingStateFrameScrollingNode>&&);
@@ -88 +90 @@
     void removeNodeAndAllDescendants(ScrollingStateNode*);
 
-    void recursiveNodeWillBeRemoved(ScrollingStateNode* currNode);
+    void recursiveNodeWillBeRemoved(ScrollingStateNode*);
     void willRemoveNode(ScrollingStateNode*);
+
+    void reconcileLayerPositionsRecursive(ScrollingStateNode&, const LayoutRect& viewportRect, ScrollingLayerPositionAction);
 
     AsyncScrollingCoordinator* m_scrollingCoordinator;