[iOS WK2] Header bar on nytimes articles lands in the wrong place after rubberbanding
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jul 2014 00:24:56 +0000 (00:24 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jul 2014 00:24:56 +0000 (00:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135221
<rdar://problem/17542454>

Reviewed by Benjamin Poulain.

The call to didCommitLayerTree() can cause one or two visible rect updates,
via changes to the UIScrollView contentSize and contentOffset. As a result, we
would notify the scrolling tree about a viewport change, but using the old
scrolling tree rather than the new one, so we could move layers around for
nodes which are about to be removed from the tree.

However, we also have to ensure that programmatic scrolls are applied after
didCommitLayerTree() has updated the view size, so have RemoteScrollingCoordinatorProxy
store data about programmatic scrolls and return them to the caller, which
can apply them after didCommitLayerTree().

* UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp: Store a pointer to a RequestedScrollInfo
for the duration of the tree update, so that we can store requested scroll info in it.
(WebKit::RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy):
(WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
(WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll):
* UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didCommitLayerTree): Give Mac a stub implementation.
* UIProcess/WebPageProxy.h: Group some editing-related functions together.
(WebKit::WebPageProxy::editorState):
(WebKit::WebPageProxy::canDelete):
(WebKit::WebPageProxy::hasSelectedRange):
(WebKit::WebPageProxy::isContentEditable):
(WebKit::WebPageProxy::maintainsInactiveSelection):
* UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree): Ordering change: update
the layer tree, then call didCommitLayerTree(), then do the viewport update, followed
by any programmatic scroll.

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

Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp
Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm

index 4eb1fadaa17f8bd890bf3cddb4288907928bb48f..92ce8a54a870724fb205486c07a1c02fbb26f940 100644 (file)
@@ -1,3 +1,41 @@
+2014-07-24  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] Header bar on nytimes articles lands in the wrong place after rubberbanding
+        https://bugs.webkit.org/show_bug.cgi?id=135221
+        <rdar://problem/17542454>
+
+        Reviewed by Benjamin Poulain.
+        
+        The call to didCommitLayerTree() can cause one or two visible rect updates,
+        via changes to the UIScrollView contentSize and contentOffset. As a result, we
+        would notify the scrolling tree about a viewport change, but using the old
+        scrolling tree rather than the new one, so we could move layers around for
+        nodes which are about to be removed from the tree.
+        
+        However, we also have to ensure that programmatic scrolls are applied after
+        didCommitLayerTree() has updated the view size, so have RemoteScrollingCoordinatorProxy
+        store data about programmatic scrolls and return them to the caller, which
+        can apply them after didCommitLayerTree().
+
+        * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp: Store a pointer to a RequestedScrollInfo
+        for the duration of the tree update, so that we can store requested scroll info in it.
+        (WebKit::RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy):
+        (WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
+        (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll):
+        * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didCommitLayerTree): Give Mac a stub implementation.
+        * UIProcess/WebPageProxy.h: Group some editing-related functions together.
+        (WebKit::WebPageProxy::editorState):
+        (WebKit::WebPageProxy::canDelete):
+        (WebKit::WebPageProxy::hasSelectedRange):
+        (WebKit::WebPageProxy::isContentEditable):
+        (WebKit::WebPageProxy::maintainsInactiveSelection):
+        * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree): Ordering change: update
+        the layer tree, then call didCommitLayerTree(), then do the viewport update, followed
+        by any programmatic scroll.
+
 2014-07-24  Peyton Randolph  <prandolph@apple.com>
 
         Rename feature flag for long-press gesture on Mac.                                                                   
 2014-07-24  Peyton Randolph  <prandolph@apple.com>
 
         Rename feature flag for long-press gesture on Mac.                                                                   
