[iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Apr 2019 05:18:17 +0000 (05:18 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Apr 2019 05:18:17 +0000 (05:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195584

Reviewed by Zalan Bujtas.

Source/WebCore:

Push data to the scrolling tree about whether an overflow:scroll scroll was programmatic, by having
RenderLayer::scrollToOffset() call into AsyncScrollingCoordinator::requestScrollPositionUpdate(),
just as we do for frames.

AsyncScrollingCoordinator::requestScrollPositionUpdate() is generalized to take any ScrollableArea.

Fix an assumption in the ScrollingTree that we only care about programmatic scrolls on the root node.
ScrollingTree::commitTreeState() no longer sets isHandlingProgrammaticScroll; instead,
callers of ScrollingTreeScrollingNode::scrollTo() pass a ScrollType. Commit functions pass
ScrollType::Programmatic when handling RequestedScrollPosition changes as necessary.

Programmatic scrolls need to get to the scrolling tree in the UI process so that we update
the tree's notion of scroll position, and trigger actual UIScrollView scrolls (layers may have
already been put in the right locations, but the UI process needs to know that a scroll happened).
However, we need to prevent notifications from programmatic scrolls getting back to the
web process, because this causes jumpiness. This is done via an early return in
RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll().

Tests: scrollingcoordinator/ios/programmatic-overflow-scroll.html
       scrollingcoordinator/ios/programmatic-page-scroll.html

* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
* page/scrolling/AsyncScrollingCoordinator.h:
* page/scrolling/ScrollingCoordinator.h:
(WebCore::ScrollingCoordinator::requestScrollPositionUpdate):
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::commitTreeState):
(WebCore::ScrollingTree::isHandlingProgrammaticScroll): Deleted.
* page/scrolling/ScrollingTree.h:
(WebCore::ScrollingTree::isHandlingProgrammaticScroll const):
(WebCore::ScrollingTree::setIsHandlingProgrammaticScroll):
* page/scrolling/ScrollingTreeScrollingNode.cpp:
(WebCore::ScrollingTreeScrollingNode::scrollBy):
(WebCore::ScrollingTreeScrollingNode::scrollTo):
* page/scrolling/ScrollingTreeScrollingNode.h:
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::commitStateAfterChildren):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollToOffset):
(WebCore::RenderLayer::scrollingNodeID const):
* rendering/RenderLayer.h:
* rendering/RenderMarquee.cpp:
(WebCore::RenderMarquee::timerFired):

Source/WebKit:

Push data to the scrolling tree about whether an overflow:scroll scroll was programmatic, by having
RenderLayer::scrollToOffset() call into AsyncScrollingCoordinator::requestScrollPositionUpdate(),
just as we do for frames.

AsyncScrollingCoordinator::requestScrollPositionUpdate() is generalized to take any ScrollableArea.

Fix an assumption in the ScrollingTree that we only care about programmatic scrolls on the root node.
ScrollingTree::commitTreeState() no longer sets isHandlingProgrammaticScroll; instead,
callers of ScrollingTreeScrollingNode::scrollTo() pass a ScrollType. Commit functions pass
ScrollType::Programmatic when handling RequestedScrollPosition changes as necessary.

Programmatic scrolls need to get to the scrolling tree in the UI process so that we update
the tree's notion of scroll position, and trigger actual UIScrollView scrolls (layers may have
already been put in the right locations, but the UI process needs to know that a scroll happened).
However, we need to prevent notifications from programmatic scrolls getting back to the
web process, because this causes jumpiness. This is done via an early return in
RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll().

* UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
(WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll):
* UIProcess/RemoteLayerTree/ios/ScrollingTreeFrameScrollingNodeRemoteIOS.mm:
(WebKit::ScrollingTreeFrameScrollingNodeRemoteIOS::commitStateAfterChildren): Subframe nodes have
a delegate, and that will take care of the requestedScrollPosition update.
* UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.h:
* UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:
(WebKit::ScrollingTreeOverflowScrollingNodeIOS::commitStateAfterChildren):
* UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:
(WebKit::ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren):

LayoutTests:

Testing of programmatic scrolls in frames is prevented by webkit.org/b/196635.

* scrollingcoordinator/ios/programmatic-overflow-scroll-expected.html: Added.
* scrollingcoordinator/ios/programmatic-overflow-scroll.html: Added.
* scrollingcoordinator/ios/programmatic-page-scroll-expected.html: Added.
* scrollingcoordinator/ios/programmatic-page-scroll.html: Added.

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

