Attachment 371500 Details for Bug 154399 – patch

[patch] patch

commit-scrollingtree-apply-7.patch (text/plain), 16.82 KB, created by Antti Koivisto on 2019-06-06 08:57:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2019-06-06 08:57:53 PDT

Size: 16.82 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 246152)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,35 @@
>+2019-06-06  Antti Koivisto  <antti@apple.com>
>+
>+        Position fixed is buggy with overflow:auto scrolling inside iframes
>+        https://bugs.webkit.org/show_bug.cgi?id=154399
>+        <rdar://problem/24742251>
>+
>+        Reviewed by Frederic Wang and Simon Fraser.
>+
>+        Test: scrollingcoordinator/ios/fixed-frame-overflow-swipe.html
>+
>+        After layer tree commit we were calling mainFrameViewportChangedViaDelegatedScrolling (even if viewport did not change)
>+        and expecting it to apply UI side scrolling deltas. However optimization prevents it from descending into subframes
>+        and we fail to update those properly.
>+
>+        In reality we only need to to apply scrolling tree positiong after commit if there has been delegated scrolling after the last
>+        one. Track this and do full update when needed.
>+
>+        * page/scrolling/ScrollingTree.cpp:
>+        (WebCore::ScrollingTree::applyLayerPositionsAfterCommit):
>+
>+        Add specific function for this. Don't do anything unless needed.
>+
>+        * page/scrolling/ScrollingTree.h:
>+        (WebCore::ScrollingTree::didScrollByDelegatedScrolling):
>+
>+        Track if there has been any delegated scrolling.
>+
>+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
>+        (WebCore::ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling):
>+
>+        We can now bail out if nothing changes since we no longer rely on this for post-commit updates.
>+
> 2019-06-05  Takashi Komori  <Takashi.Komori@sony.com>
> 
>         [Curl] Report all request headers to web inspector.
>Index: Source/WebCore/page/scrolling/ScrollingTree.cpp
>===================================================================
>--- Source/WebCore/page/scrolling/ScrollingTree.cpp	(revision 246145)
>+++ Source/WebCore/page/scrolling/ScrollingTree.cpp	(working copy)
>@@ -254,6 +254,16 @@ void ScrollingTree::updateTreeFromStateN
>     node->commitStateAfterChildren(*stateNode);
> }
> 
>+void ScrollingTree::applyLayerPositionsAfterCommit()
>+{
>+    // Scrolling tree needs to make adjustments only if the UI side positions have changed.
>+    if (!m_wasScrolledByDelegatedScrollingSincePreviousCommit)
>+        return;
>+    m_wasScrolledByDelegatedScrollingSincePreviousCommit = false;
>+
>+    applyLayerPositions();
>+}
>+
> void ScrollingTree::applyLayerPositions()
> {
>     ASSERT(isMainThread());
>Index: Source/WebCore/page/scrolling/ScrollingTree.h
>===================================================================
>--- Source/WebCore/page/scrolling/ScrollingTree.h	(revision 246145)
>+++ Source/WebCore/page/scrolling/ScrollingTree.h	(working copy)
>@@ -71,6 +71,7 @@ public:
>     WEBCORE_EXPORT virtual void commitTreeState(std::unique_ptr<ScrollingStateTree>);
>     
>     WEBCORE_EXPORT virtual void applyLayerPositions();
>+    WEBCORE_EXPORT void applyLayerPositionsAfterCommit();
> 
>     virtual Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) = 0;
>     
>@@ -84,8 +85,9 @@ public:
>     virtual void scrollingTreeNodeRequestsScroll(ScrollingNodeID, const FloatPoint& /*scrollPosition*/, bool /*representsProgrammaticScroll*/) { }
> 
>     // Delegated scrolling/zooming has caused the viewport to change, so update viewport-constrained layers
>-    // (but don't cause scroll events to be fired).
>-    WEBCORE_EXPORT virtual void mainFrameViewportChangedViaDelegatedScrolling(const FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
>+    WEBCORE_EXPORT void mainFrameViewportChangedViaDelegatedScrolling(const FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
>+
>+    void didScrollByDelegatedScrolling() { m_wasScrolledByDelegatedScrollingSincePreviousCommit = true; }
> 
>     void notifyRelatedNodesAfterScrollPositionChange(ScrollingTreeScrollingNode& changedNode);
> 
>@@ -209,6 +211,7 @@ private:
>     bool m_isHandlingProgrammaticScroll { false };
>     bool m_scrollingPerformanceLoggingEnabled { false };
>     bool m_asyncFrameOrOverflowScrollingEnabled { false };
>+    bool m_wasScrolledByDelegatedScrollingSincePreviousCommit { false };
> };
>     
> } // namespace WebCore
>Index: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
>===================================================================
>--- Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp	(revision 246145)
>+++ Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp	(working copy)
>@@ -196,10 +196,9 @@ void ScrollingTreeScrollingNode::applyLa
> 
> void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
> {
>-    // 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);
>+    if (!scrollPositionChanged)
>+        return;
> 
>     m_currentScrollPosition = adjustedScrollPosition(position, ScrollPositionClamp::None);
>     updateViewportForCurrentScrollPosition(overrideLayoutViewport);
>@@ -207,9 +206,8 @@ void ScrollingTreeScrollingNode::wasScro
>     repositionRelatedLayers();
> 
>     scrollingTree().notifyRelatedNodesAfterScrollPositionChange(*this);
>-    
>-    if (scrollPositionChanged)
>-        scrollingTree().scrollingTreeNodeDidScroll(*this);
>+    scrollingTree().scrollingTreeNodeDidScroll(*this);
>+    scrollingTree().didScrollByDelegatedScrolling();
> }
> 
> LayoutPoint ScrollingTreeScrollingNode::parentToLocalPoint(LayoutPoint point) const
>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 246152)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,25 @@
>+2019-06-06  Antti Koivisto  <antti@apple.com>
>+
>+        Position fixed is buggy with overflow:auto scrolling inside iframes
>+        https://bugs.webkit.org/show_bug.cgi?id=154399
>+        <rdar://problem/24742251>
>+
>+        Reviewed by Frederic Wang and Simon Fraser.
>+
>+        * UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
>+        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
>+
>+        Remove viewportChangedViaDelegatedScrolling call as we were just relying on its side effect of (partially) applying
>+        the scrolling tree. Instead call the new applyScrollingTreeLayerPositionsAfterCommit() unconditionally.
>+        It only does work if there are local deltas to apply.
>+
>+        Local deltas will potentially need to be applied in non-fixed cases too and it is hard to reason about the conditions.
>+
>+        * UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
>+        (WebKit::RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositionsAfterCommit):
>+        (WebKit::RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositions): Deleted.
>+        * UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h:
>+
> 2019-06-06  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [WPE][GTK] Clean up use of initiatingPageID in implementation of webkit_uri_scheme_request_get_web_view()
>Index: Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm
>===================================================================
>--- Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm	(revision 246145)
>+++ Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm	(working copy)
>@@ -217,15 +217,7 @@ void RemoteLayerTreeDrawingAreaProxy::co
>     m_webPageProxy.didCommitLayerTree(layerTreeTransaction);
> 
> #if ENABLE(ASYNC_SCROLLING)
>-    if (m_webPageProxy.scrollingCoordinatorProxy()->hasFixedOrSticky()) {
>-#if PLATFORM(IOS_FAMILY)
>-        // 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 layoutViewport = m_webPageProxy.computeCustomFixedPositionRect(m_webPageProxy.unobscuredContentRect(), m_webPageProxy.unobscuredContentRectRespectingInputViewBounds(), m_webPageProxy.customFixedPositionRect(), m_webPageProxy.displayedContentScale(), FrameView::LayoutViewportConstraint::Unconstrained);
>-        m_webPageProxy.scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy.unobscuredContentRect().location(), layoutViewport, m_webPageProxy.displayedContentScale());
>-#else
>-        m_webPageProxy.scrollingCoordinatorProxy()->applyScrollingTreeLayerPositions();
>-#endif
>-    }
>+    m_webPageProxy.scrollingCoordinatorProxy()->applyScrollingTreeLayerPositionsAfterCommit();
> 
>     // Handle requested scroll position updates from the scrolling tree transaction after didCommitLayerTree()
>     // has updated the view size based on the content size.
>Index: Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp
>===================================================================
>--- Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp	(revision 246145)
>+++ Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp	(working copy)
>@@ -192,9 +192,9 @@ void RemoteScrollingCoordinatorProxy::vi
>     m_scrollingTree->mainFrameViewportChangedViaDelegatedScrolling(scrollPosition, layoutViewport, scale);
> }
> 
>-void RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositions()
>+void RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositionsAfterCommit()
> {
>-    m_scrollingTree->applyLayerPositions();
>+    m_scrollingTree->applyLayerPositionsAfterCommit();
> }
> 
> void RemoteScrollingCoordinatorProxy::currentSnapPointIndicesDidChange(WebCore::ScrollingNodeID nodeID, unsigned horizontal, unsigned vertical)
>Index: Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h
>===================================================================
>--- Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h	(revision 246145)
>+++ Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h	(working copy)
>@@ -61,7 +61,7 @@ public:
>     // Called externally when native views move around.
>     void viewportChangedViaDelegatedScrolling(const WebCore::FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
> 
>-    void applyScrollingTreeLayerPositions();
>+    void applyScrollingTreeLayerPositionsAfterCommit();
> 
>     void currentSnapPointIndicesDidChange(WebCore::ScrollingNodeID, unsigned horizontal, unsigned vertical);
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 246145)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,14 @@
>+2019-06-06  Antti Koivisto  <antti@apple.com>
>+
>+        Position fixed is buggy with overflow:auto scrolling inside iframes
>+        https://bugs.webkit.org/show_bug.cgi?id=154399
>+        <rdar://problem/24742251>
>+
>+        Reviewed by Frederic Wang and Simon Fraser.
>+
>+        * scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html: Added.
>+        * scrollingcoordinator/ios/fixed-frame-overflow-swipe.html: Added.
>+
> 2019-06-05  Takashi Komori  <Takashi.Komori@sony.com>
> 
>         [Curl] Report all request headers to web inspector.
>Index: LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html
>===================================================================
>--- LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html	(nonexistent)
>+++ LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html	(working copy)
>@@ -0,0 +1,86 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+    <meta name='viewport' content='initial-scale=1.0'>
>+    <style>
>+        .scroller {
>+            margin: 10px;
>+            height: 300px;
>+            width: 300px;
>+            border: 1px solid black;
>+            overflow: scroll;
>+            z-index: 0;
>+            position: relative;
>+        }
>+        
>+        .fixed {
>+            position: fixed;
>+            top: 0px;
>+            width: 200px;
>+            height: 200px;
>+            background-color: green;
>+        }
>+    </style>
>+    <script src='../../resources/ui-helper.js'></script>
>+    <script>
>+        if (window.testRunner)
>+            testRunner.waitUntilDone();
>+
>+        async function doTest()
>+        {
>+            if (!window.testRunner)
>+                return;
>+
>+            if (!testRunner.runUIScript)
>+                return;
>+
>+            await UIHelper.ensurePresentationUpdate();
>+            frame.contentDocument.querySelector('.scroller').scrollTo(0, 200);
>+            await UIHelper.ensurePresentationUpdate();
>+
>+            if (window.testRunner)
>+                testRunner.notifyDone();
>+        }
>+        
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+</head>
>+<body>
>+    <iframe id='frame' width=300 height=400 srcdoc="<!DOCTYPE html>
>+        <html>
>+        <head>
>+            <style>
>+                .scroller {
>+                    margin: 10px;
>+                    height: 300px;
>+                    width: 300px;
>+                    border: 1px solid black;
>+                    overflow: scroll;
>+                    z-index: 0;
>+                    position: relative;
>+                }
>+
>+                .fixed {
>+                    position: fixed;
>+                    bottom: 0px;
>+                    right: 0px;
>+                    width: 200px;
>+                    height: 200px;
>+                    background-color: green;
>+                }
>+                .spacer {
>+                    height: 1000px;
>+                    border: 2px solid blue;
>+                }
>+            </style>
>+        </head>
>+        <body>
>+            <div class='scroller'>
>+                <div class='fixed'></div>
>+                <div class='spacer'></div>
>+            </div>
>+        </body>
>+        </html>"
>+    ></iframe>
>+</body>
>+</html>
>Index: LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe.html
>===================================================================
>--- LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe.html	(nonexistent)
>+++ LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe.html	(working copy)
>@@ -0,0 +1,84 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+    <meta name='viewport' content='initial-scale=1.0'>
>+    <style>
>+        .scroller {
>+            margin: 10px;
>+            height: 300px;
>+            width: 300px;
>+            border: 1px solid black;
>+            overflow: scroll;
>+            z-index: 0;
>+            position: relative;
>+        }
>+        
>+        .fixed {
>+            position: fixed;
>+            top: 0px;
>+            width: 200px;
>+            height: 200px;
>+            background-color: green;
>+        }
>+    </style>
>+    <script src='../../resources/basic-gestures.js'></script>
>+    <script>
>+        if (window.testRunner)
>+            testRunner.waitUntilDone();
>+
>+        async function doTest()
>+        {
>+            if (!window.testRunner)
>+                return;
>+
>+            if (!testRunner.runUIScript)
>+                return;
>+
>+            await touchAndDragFromPointToPoint(50, 200, 50, 50);
>+            await liftUpAtPoint(50, 200);
>+
>+            testRunner.notifyDone();
>+        }
>+        
>+        window.addEventListener('load', doTest, false);
>+    </script>
>+</head>
>+<body>
>+    <iframe width=300 height=400 srcdoc="<!DOCTYPE html>
>+        <html>
>+        <head>
>+            <style>
>+                .scroller {
>+                    margin: 10px;
>+                    height: 300px;
>+                    width: 300px;
>+                    border: 1px solid black;
>+                    overflow: scroll;
>+                    z-index: 0;
>+                    position: relative;
>+                }
>+
>+                .fixed {
>+                    position: fixed;
>+                    bottom: 0px;
>+                    right: 0px;
>+                    width: 200px;
>+                    height: 200px;
>+                    background-color: green;
>+                }
>+                .spacer {
>+                    height: 1000px;
>+                    border: 2px solid blue;
>+                }
>+            </style>
>+        </head>
>+        <body>
>+            <div class='scroller'>
>+                <div class='fixed'></div>
>+                <div class='spacer'></div>
>+            </div>
>+        </body>
>+        </html>"
>+    ></iframe>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 154399: 311845 | 311846 | 316469 | 316551 | 316959 | 371495 | 371497 | 371499 | 371500