Changeset 242601 in webkit

Timestamp:

Mar 7, 2019, 10:03:41 AM (7 years ago)

Author:

Simon Fraser

Message:

[iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit)
​https://bugs.webkit.org/show_bug.cgi?id=195396
rdar://problem/48518959

Reviewed by Antti Koivisto.

r242132 introduced two issues that contributed to jumpiness of position:fixed layers when scrolling.

First, ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling() would early return if the scroll position
hadn't changed. It also needs to check the supplied layoutViewport (if any), but in some cases running the
notifyRelatedNodesAfterScrollPositionChange() code is necessary even without a scroll position change:
if the web process has committed new scrolling tree state (e.g. with new fixed constraints) since
the last call, we have to run the layer positioning code to have fixed layers re-adjust their position relative
to the root. This was the primary bug fix.

Secondly, a layer tree commit can give ScrollingTreeFrameScrollingNode a new layout viewport, but we need to
adjust this by the scrolling tree's current scroll position in case it gets used before the next scroll.

Currently no way to test this, as it's very timing-dependent.

• page/scrolling/ScrollingTreeFrameScrollingNode.cpp:

(WebCore::ScrollingTreeFrameScrollingNode::commitStateBeforeChildren):
(WebCore::ScrollingTreeFrameScrollingNode::scrollPositionAndLayoutViewportMatch):

• page/scrolling/ScrollingTreeFrameScrollingNode.h:
• page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::scrollPositionAndLayoutViewportMatch):
(WebCore::ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling):

• page/scrolling/ScrollingTreeScrollingNode.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ScrollingTreeFrameScrollingNode.cpp (modified) (2 diffs)
• page/scrolling/ScrollingTreeFrameScrollingNode.h (modified) (1 diff)
• page/scrolling/ScrollingTreeScrollingNode.cpp (modified) (2 diffs)
• page/scrolling/ScrollingTreeScrollingNode.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-07  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling (Apple, Tesla, YouTube, Reddit)
+        https://bugs.webkit.org/show_bug.cgi?id=195396
+        rdar://problem/48518959
+
+        Reviewed by Antti Koivisto.
+        
+        r242132 introduced two issues that contributed to jumpiness of position:fixed layers when scrolling.
+        
+        First, ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling() would early return if the scroll position
+        hadn't changed. It also needs to check the supplied layoutViewport (if any), but in some cases running the
+        notifyRelatedNodesAfterScrollPositionChange() code is necessary even without a scroll position change:
+        if the web process has committed new scrolling tree state (e.g. with new fixed constraints) since
+        the last call, we have to run the layer positioning code to have fixed layers re-adjust their position relative
+        to the root. This was the primary bug fix.
+
+        Secondly, a layer tree commit can give ScrollingTreeFrameScrollingNode a new layout viewport, but we need to
+        adjust this by the scrolling tree's current scroll position in case it gets used before the next scroll.
+
+        Currently no way to test this, as it's very timing-dependent.
+
+        * page/scrolling/ScrollingTreeFrameScrollingNode.cpp:
+        (WebCore::ScrollingTreeFrameScrollingNode::commitStateBeforeChildren):
+        (WebCore::ScrollingTreeFrameScrollingNode::scrollPositionAndLayoutViewportMatch):
+        * page/scrolling/ScrollingTreeFrameScrollingNode.h:
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::scrollPositionAndLayoutViewportMatch):
+        (WebCore::ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+
 2019-03-07  Youenn Fablet  <youenn@apple.com>
 

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

@@ -73 +73 @@
         m_fixedElementsLayoutRelativeToFrame = state.fixedElementsLayoutRelativeToFrame();
 
-    if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::LayoutViewport))
+    if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::LayoutViewport)) {
         m_layoutViewport = state.layoutViewport();
+        updateViewportForCurrentScrollPosition({ });
+    }
 
     if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::MinLayoutViewportOrigin))
@@ -81 +83 @@
     if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::MaxLayoutViewportOrigin))
         m_maxLayoutViewportOrigin = state.maxLayoutViewportOrigin();
+}
+
+bool ScrollingTreeFrameScrollingNode::scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
+{
+    return position == currentScrollPosition() && (!overrideLayoutViewport || overrideLayoutViewport.value() == m_layoutViewport);
 }
 

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

@@ -71 +71 @@
 
     WEBCORE_EXPORT void updateViewportForCurrentScrollPosition(Optional<FloatRect>) override;
+    bool scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport) override;
 
     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;

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

@@ -175 +175 @@
 }
 
+bool ScrollingTreeScrollingNode::scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect>)
+{
+    return position == m_currentScrollPosition;
+}
+
 void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
 {
-    if (position == m_currentScrollPosition)
-        return;
+    // Even if position and overrideLayoutViewport haven't changed for this node, other nodes may have received new constraint data
+    // via a commit, so the call to notifyRelatedNodesAfterScrollPositionChange() is necessary. We could avoid this if we knew that
+    // no commits had happened.
+    bool scrollPositionChanged = !scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
 
     m_currentScrollPosition = adjustedScrollPosition(position, ScrollPositionClamp::None);
@@ -186 +193 @@
 
     scrollingTree().notifyRelatedNodesAfterScrollPositionChange(*this);
-    scrollingTree().scrollingTreeNodeDidScroll(*this);
+    
+    if (scrollPositionChanged)
+        scrollingTree().scrollingTreeNodeDidScroll(*this);
 }
 

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

@@ -94 +94 @@
     virtual void currentScrollPositionChanged();
     WEBCORE_EXPORT virtual void updateViewportForCurrentScrollPosition(Optional<FloatRect> = { }) { }
+    virtual bool scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport);
 
     WEBCORE_EXPORT virtual void repositionScrollingLayers() { }