23 files changed:
LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll-expected.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll-expected.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingTree.cpp
Source/WebCore/page/scrolling/ScrollingTree.h
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderMarquee.cpp
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeFrameScrollingNodeRemoteIOS.mm
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.h
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.mm
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm

index 969598d..258959f 100644 (file)
@@ -1,3 +1,17 @@
+2019-04-04  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in bad rendering
+        https://bugs.webkit.org/show_bug.cgi?id=195584
+
+        Reviewed by Zalan Bujtas.
+
+        Testing of programmatic scrolls in frames is prevented by webkit.org/b/196635.
+
+        * scrollingcoordinator/ios/programmatic-overflow-scroll-expected.html: Added.
+        * scrollingcoordinator/ios/programmatic-overflow-scroll.html: Added.
+        * scrollingcoordinator/ios/programmatic-page-scroll-expected.html: Added.
+        * scrollingcoordinator/ios/programmatic-page-scroll.html: Added.
+
 2019-04-04  Shawn Roberts  <sroberts@apple.com>
 
         Unreviewed, rolling out r243868.
diff --git a/LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll-expected.html b/LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll-expected.html
new file mode 100644 (file)
index 0000000..bc32c36
--- /dev/null
@@ -0,0 +1,33 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width">
+    <style>
+        #scroller {
+            height: 300px;
+            width: 300px;
+            overflow: scroll;
+            -webkit-overflow-scrolling: touch;
+        }
+        
+        .box {
+            width: 100%;
+            height: 100%;
+        }
+        
+        .top {
+            background-color: green;
+        }
+
+        .bottom {
+            background-color: green;
+        }
+    </style>
+</head>
+<body>
+    <div id="scroller">
+        <div class="top box"></div>
+        <div class="bottom box"></div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll.html b/LayoutTests/scrollingcoordinator/ios/programmatic-overflow-scroll.html
new file mode 100644 (file)
index 0000000..7bc82ca
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width">
+    <style>
+        #scroller {
+            height: 300px;
+            width: 300px;
+            overflow: scroll;
+            -webkit-overflow-scrolling: touch;
+        }
+        
+        .box {
+            width: 100%;
+            height: 100%;
+        }
+        
+        .top {
+            background-color: red;
+        }
+
+        .bottom {
+            background-color: green;
+        }
+
+    </style>
+    <script>
+        function doTest()
+        {
+            scroller.scrollTop = 500;
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div id="scroller">
+        <div class="top box"></div>
+        <div class="bottom box"></div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll-expected.html b/LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll-expected.html
new file mode 100644 (file)
index 0000000..f0e81d3
--- /dev/null
@@ -0,0 +1,52 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width">
+    <style>
+        body {
+            height: 2000px;
+        }
+        #scroller {
+            height: 300px;
+            width: 300px;
+            overflow: scroll;
+            -webkit-overflow-scrolling: touch;
+        }
+        
+        .box {
+            width: 100%;
+            height: 100%;
+        }
+        
+        .top {
+            background-color: red;
+        }
+
+        .bottom {
+            background-color: green;
+        }
+
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function doTest()
+        {
+            setTimeout(() => {
+                document.scrollingElement.scrollTop = 200;
+            }, 2000);
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div id="scroller">
+        <div class="top box"></div>
+        <div class="bottom box"></div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll.html b/LayoutTests/scrollingcoordinator/ios/programmatic-page-scroll.html
new file mode 100644 (file)
index 0000000..f0e81d3
--- /dev/null
@@ -0,0 +1,52 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <meta name="viewport" content="width=device-width">
+    <style>
+        body {
+            height: 2000px;
+        }
+        #scroller {
+            height: 300px;
+            width: 300px;
+            overflow: scroll;
+            -webkit-overflow-scrolling: touch;
+        }
+        
+        .box {
+            width: 100%;
+            height: 100%;
+        }
+        
+        .top {
+            background-color: red;
+        }
+
+        .bottom {
+            background-color: green;
+        }
+
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function doTest()
+        {
+            setTimeout(() => {
+                document.scrollingElement.scrollTop = 200;
+            }, 2000);
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div id="scroller">
+        <div class="top box"></div>
+        <div class="bottom box"></div>
+    </div>
+</body>
+</html>
index 78556f7..e511a7f 100644 (file)
@@ -1,3 +1,55 @@
+2019-04-04  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in bad rendering
+        https://bugs.webkit.org/show_bug.cgi?id=195584
+
+        Reviewed by Zalan Bujtas.
+
+        Push data to the scrolling tree about whether an overflow:scroll scroll was programmatic, by having
+        RenderLayer::scrollToOffset() call into AsyncScrollingCoordinator::requestScrollPositionUpdate(),
+        just as we do for frames.
+
+        AsyncScrollingCoordinator::requestScrollPositionUpdate() is generalized to take any ScrollableArea.
+
+        Fix an assumption in the ScrollingTree that we only care about programmatic scrolls on the root node.
+        ScrollingTree::commitTreeState() no longer sets isHandlingProgrammaticScroll; instead,
+        callers of ScrollingTreeScrollingNode::scrollTo() pass a ScrollType. Commit functions pass
+        ScrollType::Programmatic when handling RequestedScrollPosition changes as necessary.
+
+        Programmatic scrolls need to get to the scrolling tree in the UI process so that we update
+        the tree's notion of scroll position, and trigger actual UIScrollView scrolls (layers may have
+        already been put in the right locations, but the UI process needs to know that a scroll happened).
+        However, we need to prevent notifications from programmatic scrolls getting back to the
+        web process, because this causes jumpiness. This is done via an early return in
+        RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll().
+
+        Tests: scrollingcoordinator/ios/programmatic-overflow-scroll.html
+               scrollingcoordinator/ios/programmatic-page-scroll.html
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
+        * page/scrolling/AsyncScrollingCoordinator.h:
+        * page/scrolling/ScrollingCoordinator.h:
+        (WebCore::ScrollingCoordinator::requestScrollPositionUpdate):
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::commitTreeState):
+        (WebCore::ScrollingTree::isHandlingProgrammaticScroll): Deleted.
+        * page/scrolling/ScrollingTree.h:
+        (WebCore::ScrollingTree::isHandlingProgrammaticScroll const):
+        (WebCore::ScrollingTree::setIsHandlingProgrammaticScroll):
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::scrollBy):
+        (WebCore::ScrollingTreeScrollingNode::scrollTo):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
+        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
+        (WebCore::ScrollingTreeFrameScrollingNodeMac::commitStateAfterChildren):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollToOffset):
+        (WebCore::RenderLayer::scrollingNodeID const):
+        * rendering/RenderLayer.h:
+        * rendering/RenderMarquee.cpp:
+        (WebCore::RenderMarquee::timerFired):
+
 2019-04-04  Yusuke Suzuki  <ysuzuki@apple.com>
 
         Unreviewed, speculative fix for build failure
