[iOS WK] REGRESSION (r242132): Fixed position banners flicker and move when scrolling...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2019 18:03:41 +0000 (18:03 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2019 18:03:41 +0000 (18:03 +0000)
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:

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

Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.h
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h

index 0ad3ee1..086c2db 100644 (file)
@@ -1,3 +1,34 @@
+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>
 
         Introduce a quota manager for Cache API/Service Worker/IDB storage
index 4b2581d..bade01d 100644 (file)
@@ -72,8 +72,10 @@ void ScrollingTreeFrameScrollingNode::commitStateBeforeChildren(const ScrollingS
     if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::FixedElementsLayoutRelativeToFrame))
         m_fixedElementsLayoutRelativeToFrame = state.fixedElementsLayoutRelativeToFrame();
 
-    if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::LayoutViewport))
+    if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::LayoutViewport)) {
         m_layoutViewport = state.layoutViewport();
+        updateViewportForCurrentScrollPosition({ });
+    }
 
     if (state.hasChangedProperty(ScrollingStateFrameScrollingNode::MinLayoutViewportOrigin))
         m_minLayoutViewportOrigin = state.minLayoutViewportOrigin();
@@ -82,6 +84,11 @@ void ScrollingTreeFrameScrollingNode::commitStateBeforeChildren(const ScrollingS
         m_maxLayoutViewportOrigin = state.maxLayoutViewportOrigin();
 }
 
+bool ScrollingTreeFrameScrollingNode::scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
+{
+    return position == currentScrollPosition() && (!overrideLayoutViewport || overrideLayoutViewport.value() == m_layoutViewport);
+}
+
 FloatRect ScrollingTreeFrameScrollingNode::layoutViewportForScrollPosition(const FloatPoint& visibleContentOrigin, float scale) const
 {
     FloatRect visibleContentRect(visibleContentOrigin, scrollableAreaSize());
index 8f080e1..59ca2f6 100644 (file)
@@ -70,6 +70,7 @@ private:
     LayoutPoint localToContentsPoint(LayoutPoint) const final;
 
     WEBCORE_EXPORT void updateViewportForCurrentScrollPosition(Optional<FloatRect>) override;
+    bool scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport) override;
 
     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
 
index 32579e3..5bd7e25 100644 (file)
@@ -174,10 +174,17 @@ void ScrollingTreeScrollingNode::currentScrollPositionChanged()
     scrollingTree().scrollingTreeNodeDidScroll(*this);
 }
 
+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);
     updateViewportForCurrentScrollPosition(overrideLayoutViewport);
@@ -185,7 +192,9 @@ void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoin
     repositionRelatedLayers();
 
     scrollingTree().notifyRelatedNodesAfterScrollPositionChange(*this);
-    scrollingTree().scrollingTreeNodeDidScroll(*this);
+    
+    if (scrollPositionChanged)
+        scrollingTree().scrollingTreeNodeDidScroll(*this);
 }
 
 LayoutPoint ScrollingTreeScrollingNode::parentToLocalPoint(LayoutPoint point) const
index c10cff6..2a19836 100644 (file)
@@ -93,6 +93,7 @@ protected:
 
     virtual void currentScrollPositionChanged();
     WEBCORE_EXPORT virtual void updateViewportForCurrentScrollPosition(Optional<FloatRect> = { }) { }
+    virtual bool scrollPositionAndLayoutViewportMatch(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport);
 
     WEBCORE_EXPORT virtual void repositionScrollingLayers() { }
     WEBCORE_EXPORT virtual void repositionRelatedLayers() { }