index a1ea0e04868862c35e7f8656c2fc2f8abe34883e..1dc824c1ee6bc2c2c6a1511e8f08d5480ee1de2e 100644 (file)
@@ -49,6 +49,7 @@ namespace WebKit {
 RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy(WebPageProxy& webPageProxy)
     : m_webPageProxy(webPageProxy)
     , m_scrollingTree(RemoteScrollingTree::create(*this))
 RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy(WebPageProxy& webPageProxy)
     : m_webPageProxy(webPageProxy)
     , m_scrollingTree(RemoteScrollingTree::create(*this))
+    , m_requestedScrollInfo(nullptr)
     , m_propagatesMainFrameScrolls(true)
 {
 }
     , m_propagatesMainFrameScrolls(true)
 {
 }
@@ -77,8 +78,10 @@ const RemoteLayerTreeHost* RemoteScrollingCoordinatorProxy::layerTreeHost() cons
     return &remoteDrawingArea->remoteLayerTreeHost();
 }
 
     return &remoteDrawingArea->remoteLayerTreeHost();
 }
 
-void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction)
+void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction, RequestedScrollInfo& requestedScrollInfo)
 {
 {
+    m_requestedScrollInfo = &requestedScrollInfo;
+
     OwnPtr<ScrollingStateTree> stateTree = const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree().release();
     
     const RemoteLayerTreeHost* layerTreeHost = this->layerTreeHost();
     OwnPtr<ScrollingStateTree> stateTree = const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree().release();
     
     const RemoteLayerTreeHost* layerTreeHost = this->layerTreeHost();
@@ -89,6 +92,8 @@ void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingC
 
     connectStateNodeLayers(*stateTree, *layerTreeHost);
     m_scrollingTree->commitNewTreeState(stateTree.release());
 
     connectStateNodeLayers(*stateTree, *layerTreeHost);
     m_scrollingTree->commitNewTreeState(stateTree.release());
+
+    m_requestedScrollInfo = nullptr;
 }
 
 #if !PLATFORM(IOS)
 }
 
 #if !PLATFORM(IOS)
@@ -169,8 +174,11 @@ void RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll(ScrollingNodeID
 
 void RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll(ScrollingNodeID scrolledNodeID, const FloatPoint& scrollPosition, bool representsProgrammaticScroll)
 {
 
 void RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll(ScrollingNodeID scrolledNodeID, const FloatPoint& scrollPosition, bool representsProgrammaticScroll)
 {
-    if (scrolledNodeID == rootScrollingNodeID())
-        m_webPageProxy.requestScroll(scrollPosition, representsProgrammaticScroll);
+    if (scrolledNodeID == rootScrollingNodeID() && m_requestedScrollInfo) {
+        m_requestedScrollInfo->requestsScrollPositionUpdate = true;
+        m_requestedScrollInfo->requestIsProgrammaticScroll = representsProgrammaticScroll;
+        m_requestedScrollInfo->requestedScrollPosition = scrollPosition;
+    }
 }
 
 } // namespace WebKit
 }
 
 } // namespace WebKit
index 26fc54ba2a3bad68b2223af626126edd5d1b5ade..40d283116fee4472f92c23052469c5ade794b0ab 100644 (file)
@@ -68,7 +68,12 @@ public:
 
     const RemoteLayerTreeHost* layerTreeHost() const;
 
 
     const RemoteLayerTreeHost* layerTreeHost() const;
 
-    void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&);
+    struct RequestedScrollInfo {
+        bool requestsScrollPositionUpdate { };
+        bool requestIsProgrammaticScroll { };
+        WebCore::FloatPoint requestedScrollPosition;
+    };
+    void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&, RequestedScrollInfo&);
 
     void setPropagatesMainFrameScrolls(bool propagatesMainFrameScrolls) { m_propagatesMainFrameScrolls = propagatesMainFrameScrolls; }
     bool propagatesMainFrameScrolls() const { return m_propagatesMainFrameScrolls; }
 
     void setPropagatesMainFrameScrolls(bool propagatesMainFrameScrolls) { m_propagatesMainFrameScrolls = propagatesMainFrameScrolls; }
     bool propagatesMainFrameScrolls() const { return m_propagatesMainFrameScrolls; }
@@ -86,6 +91,7 @@ private:
 
     WebPageProxy& m_webPageProxy;
     RefPtr<RemoteScrollingTree> m_scrollingTree;
 
     WebPageProxy& m_webPageProxy;
     RefPtr<RemoteScrollingTree> m_scrollingTree;