index 90500d7..bfeef77 100644 (file)
@@ -204,25 +204,33 @@ void AsyncScrollingCoordinator::frameViewRootLayerDidChange(FrameView& frameView
     node->setHorizontalScrollbarLayer(frameView.layerForHorizontalScrollbar());
 }
 
-bool AsyncScrollingCoordinator::requestScrollPositionUpdate(FrameView& frameView, const IntPoint& scrollPosition)
+bool AsyncScrollingCoordinator::requestScrollPositionUpdate(ScrollableArea& scrollableArea, const IntPoint& scrollPosition)
 {
     ASSERT(isMainThread());
     ASSERT(m_page);
 
-    if (!coordinatesScrollingForFrameView(frameView))
+    auto scrollingNodeID = scrollableArea.scrollingNodeID();
+    if (!scrollingNodeID)
+        return false;
+
+    auto* frameView = frameViewForScrollingNode(scrollingNodeID);
+    if (!frameView)
+        return false;
+
+    if (!coordinatesScrollingForFrameView(*frameView))
         return false;
 
-    bool inPageCache = frameView.frame().document()->pageCacheState() != Document::NotInPageCache;
-    bool inProgrammaticScroll = frameView.currentScrollType() == ScrollType::Programmatic;
+    bool inPageCache = frameView->frame().document()->pageCacheState() != Document::NotInPageCache;
+    bool inProgrammaticScroll = scrollableArea.currentScrollType() == ScrollType::Programmatic;
     if (inProgrammaticScroll || inPageCache)
-        updateScrollPositionAfterAsyncScroll(frameView.scrollingNodeID(), scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set);
+        updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set);
 
     // If this frame view's document is being put into the page cache, we don't want to update our
     // main frame scroll position. Just let the FrameView think that we did.
     if (inPageCache)
         return true;
 
