Changeset 266292 in webkit


Ignore:
Timestamp:
Aug 28, 2020, 2:09:51 PM (5 years ago)
Author:
Simon Fraser
Message:

Vertical scrolling gets stuck when a horizontal scroller is under the mouse (google search results)
​https://bugs.webkit.org/show_bug.cgi?id=215641
<rdar://problem/67430532>

Reviewed by Tim Horton.
Source/WebCore:

There are two parts to this fix. First, findEnclosingScrollableContainer() needs
to use the same vertical-biasing delta fixup that we use in other places, to bias towards
vertical scrolling.

Second, when we've determined that the main frame should perform the scroll and dispatch
the wheel event to the scrolling thread, we used to hit-test from scratch on the scrolling
thread and and try to send the event to a scroller which we already know should not handle
it. So pass along a target ScrollingNodeID, and start the scrolling thread handling from
that node.

Test: fast/scrolling/mac/horizontal-overflow-trapping-small-deltas.html

  • page/FrameView.cpp:

(WebCore::FrameView::wheelEvent):

  • page/mac/EventHandlerMac.mm:

(WebCore::findEnclosingScrollableContainer):

  • page/scrolling/ScrollingCoordinator.h:

(WebCore::ScrollingCoordinator::handleWheelEvent):

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::handleWheelEvent):
(WebCore::ScrollingTree::handleWheelEventWithNode):

  • page/scrolling/ScrollingTree.h:
  • page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::handleWheelEventAfterMainThread):

  • page/scrolling/ThreadedScrollingTree.h:
  • page/scrolling/mac/ScrollingCoordinatorMac.h:
  • page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::handleWheelEvent):

  • page/scrolling/nicosia/ScrollingCoordinatorNicosia.cpp:

(WebCore::ScrollingCoordinatorNicosia::handleWheelEvent):

  • page/scrolling/nicosia/ScrollingCoordinatorNicosia.h:

LayoutTests:

After this change iframe-latch-small-deltas.html would time-out because WheelEventTestMonitor
would get stuck with a "content scrolling" defer region, due to begin/end wheel
events not getting both sent to the iframe. But the test was actually broken; logging shows
that it reset latching state anyway, and it's not testing the shipping configuration of
async iframe scrolling. So fix the test to use async iframe scrolling, and to avoid
rubber-banding, so that latching is not cleared due to elapsed time.

  • fast/scrolling/latching/iframe-latch-small-deltas-expected.txt:
  • fast/scrolling/latching/iframe-latch-small-deltas.html:
  • fast/scrolling/mac/horizontal-overflow-trapping-small-deltas-expected.txt: Added.
  • fast/scrolling/mac/horizontal-overflow-trapping-small-deltas.html: Added.
