Changeset 246156 in webkit


Ignore:
Timestamp:
Jun 6, 2019 9:39:34 AM (5 years ago)
Author:
Antti Koivisto
Message:

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.

Source/WebCore:

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.

Source/WebKit:

  • 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:

LayoutTests:

  • scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html: Added.
  • scrollingcoordinator/ios/fixed-frame-overflow-swipe.html: Added.
Location:
trunk
Files:
2 added
9 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r246149 r246156  
     12019-06-06  Antti Koivisto  <antti@apple.com>
     2
     3        Position fixed is buggy with overflow:auto scrolling inside iframes
     4        https://bugs.webkit.org/show_bug.cgi?id=154399
     5        <rdar://problem/24742251>
     6
     7        Reviewed by Frederic Wang and Simon Fraser.
     8
     9        * scrollingcoordinator/ios/fixed-frame-overflow-swipe-expected.html: Added.
     10        * scrollingcoordinator/ios/fixed-frame-overflow-swipe.html: Added.
     11
    1122019-06-06  Antoine Quint  <graouts@apple.com>
    213
  • trunk/Source/WebCore/ChangeLog

    r246154 r246156  
     12019-06-06  Antti Koivisto  <antti@apple.com>
     2
     3        Position fixed is buggy with overflow:auto scrolling inside iframes
     4        https://bugs.webkit.org/show_bug.cgi?id=154399
     5        <rdar://problem/24742251>
     6
     7        Reviewed by Frederic Wang and Simon Fraser.
     8
     9        Test: scrollingcoordinator/ios/fixed-frame-overflow-swipe.html
     10
     11        After layer tree commit we were calling mainFrameViewportChangedViaDelegatedScrolling (even if viewport did not change)
     12        and expecting it to apply UI side scrolling deltas. However optimization prevents it from descending into subframes
     13        and we fail to update those properly.
     14
     15        In reality we only need to to apply scrolling tree positiong after commit if there has been delegated scrolling after the last
     16        one. Track this and do full update when needed.
     17
     18        * page/scrolling/ScrollingTree.cpp:
     19        (WebCore::ScrollingTree::applyLayerPositionsAfterCommit):
     20
     21        Add specific function for this. Don't do anything unless needed.
     22
     23        * page/scrolling/ScrollingTree.h:
     24        (WebCore::ScrollingTree::didScrollByDelegatedScrolling):
     25
     26        Track if there has been any delegated scrolling.
     27
     28        * page/scrolling/ScrollingTreeScrollingNode.cpp:
     29        (WebCore::ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling):
     30
     31        We can now bail out if nothing changes since we no longer rely on this for post-commit updates.
     32
    1332019-06-06  Zalan Bujtas  <zalan@apple.com>
    234
  • trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp

    r246083 r246156  
    255255}
    256256
     257void ScrollingTree::applyLayerPositionsAfterCommit()
     258{
     259    // Scrolling tree needs to make adjustments only if the UI side positions have changed.
     260    if (!m_wasScrolledByDelegatedScrollingSincePreviousCommit)
     261        return;
     262    m_wasScrolledByDelegatedScrollingSincePreviousCommit = false;
     263
     264    applyLayerPositions();
     265}
     266
    257267void ScrollingTree::applyLayerPositions()
    258268{
  • trunk/Source/WebCore/page/scrolling/ScrollingTree.h

    r246083 r246156  
    7272   
    7373    WEBCORE_EXPORT virtual void applyLayerPositions();
     74    WEBCORE_EXPORT void applyLayerPositionsAfterCommit();
    7475
    7576    virtual Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) = 0;
     
    8586
    8687    // Delegated scrolling/zooming has caused the viewport to change, so update viewport-constrained layers
    87     // (but don't cause scroll events to be fired).
    88     WEBCORE_EXPORT virtual void mainFrameViewportChangedViaDelegatedScrolling(const FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
     88    WEBCORE_EXPORT void mainFrameViewportChangedViaDelegatedScrolling(const FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
     89
     90    void didScrollByDelegatedScrolling() { m_wasScrolledByDelegatedScrollingSincePreviousCommit = true; }
    8991
    9092    void notifyRelatedNodesAfterScrollPositionChange(ScrollingTreeScrollingNode& changedNode);
     
    210212    bool m_scrollingPerformanceLoggingEnabled { false };
    211213    bool m_asyncFrameOrOverflowScrollingEnabled { false };
     214    bool m_wasScrolledByDelegatedScrollingSincePreviousCommit { false };
    212215};
    213216   
  • trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

    r245818 r246156  
    197197void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
    198198{
    199     // Even if position and overrideLayoutViewport haven't changed for this node, other nodes may have received new constraint data
    200     // via a commit, so the call to notifyRelatedNodesAfterScrollPositionChange() is necessary. We could avoid this if we knew that
    201     // no commits had happened.
    202199    bool scrollPositionChanged = !scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
     200    if (!scrollPositionChanged)
     201        return;
    203202
    204203    m_currentScrollPosition = adjustedScrollPosition(position, ScrollPositionClamp::None);
     
    208207
    209208    scrollingTree().notifyRelatedNodesAfterScrollPositionChange(*this);
    210    
    211     if (scrollPositionChanged)
    212         scrollingTree().scrollingTreeNodeDidScroll(*this);
     209    scrollingTree().scrollingTreeNodeDidScroll(*this);
     210    scrollingTree().didScrollByDelegatedScrolling();
    213211}
    214212
  • trunk/Source/WebKit/ChangeLog

    r246152 r246156  
     12019-06-06  Antti Koivisto  <antti@apple.com>
     2
     3        Position fixed is buggy with overflow:auto scrolling inside iframes
     4        https://bugs.webkit.org/show_bug.cgi?id=154399
     5        <rdar://problem/24742251>
     6
     7        Reviewed by Frederic Wang and Simon Fraser.
     8
     9        * UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
     10        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
     11
     12        Remove viewportChangedViaDelegatedScrolling call as we were just relying on its side effect of (partially) applying
     13        the scrolling tree. Instead call the new applyScrollingTreeLayerPositionsAfterCommit() unconditionally.
     14        It only does work if there are local deltas to apply.
     15
     16        Local deltas will potentially need to be applied in non-fixed cases too and it is hard to reason about the conditions.
     17
     18        * UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
     19        (WebKit::RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositionsAfterCommit):
     20        (WebKit::RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositions): Deleted.
     21        * UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h:
     22
    1232019-06-06  Michael Catanzaro  <mcatanzaro@igalia.com>
    224
  • trunk/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm

    r245796 r246156  
    218218
    219219#if ENABLE(ASYNC_SCROLLING)
    220     if (m_webPageProxy.scrollingCoordinatorProxy()->hasFixedOrSticky()) {
    221 #if PLATFORM(IOS_FAMILY)
    222         // 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.
    223         FloatRect layoutViewport = m_webPageProxy.computeCustomFixedPositionRect(m_webPageProxy.unobscuredContentRect(), m_webPageProxy.unobscuredContentRectRespectingInputViewBounds(), m_webPageProxy.customFixedPositionRect(), m_webPageProxy.displayedContentScale(), FrameView::LayoutViewportConstraint::Unconstrained);
    224         m_webPageProxy.scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy.unobscuredContentRect().location(), layoutViewport, m_webPageProxy.displayedContentScale());
    225 #else
    226         m_webPageProxy.scrollingCoordinatorProxy()->applyScrollingTreeLayerPositions();
    227 #endif
    228     }
     220    m_webPageProxy.scrollingCoordinatorProxy()->applyScrollingTreeLayerPositionsAfterCommit();
    229221
    230222    // Handle requested scroll position updates from the scrolling tree transaction after didCommitLayerTree()
  • trunk/Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp

    r245112 r246156  
    193193}
    194194
    195 void RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositions()
    196 {
    197     m_scrollingTree->applyLayerPositions();
     195void RemoteScrollingCoordinatorProxy::applyScrollingTreeLayerPositionsAfterCommit()
     196{
     197    m_scrollingTree->applyLayerPositionsAfterCommit();
    198198}
    199199
  • trunk/Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h

    r245112 r246156  
    6262    void viewportChangedViaDelegatedScrolling(const WebCore::FloatPoint& scrollPosition, const WebCore::FloatRect& layoutViewport, double scale);
    6363
    64     void applyScrollingTreeLayerPositions();
     64    void applyScrollingTreeLayerPositionsAfterCommit();
    6565
    6666    void currentSnapPointIndicesDidChange(WebCore::ScrollingNodeID, unsigned horizontal, unsigned vertical);
Note: See TracChangeset for help on using the changeset viewer.