-    auto* stateNode = downcast<ScrollingStateScrollingNode>(m_scrollingStateTree->stateNodeForID(frameView.scrollingNodeID()));
+    auto* stateNode = downcast<ScrollingStateScrollingNode>(m_scrollingStateTree->stateNodeForID(scrollingNodeID));
     if (!stateNode)
         return false;
 
index 51c45d6..b153bac 100644 (file)
@@ -95,7 +95,7 @@ private:
     WEBCORE_EXPORT void frameViewRootLayerDidChange(FrameView&) override;
     WEBCORE_EXPORT void frameViewEventTrackingRegionsChanged(FrameView&) override;
 
-    WEBCORE_EXPORT bool requestScrollPositionUpdate(FrameView&, const IntPoint&) override;
+    WEBCORE_EXPORT bool requestScrollPositionUpdate(ScrollableArea&, const IntPoint&) override;
 
     WEBCORE_EXPORT void applyScrollingTreeLayerPositions() override;
 
index 197d04b..65c6258 100644 (file)
@@ -114,7 +114,7 @@ public:
 
     // These virtual functions are currently unique to the threaded scrolling architecture. 
     virtual void commitTreeStateIfNeeded() { }
-    virtual bool requestScrollPositionUpdate(FrameView&, const IntPoint&) { return false; }
+    virtual bool requestScrollPositionUpdate(ScrollableArea&, const IntPoint&) { return false; }
     virtual ScrollingEventResult handleWheelEvent(FrameView&, const PlatformWheelEvent&) { return ScrollingEventResult::DidNotHandleEvent; }
 
     // Create an unparented node.
index 16d7335..1d1243d 100644 (file)
@@ -166,9 +166,6 @@ void ScrollingTree::commitTreeState(std::unique_ptr<ScrollingStateTree> scrollin
             m_asyncFrameOrOverflowScrollingEnabled = scrollingStateTree->rootStateNode()->asyncFrameOrOverflowScrollingEnabled();
     }
     
-    bool scrollRequestIsProgammatic = rootNode ? rootNode->requestedScrollPositionRepresentsProgrammaticScroll() : false;
-    SetForScope<bool> changeHandlingProgrammaticScroll(m_isHandlingProgrammaticScroll, scrollRequestIsProgammatic);
-
     // unvisitedNodes starts with all nodes in the map; we remove nodes as we visit them. At the end, it's the unvisited nodes.
     // We can't use orphanNodes for this, because orphanNodes won't contain descendants of removed nodes.
     HashSet<ScrollingNodeID> unvisitedNodes;
@@ -404,11 +401,6 @@ void ScrollingTree::setCanRubberBandState(bool canRubberBandAtLeft, bool canRubb
     m_swipeState.rubberBandsAtBottom = canRubberBandAtBottom;
 }
 
-bool ScrollingTree::isHandlingProgrammaticScroll()
-{
-    return m_isHandlingProgrammaticScroll;
-}
-
 // Can be called from the main thread.
 void ScrollingTree::setScrollPinningBehavior(ScrollPinningBehavior pinning)
 {
index b4fbdaf..57cdb45 100644 (file)
@@ -118,7 +118,8 @@ public:
     // Can be called from any thread. Will update what edges allow rubber-banding.
     WEBCORE_EXPORT void setCanRubberBandState(bool canRubberBandAtLeft, bool canRubberBandAtRight, bool canRubberBandAtTop, bool canRubberBandAtBottom);
 
-    bool isHandlingProgrammaticScroll();
+    bool isHandlingProgrammaticScroll() const { return m_isHandlingProgrammaticScroll; }
+    void setIsHandlingProgrammaticScroll(bool isHandlingProgrammaticScroll) { m_isHandlingProgrammaticScroll = isHandlingProgrammaticScroll; }
     
     void setScrollPinningBehavior(ScrollPinningBehavior);
     WEBCORE_EXPORT ScrollPinningBehavior scrollPinningBehavior();
@@ -204,8 +205,8 @@ private:
     SwipeState m_swipeState;
 
     unsigned m_fixedOrStickyNodeCount { 0 };
-    bool m_scrollingPerformanceLoggingEnabled { false };
     bool m_isHandlingProgrammaticScroll { false };
+    bool m_scrollingPerformanceLoggingEnabled { false };
     bool m_asyncFrameOrOverflowScrollingEnabled { false };
 };
     