Location:
trunk
Files:
2 added
16 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r266280 r266292  
     12020-08-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Vertical scrolling gets stuck when a horizontal scroller is under the mouse (google search results)
     4        https://bugs.webkit.org/show_bug.cgi?id=215641
     5        <rdar://problem/67430532>
     6
     7        Reviewed by Tim Horton.
     8       
     9        After this change iframe-latch-small-deltas.html would time-out because WheelEventTestMonitor
     10        would get stuck with a "content scrolling" defer region, due to begin/end wheel
     11        events not getting both sent to the iframe. But the test was actually broken; logging shows
     12        that it reset latching state anyway, and it's not testing the shipping configuration of
     13        async iframe scrolling. So fix the test to use async iframe scrolling, and to avoid
     14        rubber-banding, so that latching is not cleared due to elapsed time.
     15
     16        * fast/scrolling/latching/iframe-latch-small-deltas-expected.txt:
     17        * fast/scrolling/latching/iframe-latch-small-deltas.html:
     18        * fast/scrolling/mac/horizontal-overflow-trapping-small-deltas-expected.txt: Added.
     19        * fast/scrolling/mac/horizontal-overflow-trapping-small-deltas.html: Added.
     20
    1212020-08-28  Antoine Quint  <graouts@webkit.org>
    222

  • trunk/LayoutTests/fast/scrolling/latching/iframe-latch-small-deltas-expected.txt

    r262686 r266292  
    88PASS window.pageYOffset is 200
    99After scroll
    10 PASS iframeTarget.contentWindow.pageYOffset is 500
     10PASS iframeTarget.contentWindow.pageYOffset is 400
    1111PASS window.pageYOffset is 200
    1212After wait
    13 PASS iframeTarget.contentWindow.pageYOffset is 500
     13PASS iframeTarget.contentWindow.pageYOffset is 400
    1414PASS window.pageYOffset is 200
    15 PASS iframeTarget.contentWindow.pageYOffset is 480
     15PASS iframeTarget.contentWindow.pageYOffset is 380
    1616PASS window.pageYOffset is 200
    1717PASS successfullyParsed is true

  • trunk/LayoutTests/fast/scrolling/latching/iframe-latch-small-deltas.html

    r262686 r266292  
    1 <!DOCTYPE html>
     1<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncFrameScrollingEnabled=true ] -->
    22<html>
    33<head>
    … …  
    3131            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none');
    3232            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'changed', 'none');
    33             eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'changed', 'none');
    3433            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
    3534            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'begin');
    36             eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'continue');
    37             eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'continue');
    38             eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'continue');
     35            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'begin');
     36            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, 'none', 'begin');
    3937            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'none', 'end');
    4038            await UIHelper.waitForScrollCompletion();
    4139
    4240            debug('After scroll');
    43             shouldBe('iframeTarget.contentWindow.pageYOffset', '500');
     41            shouldBe('iframeTarget.contentWindow.pageYOffset', '400');
    4442            shouldBe('window.pageYOffset', '200');
    4543
    … …  
    4745
    4846            debug('After wait');
    49             shouldBe('iframeTarget.contentWindow.pageYOffset', '500');
     47            shouldBe('iframeTarget.contentWindow.pageYOffset', '400');
    5048            shouldBe('window.pageYOffset', '200');
    5149
    … …  
    5755
    5856            await UIHelper.waitForScrollCompletion();
    59             shouldBe('iframeTarget.contentWindow.pageYOffset', '480');
     57            shouldBe('iframeTarget.contentWindow.pageYOffset', '380');
    6058            shouldBe('window.pageYOffset', '200');
    6159

  • trunk/LayoutTests/platform/mac-wk1/fast/scrolling/latching/iframe-latch-small-deltas-expected.txt

    r259417 r266292  
    88PASS window.pageYOffset is 200
    99After scroll
    10 FAIL iframeTarget.contentWindow.pageYOffset should be 500. Was 305.
     10FAIL iframeTarget.contentWindow.pageYOffset should be 400. Was 205.
    1111PASS window.pageYOffset is 200
    1212After wait
    13 FAIL iframeTarget.contentWindow.pageYOffset should be 500. Was 305.
     13FAIL iframeTarget.contentWindow.pageYOffset should be 400. Was 205.
    1414PASS window.pageYOffset is 200
    15 FAIL iframeTarget.contentWindow.pageYOffset should be 480. Was 300.
     15FAIL iframeTarget.contentWindow.pageYOffset should be 380. Was 200.
    1616PASS window.pageYOffset is 200
    1717PASS successfullyParsed is true

  • trunk/Source/WebCore/ChangeLog

    r266291 r266292  
     12020-08-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Vertical scrolling gets stuck when a horizontal scroller is under the mouse (google search results)
     4        https://bugs.webkit.org/show_bug.cgi?id=215641
     5        <rdar://problem/67430532>
     6
     7        Reviewed by Tim Horton.
     8
     9        There are two parts to this fix. First, findEnclosingScrollableContainer() needs
     10        to use the same vertical-biasing delta fixup that we use in other places, to bias towards
     11        vertical scrolling.
     12
     13        Second, when we've determined that the main frame should perform the scroll and dispatch
     14        the wheel event to the scrolling thread, we used to hit-test from scratch on the scrolling
     15        thread and and try to send the event to a scroller which we already know should not handle
     16        it. So pass along a target ScrollingNodeID, and start the scrolling thread handling from
     17        that node.
     18
     19        Test: fast/scrolling/mac/horizontal-overflow-trapping-small-deltas.html
     20
     21        * page/FrameView.cpp:
     22        (WebCore::FrameView::wheelEvent):
     23        * page/mac/EventHandlerMac.mm:
     24        (WebCore::findEnclosingScrollableContainer):
     25        * page/scrolling/ScrollingCoordinator.h:
     26        (WebCore::ScrollingCoordinator::handleWheelEvent):
     27        * page/scrolling/ScrollingTree.cpp:
     28        (WebCore::ScrollingTree::handleWheelEvent):
     29        (WebCore::ScrollingTree::handleWheelEventWithNode):
     30        * page/scrolling/ScrollingTree.h:
     31        * page/scrolling/ThreadedScrollingTree.cpp:
     32        (WebCore::ThreadedScrollingTree::handleWheelEventAfterMainThread):
     33        * page/scrolling/ThreadedScrollingTree.h:
     34        * page/scrolling/mac/ScrollingCoordinatorMac.h:
     35        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
     36        (WebCore::ScrollingCoordinatorMac::handleWheelEvent):
     37        * page/scrolling/nicosia/ScrollingCoordinatorNicosia.cpp:
     38        (WebCore::ScrollingCoordinatorNicosia::handleWheelEvent):
     39        * page/scrolling/nicosia/ScrollingCoordinatorNicosia.h:
     40
    1412020-08-28  Zalan Bujtas  <zalan@apple.com>
    242

  • trunk/Source/WebCore/page/FrameView.cpp

    r265623 r266292  
    51235123    if (auto scrollingCoordinator = this->scrollingCoordinator()) {
    51245124        if (scrollingCoordinator->coordinatesScrollingForFrameView(*this))
    5125             return scrollingCoordinator->handleWheelEvent(*this, wheelEvent);
     5125            return scrollingCoordinator->handleWheelEvent(*this, wheelEvent, scrollingNodeID());
    51265126    }
    51275127#endif

  • trunk/Source/WebCore/page/mac/EventHandlerMac.mm

    r266235 r266292  
    788788static ContainerNode* findEnclosingScrollableContainer(ContainerNode* node, const PlatformWheelEvent& wheelEvent)
    789789{
     790    auto biasedDelta = ScrollController::wheelDeltaBiasingTowardsVertical(wheelEvent);
     791
    790792    // Find the first node with a valid scrollable area starting with the current
    791793    // node and traversing its parents (or shadow hosts).
    … …  
    808810            return candidate;
    809811
    810         // FIXME: This needs to have the same axis bias that we use in other latching code.
    811         auto deltaX = wheelEvent.deltaX();
    812         auto deltaY = wheelEvent.deltaY();
    813         if ((deltaY > 0 && !scrollableArea->scrolledToTop()) || (deltaY < 0 && !scrollableArea->scrolledToBottom())
    814             || (deltaX > 0 && !scrollableArea->scrolledToLeft()) || (deltaX < 0 && !scrollableArea->scrolledToRight()))
     812        if ((biasedDelta.height() > 0 && !scrollableArea->scrolledToTop()) || (biasedDelta.height() < 0 && !scrollableArea->scrolledToBottom())
     813            || (biasedDelta.width() > 0 && !scrollableArea->scrolledToLeft()) || (biasedDelta.width() < 0 && !scrollableArea->scrolledToRight()))
    815814            return candidate;
    816815    }

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h

    r264203 r266292  
    128128    virtual void commitTreeStateIfNeeded() { }
    129129    virtual bool requestScrollPositionUpdate(ScrollableArea&, const IntPoint&, ScrollType = ScrollType::Programmatic, ScrollClamping = ScrollClamping::Clamped) { return false; }
    130     virtual bool handleWheelEvent(FrameView&, const PlatformWheelEvent&) { return false; }
     130    virtual bool handleWheelEvent(FrameView&, const PlatformWheelEvent&, ScrollingNodeID) { return false; }
    131131
    132132    // Create an unparented node.

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp

    r265823 r266292  
    135135        LOG_WITH_STREAM(Scrolling, stream << "ScrollingTree::handleWheelEvent found node " << (node ? node->scrollingNodeID() : 0) << " for point " << position);
    136136
    137         while (node) {
    138             if (is<ScrollingTreeScrollingNode>(*node)) {
    139                 auto& scrollingNode = downcast<ScrollingTreeScrollingNode>(*node);
    140                 auto result = scrollingNode.handleWheelEvent(wheelEvent);
    141 
    142                 if (result.wasHandled) {
    143                     m_latchingController.nodeDidHandleEvent(scrollingNode.scrollingNodeID(), wheelEvent, m_allowLatching);
    144                     m_gestureState.nodeDidHandleEvent(scrollingNode.scrollingNodeID(), wheelEvent);
    145                     return result;
    146                 }
    147 
    148                 if (result.needsMainThreadProcessing())
    149                     return result;
     137        return handleWheelEventWithNode(wheelEvent, node.get());
     138    }();
     139
     140    return result;
     141}
     142
     143WheelEventHandlingResult ScrollingTree::handleWheelEventWithNode(const PlatformWheelEvent& wheelEvent, ScrollingTreeNode* node)
     144{
     145    while (node) {
     146        if (is<ScrollingTreeScrollingNode>(*node)) {
     147            auto& scrollingNode = downcast<ScrollingTreeScrollingNode>(*node);
     148            auto result = scrollingNode.handleWheelEvent(wheelEvent);
     149
     150            if (result.wasHandled) {
     151                m_latchingController.nodeDidHandleEvent(scrollingNode.scrollingNodeID(), wheelEvent, m_allowLatching);
     152                m_gestureState.nodeDidHandleEvent(scrollingNode.scrollingNodeID(), wheelEvent);
     153                return result;
    150154            }
    151155
    152             if (is<ScrollingTreeOverflowScrollProxyNode>(*node)) {
    153                 if (auto relatedNode = nodeForID(downcast<ScrollingTreeOverflowScrollProxyNode>(*node).overflowScrollingNodeID())) {
    154                     node = relatedNode;
    155                     continue;
    156                 }
     156            if (result.needsMainThreadProcessing())
     157                return result;
     158        }
     159
     160        if (is<ScrollingTreeOverflowScrollProxyNode>(*node)) {
     161            if (auto relatedNode = nodeForID(downcast<ScrollingTreeOverflowScrollProxyNode>(*node).overflowScrollingNodeID())) {
     162                node = relatedNode;
     163                continue;
    157164            }
    158 
    159             node = node->parent();
    160165        }
    161         return WheelEventHandlingResult::unhandled();
    162     }();
    163 
    164     return result;
     166
     167        node = node->parent();
     168    }
     169    return WheelEventHandlingResult::unhandled();
    165170}
    166171

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.h

    r265823 r266292  
    213213
    214214protected:
     215    WheelEventHandlingResult handleWheelEventWithNode(const PlatformWheelEvent&, ScrollingTreeNode*);
     216
    215217    FloatPoint mainFrameScrollPosition() const;
    216218    void setMainFrameScrollPosition(FloatPoint);
    … …  
    235237    WEBCORE_EXPORT virtual OptionSet<EventListenerRegionType> eventListenerRegionTypesForPoint(FloatPoint) const;
    236238#endif
     239
    237240    virtual void receivedWheelEvent(const PlatformWheelEvent&) { }
    238241   

  • trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp

    r265743 r266292  
    6262}
    6363
    64 bool ThreadedScrollingTree::handleWheelEventAfterMainThread(const PlatformWheelEvent& wheelEvent)
     64bool ThreadedScrollingTree::handleWheelEventAfterMainThread(const PlatformWheelEvent& wheelEvent, ScrollingNodeID targetNodeID)
    6565{
    6666    SetForScope<bool> disallowLatchingScope(m_allowLatching, false);
    67     auto result = handleWheelEvent(wheelEvent);
     67
     68    RefPtr<ScrollingTreeNode> targetNode = nodeForID(targetNodeID);
     69    auto result = handleWheelEventWithNode(wheelEvent, targetNode.get());
    6870    return result.wasHandled;
    6971}

  • trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h

    r262294 r266292  
    4848    WheelEventHandlingResult handleWheelEvent(const PlatformWheelEvent&) override;
    4949
    50     bool handleWheelEventAfterMainThread(const PlatformWheelEvent&);
     50    bool handleWheelEventAfterMainThread(const PlatformWheelEvent&, ScrollingNodeID);
    5151
    5252    void invalidate() override;

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h

    r262294 r266292  
    4242
    4343    // Handle the wheel event on the scrolling thread. Returns whether the event was handled or not.
    44     bool handleWheelEvent(FrameView&, const PlatformWheelEvent&) override;
     44    bool handleWheelEvent(FrameView&, const PlatformWheelEvent&, ScrollingNodeID) override;
    4545
    4646private:

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

    r266016 r266292  
    7474}
    7575
    76 bool ScrollingCoordinatorMac::handleWheelEvent(FrameView&, const PlatformWheelEvent& wheelEvent)
     76bool ScrollingCoordinatorMac::handleWheelEvent(FrameView&, const PlatformWheelEvent& wheelEvent, ScrollingNodeID targetNode)
    7777{
    7878    ASSERT(isMainThread());
    … …  
    8888
    8989    RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
    90     ScrollingThread::dispatch([threadedScrollingTree, wheelEvent] {
    91         threadedScrollingTree->handleWheelEventAfterMainThread(wheelEvent);
     90    ScrollingThread::dispatch([threadedScrollingTree, wheelEvent, targetNode] {
     91        threadedScrollingTree->handleWheelEventAfterMainThread(wheelEvent, targetNode);
    9292    });
    9393    return true;

  • trunk/Source/WebCore/page/scrolling/nicosia/ScrollingCoordinatorNicosia.cpp

    r262294 r266292  
    7070}
    7171
    72 bool ScrollingCoordinatorNicosia::handleWheelEvent(FrameView&, const PlatformWheelEvent& wheelEvent)
     72bool ScrollingCoordinatorNicosia::handleWheelEvent(FrameView&, const PlatformWheelEvent& wheelEvent, ScrollingNodeID targetNode)
    7373{
    7474    ASSERT(isMainThread());
    … …  
    7676    ASSERT(scrollingTree());
    7777
    78     ScrollingThread::dispatch([threadedScrollingTree = makeRef(downcast<ThreadedScrollingTree>(*scrollingTree())), wheelEvent] {
    79         threadedScrollingTree->handleWheelEvent(wheelEvent);
     78    ScrollingThread::dispatch([threadedScrollingTree = makeRef(downcast<ThreadedScrollingTree>(*scrollingTree())), wheelEvent, targetNode] {
     79        threadedScrollingTree->handleWheelEventAfterMainThread(wheelEvent, targetNode);
    8080    });
    8181    return true;

  • trunk/Source/WebCore/page/scrolling/nicosia/ScrollingCoordinatorNicosia.h

    r262294 r266292  
    4545    void commitTreeStateIfNeeded() override;
    4646
    47     bool handleWheelEvent(FrameView&, const PlatformWheelEvent&) override;
     47    bool handleWheelEvent(FrameView&, const PlatformWheelEvent&, ScrollingNodeID) override;
    4848
    4949private:
Note: See TracChangeset for help on using the changeset viewer.