+    RequestedScrollInfo* m_requestedScrollInfo;
     bool m_propagatesMainFrameScrolls;
 };
 
     bool m_propagatesMainFrameScrolls;
 };
 
index b5af385291272b0f452ecbd2e174c298253f8493..65f7ac6f1936ee411f7f57919d19ed38a3c90adb 100644 (file)
@@ -1313,7 +1313,13 @@ void WebPageProxy::executeEditCommand(const String& commandName)
 
     m_process->send(Messages::WebPage::ExecuteEditCommand(commandName), m_pageID);
 }
 
     m_process->send(Messages::WebPage::ExecuteEditCommand(commandName), m_pageID);
 }
-    
+
+#if !PLATFORM(IOS)
+void WebPageProxy::didCommitLayerTree(const RemoteLayerTreeTransaction&)
+{
+}
+#endif
+
 #if USE(TILED_BACKING_STORE)
 void WebPageProxy::commitPageTransitionViewport()
 {
 #if USE(TILED_BACKING_STORE)
 void WebPageProxy::commitPageTransitionViewport()
 {
index fa3d008730c3ec281d166ed4b01001a9ea238d20..587bf4e4b8e6d0eb59b3380cd79ae35f09babe76 100644 (file)
@@ -377,6 +377,15 @@ public:
 
     void executeEditCommand(const String& commandName);
     void validateCommand(const String& commandName, std::function<void (const String&, bool, int32_t, CallbackBase::Error)>);
 
     void executeEditCommand(const String& commandName);
     void validateCommand(const String& commandName, std::function<void (const String&, bool, int32_t, CallbackBase::Error)>);
+
+    const EditorState& editorState() const { return m_editorState; }
+    bool canDelete() const { return hasSelectedRange() && isContentEditable(); }
+    bool hasSelectedRange() const { return m_editorState.selectionIsRange; }
+    bool isContentEditable() const { return m_editorState.isContentEditable; }
+    
+    bool maintainsInactiveSelection() const { return m_maintainsInactiveSelection; }
+    void setMaintainsInactiveSelection(bool);
+
 #if PLATFORM(IOS)
     void executeEditCommand(const String& commandName, std::function<void (CallbackBase::Error)>);
     double displayedContentScale() const { return m_lastVisibleContentRectUpdate.scale(); }
 #if PLATFORM(IOS)
     void executeEditCommand(const String& commandName, std::function<void (CallbackBase::Error)>);
     double displayedContentScale() const { return m_lastVisibleContentRectUpdate.scale(); }
@@ -402,7 +411,6 @@ public:
     void setDeviceOrientation(int32_t);
     int32_t deviceOrientation() const { return m_deviceOrientation; }
     void willCommitLayerTree(uint64_t transactionID);
     void setDeviceOrientation(int32_t);
     int32_t deviceOrientation() const { return m_deviceOrientation; }
     void willCommitLayerTree(uint64_t transactionID);
-    void didCommitLayerTree(const WebKit::RemoteLayerTreeTransaction&);
 
     void selectWithGesture(const WebCore::IntPoint, WebCore::TextGranularity, uint32_t gestureType, uint32_t gestureState, std::function<void (const WebCore::IntPoint&, uint32_t, uint32_t, uint32_t, CallbackBase::Error)>);
     void updateSelectionWithTouches(const WebCore::IntPoint, uint32_t touches, bool baseIsStart, std::function<void (const WebCore::IntPoint&, uint32_t, CallbackBase::Error)>);
 
     void selectWithGesture(const WebCore::IntPoint, WebCore::TextGranularity, uint32_t gestureType, uint32_t gestureState, std::function<void (const WebCore::IntPoint&, uint32_t, uint32_t, uint32_t, CallbackBase::Error)>);
     void updateSelectionWithTouches(const WebCore::IntPoint, uint32_t touches, bool baseIsStart, std::function<void (const WebCore::IntPoint&, uint32_t, CallbackBase::Error)>);
@@ -442,13 +450,8 @@ public:
     void contentSizeCategoryDidChange(const String& contentSizeCategory);
 #endif
 
     void contentSizeCategoryDidChange(const String& contentSizeCategory);
 #endif
 
-    const EditorState& editorState() const { return m_editorState; }
-    bool canDelete() const { return hasSelectedRange() && isContentEditable(); }
-    bool hasSelectedRange() const { return m_editorState.selectionIsRange; }
-    bool isContentEditable() const { return m_editorState.isContentEditable; }
-    
-    bool maintainsInactiveSelection() const { return m_maintainsInactiveSelection; }
-    void setMaintainsInactiveSelection(bool);
+    void didCommitLayerTree(const WebKit::RemoteLayerTreeTransaction&);
+
 #if USE(TILED_BACKING_STORE) 
     void didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect);
 #endif
 #if USE(TILED_BACKING_STORE) 
     void didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect);
 #endif