index bf4e83a..581d153 100644 (file)
@@ -152,20 +152,24 @@ FloatPoint ScrollingTreeScrollingNode::adjustedScrollPosition(const FloatPoint&
 
 void ScrollingTreeScrollingNode::scrollBy(const FloatSize& delta, ScrollPositionClamp clamp)
 {
-    scrollTo(currentScrollPosition() + delta, clamp);
+    scrollTo(currentScrollPosition() + delta, ScrollType::User, clamp);
 }
 
-void ScrollingTreeScrollingNode::scrollTo(const FloatPoint& position, ScrollPositionClamp clamp)
+void ScrollingTreeScrollingNode::scrollTo(const FloatPoint& position, ScrollType scrollType, ScrollPositionClamp clamp)
 {
     if (position == m_currentScrollPosition)
         return;
 
+    scrollingTree().setIsHandlingProgrammaticScroll(scrollType == ScrollType::Programmatic);
+    
     m_currentScrollPosition = adjustedScrollPosition(position, clamp);
     
     LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeScrollingNode " << scrollingNodeID() << " scrollTo " << position << " (delta from last committed position " << (m_lastCommittedScrollPosition - m_currentScrollPosition) << ")");
 
     updateViewportForCurrentScrollPosition();
     currentScrollPositionChanged();
+
+    scrollingTree().setIsHandlingProgrammaticScroll(false);
 }
 
 void ScrollingTreeScrollingNode::currentScrollPositionChanged()
index 63be960..bbf166a 100644 (file)
@@ -57,7 +57,7 @@ public:
     FloatPoint lastCommittedScrollPosition() const { return m_lastCommittedScrollPosition; }
 
     // These are imperative; they adjust the scrolling layers.
-    void scrollTo(const FloatPoint&, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
+    void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
     void scrollBy(const FloatSize&, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
 
     void wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport = { });
index 77cee48..72da191 100644 (file)
@@ -137,8 +137,10 @@ void ScrollingTreeFrameScrollingNodeMac::commitStateAfterChildren(const Scrollin
     const auto& scrollingStateNode = downcast<ScrollingStateScrollingNode>(stateNode);
 
     // Update the scroll position after child nodes have been updated, because they need to have updated their constraints before any scrolling happens.
-    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
-        scrollTo(scrollingStateNode.requestedScrollPosition());
+    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition)) {
+        auto scrollType = scrollingStateNode.requestedScrollPositionRepresentsProgrammaticScroll() ? ScrollType::Programmatic : ScrollType::User;
+        scrollTo(scrollingStateNode.requestedScrollPosition(), scrollType);
+    }
 
     if (isRootNode()
         && (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ScrolledContentsLayer)
index b38d38d..06e50db 100644 (file)
@@ -2346,7 +2346,14 @@ void RenderLayer::scrollToOffset(const ScrollOffset& scrollOffset, ScrollType sc
     auto previousScrollType = currentScrollType();
     setCurrentScrollType(scrollType);
 
-    scrollToOffsetWithoutAnimation(newScrollOffset, clamping);
+    bool handled = false;
+#if ENABLE(ASYNC_SCROLLING)
+    if (ScrollingCoordinator* scrollingCoordinator = page().scrollingCoordinator())
+        handled = scrollingCoordinator->requestScrollPositionUpdate(*this, scrollPositionFromOffset(scrollOffset));
+#endif
+
+    if (!handled)
+        scrollToOffsetWithoutAnimation(newScrollOffset, clamping);
 
     setCurrentScrollType(previousScrollType);
 }
@@ -2804,6 +2811,14 @@ int RenderLayer::scrollOffset(ScrollbarOrientation orientation) const
     return 0;
 }
 
+ScrollingNodeID RenderLayer::scrollingNodeID() const
+{
+    if (!isComposited())
+        return 0;
+
+    return backing()->scrollingNodeIDForRole(ScrollCoordinationRole::Scrolling);
+}
+
 IntRect RenderLayer::visibleContentRectInternal(VisibleContentRectIncludesScrollbars scrollbarInclusion, VisibleContentRectBehavior) const
 {
     IntSize scrollbarSpace;
index 782a965..8cb856e 100644 (file)
@@ -420,8 +420,8 @@ public:
     void scrollToYPosition(int y, ScrollType, ScrollClamping = ScrollClamping::Clamped);
 
     // These are only used by marquee.
-    void scrollToXOffset(int x, ScrollClamping clamping = ScrollClamping::Clamped) { scrollToOffset(ScrollOffset(x, scrollOffset().y()), ScrollType::Programmatic, clamping); }
-    void scrollToYOffset(int y, ScrollClamping clamping = ScrollClamping::Clamped) { scrollToOffset(ScrollOffset(scrollOffset().x(), y), ScrollType::Programmatic, clamping); }
+    void scrollToXOffset(int x) { scrollToOffset(ScrollOffset(x, scrollOffset().y()), ScrollType::Programmatic, ScrollClamping::Unclamped); }
+    void scrollToYOffset(int y) { scrollToOffset(ScrollOffset(scrollOffset().x(), y), ScrollType::Programmatic, ScrollClamping::Unclamped); }
 
     void setPostLayoutScrollPosition(Optional<ScrollPosition>);
     void applyPostLayoutScrollPositionIfNeeded();
@@ -1050,6 +1050,7 @@ private:
     IntPoint convertFromContainingViewToScrollbar(const Scrollbar&, const IntPoint&) const override;
     int scrollSize(ScrollbarOrientation) const override;
     void setScrollOffset(const ScrollOffset&) override;
+    ScrollingNodeID scrollingNodeID() const override;
 
     IntRect visibleContentRectInternal(VisibleContentRectIncludesScrollbars, VisibleContentRectBehavior) const override;
     IntSize overhangAmount() const override;
index 9884169..76e23e4 100644 (file)
@@ -247,9 +247,9 @@ void RenderMarquee::timerFired()
     if (m_reset) {
         m_reset = false;
         if (isHorizontal())
-            m_layer->scrollToXOffset(m_start, ScrollClamping::Unclamped);
+            m_layer->scrollToXOffset(m_start);
         else
-            m_layer->scrollToYOffset(m_start, ScrollClamping::Unclamped);
+            m_layer->scrollToYOffset(m_start);
         return;
     }
     
@@ -289,9 +289,9 @@ void RenderMarquee::timerFired()
     }
     
     if (isHorizontal())
-        m_layer->scrollToXOffset(newPos, ScrollClamping::Unclamped);
+        m_layer->scrollToXOffset(newPos);
     else
-        m_layer->scrollToYOffset(newPos, ScrollClamping::Unclamped);
+        m_layer->scrollToYOffset(newPos);
 }
 
 } // namespace WebCore
index 970a811..e3cf044 100644 (file)
@@ -1,3 +1,39 @@
+2019-04-04  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in bad rendering
+        https://bugs.webkit.org/show_bug.cgi?id=195584
+
+        Reviewed by Zalan Bujtas.
+
+        Push data to the scrolling tree about whether an overflow:scroll scroll was programmatic, by having
+        RenderLayer::scrollToOffset() call into AsyncScrollingCoordinator::requestScrollPositionUpdate(),
+        just as we do for frames.
+
+        AsyncScrollingCoordinator::requestScrollPositionUpdate() is generalized to take any ScrollableArea.
+
+        Fix an assumption in the ScrollingTree that we only care about programmatic scrolls on the root node.
+        ScrollingTree::commitTreeState() no longer sets isHandlingProgrammaticScroll; instead,
+        callers of ScrollingTreeScrollingNode::scrollTo() pass a ScrollType. Commit functions pass
+        ScrollType::Programmatic when handling RequestedScrollPosition changes as necessary.
+
+        Programmatic scrolls need to get to the scrolling tree in the UI process so that we update
+        the tree's notion of scroll position, and trigger actual UIScrollView scrolls (layers may have
+        already been put in the right locations, but the UI process needs to know that a scroll happened).
+        However, we need to prevent notifications from programmatic scrolls getting back to the
+        web process, because this causes jumpiness. This is done via an early return in
+        RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll().
+
+        * UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
+        (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll):
+        * UIProcess/RemoteLayerTree/ios/ScrollingTreeFrameScrollingNodeRemoteIOS.mm:
+        (WebKit::ScrollingTreeFrameScrollingNodeRemoteIOS::commitStateAfterChildren): Subframe nodes have
+        a delegate, and that will take care of the requestedScrollPosition update.
+        * UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.h:
+        * UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:
+        (WebKit::ScrollingTreeOverflowScrollingNodeIOS::commitStateAfterChildren):
+        * UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:
+        (WebKit::ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren):
+
 2019-04-04  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r243888.