index 52362f2c11082c619cc47824fdb27cb2c9b539ed..1cc236ca33d05fae6866174464d155185dd25813 100644 (file)
@@ -200,20 +200,26 @@ void RemoteLayerTreeDrawingAreaProxy::commitLayerTree(const RemoteLayerTreeTrans
     if (m_remoteLayerTreeHost.updateLayerTree(layerTreeTransaction))
         m_webPageProxy->setAcceleratedCompositingRootLayer(m_remoteLayerTreeHost.rootLayer());
 
     if (m_remoteLayerTreeHost.updateLayerTree(layerTreeTransaction))
         m_webPageProxy->setAcceleratedCompositingRootLayer(m_remoteLayerTreeHost.rootLayer());
 
-#if PLATFORM(IOS)
-    m_webPageProxy->didCommitLayerTree(layerTreeTransaction);
+#if ENABLE(ASYNC_SCROLLING)
+    RemoteScrollingCoordinatorProxy::RequestedScrollInfo requestedScrollInfo;
+    m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction, requestedScrollInfo);
 #endif
 
 #endif
 
-#if ENABLE(ASYNC_SCROLLING)
-    m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction);
+    m_webPageProxy->didCommitLayerTree(layerTreeTransaction);
 
 
+#if ENABLE(ASYNC_SCROLLING)
 #if PLATFORM(IOS)
     if (m_webPageProxy->scrollingCoordinatorProxy()->hasFixedOrSticky()) {
 #if PLATFORM(IOS)
     if (m_webPageProxy->scrollingCoordinatorProxy()->hasFixedOrSticky()) {
-        FloatRect customFixedPositionRect = m_webPageProxy->computeCustomFixedPositionRect(m_webPageProxy->unobscuredContentRect(), m_webPageProxy->displayedContentScale());
         // If we got a new layer for a fixed or sticky node, its position from the WebProcess is probably stale. We need to re-run the "viewport" changed logic to udpate it with our UI-side state.
         // If we got a new layer for a fixed or sticky node, its position from the WebProcess is probably stale. We need to re-run the "viewport" changed logic to udpate it with our UI-side state.
+        FloatRect customFixedPositionRect = m_webPageProxy->computeCustomFixedPositionRect(m_webPageProxy->unobscuredContentRect(), m_webPageProxy->displayedContentScale());
         m_webPageProxy->scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy->scrollingCoordinatorProxy()->rootScrollingNodeID(), customFixedPositionRect, m_webPageProxy->displayedContentScale());
     }
 #endif
         m_webPageProxy->scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy->scrollingCoordinatorProxy()->rootScrollingNodeID(), customFixedPositionRect, m_webPageProxy->displayedContentScale());
     }
 #endif
+
+    // Handle requested scroll position updates from the scrolling tree transaction after didCommitLayerTree()
+    // has updated the view size based on the content size.
+    if (requestedScrollInfo.requestsScrollPositionUpdate)
+        m_webPageProxy->requestScroll(requestedScrollInfo.requestedScrollPosition, requestedScrollInfo.requestIsProgrammaticScroll);
 #endif // ENABLE(ASYNC_SCROLLING)
 
     if (m_debugIndicatorLayerTreeHost) {
 #endif // ENABLE(ASYNC_SCROLLING)
 
     if (m_debugIndicatorLayerTreeHost) {