index 6441850..1ffc7b0 100644 (file)
@@ -216,6 +216,10 @@ void RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll(ScrollingNodeID
 #if PLATFORM(IOS_FAMILY)
     m_webPageProxy.scrollingNodeScrollViewDidScroll();
 #endif
+
+    if (m_scrollingTree->isHandlingProgrammaticScroll())
+        return;
+
     m_webPageProxy.send(Messages::RemoteScrollingCoordinator::ScrollPositionChangedForNode(scrolledNodeID, newScrollPosition, scrollingLayerPositionAction == ScrollingLayerPositionAction::Sync));
 }
 
index c626c15..1136fc9 100644 (file)
@@ -82,12 +82,16 @@ void ScrollingTreeFrameScrollingNodeRemoteIOS::commitStateAfterChildren(const Sc
 
     const auto& scrollingStateNode = downcast<ScrollingStateFrameScrollingNode>(stateNode);
 
-    // Update the scroll position after child nodes have been updated, because they need to have updated their constraints before any scrolling happens.
-    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
-        scrollTo(scrollingStateNode.requestedScrollPosition());
+    if (m_scrollingNodeDelegate) {
+        m_scrollingNodeDelegate->commitStateAfterChildren(scrollingStateNode);
+        return;
+    }
 
-    if (m_scrollingNodeDelegate)
-        m_scrollingNodeDelegate->commitStateAfterChildren(downcast<ScrollingStateScrollingNode>(stateNode));
+    // Update the scroll position after child nodes have been updated, because they need to have updated their constraints before any scrolling happens.
+    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition)) {
+        auto scrollType = scrollingStateNode.requestedScrollPositionRepresentsProgrammaticScroll() ? ScrollType::Programmatic : ScrollType::User;
+        scrollTo(scrollingStateNode.requestedScrollPosition(), scrollType);
+    }
 }
 
 FloatPoint ScrollingTreeFrameScrollingNodeRemoteIOS::minimumScrollPosition() const
index 01734d1..5b986d2 100644 (file)
@@ -46,6 +46,7 @@ private:
     
     void repositionScrollingLayers() override;
 
+    // The delegate is non-null for subframes.
     std::unique_ptr<ScrollingTreeScrollingNodeDelegateIOS> m_scrollingNodeDelegate;
 };
 
index cf4f9ea..18fe088 100644 (file)
@@ -63,7 +63,9 @@ void ScrollingTreeOverflowScrollingNodeIOS::commitStateBeforeChildren(const WebC
 void ScrollingTreeOverflowScrollingNodeIOS::commitStateAfterChildren(const ScrollingStateNode& stateNode)
 {
     ScrollingTreeOverflowScrollingNode::commitStateAfterChildren(stateNode);
-    m_scrollingNodeDelegate->commitStateAfterChildren(downcast<ScrollingStateScrollingNode>(stateNode));
+
+    const auto& scrollingStateNode = downcast<ScrollingStateScrollingNode>(stateNode);
+    m_scrollingNodeDelegate->commitStateAfterChildren(scrollingStateNode);
 }
 
 void ScrollingTreeOverflowScrollingNodeIOS::repositionScrollingLayers()
index 41e0b3a..39e475c 100644 (file)
@@ -225,6 +225,7 @@ void ScrollingTreeScrollingNodeDelegateIOS::commitStateBeforeChildren(const Scro
 void ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren(const ScrollingStateScrollingNode& scrollingStateNode)
 {
     SetForScope<bool> updatingChange(m_updatingFromStateNode, true);
+
     if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ScrollContainerLayer)
         || scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::TotalContentsSize)
         || scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ReachableContentsSize)
@@ -287,6 +288,11 @@ void ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren(const Scrol
 
         END_BLOCK_OBJC_EXCEPTIONS
     }
+
+    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition)) {
+        auto scrollType = scrollingStateNode.requestedScrollPositionRepresentsProgrammaticScroll() ? ScrollType::Programmatic : ScrollType::User;
+        scrollingNode().scrollTo(scrollingStateNode.requestedScrollPosition(), scrollType);
+    }
 }
 
 void ScrollingTreeScrollingNodeDelegateIOS::repositionScrollingLayers()