Bug 127386 – Improve latching behavior for wheel events

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

When Asynchronous scrolling is used, the following can happen:

1. Display a web page that includes an iFrame or other scrollable region embedded in the main page.
2. Place the mouse above the embedded scrollable view.
3. Perform a quick swipe or two-finger-scroll to start a page scroll.

The page begins to scroll, but as the embedded section hits the mouse pointer, the page stops scrolling and the overflow section begins scrolling.

The expected behavior is for the momentum phase of the scroll to continue scrolling the page.

This is happening because the embedded region is consuming the scroll events due to improper interaction between the external fast-scrolling region and the embedded section.

[Brent Fulgham]

<rdar://problem/12176858>

[Brent Fulgham]

Created attachment 221813 [details]
WIP: Initial proposal to address the problem

[WebKit Commit Bot]

Attachment 221813 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 221813 [details]
WIP: Initial proposal to address the problem

Attachment 221813 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6420646334562304

New failing tests:
fast/repaint/overflow-scroll-in-overflow-scroll-scrolled.html
scrollbars/scroll-rtl-or-bt-layer.html
fast/frames/flattening/scrolling-in-object.html
fast/events/remove-child-onscroll.html
fast/events/wheelevent-mousewheel-interaction.html
fast/forms/search/search-scroll-hidden-decoration-container-crash.html
fast/events/platform-wheelevent-in-scrolling-div.html
fast/repaint/overflow-auto-in-overflow-auto-scrolled.html
fast/repaint/table-overflow-auto-in-overflow-auto-scrolled.html
fast/events/wheelevent-in-text-node.html
fast/events/wheelevent-basic.html
fast/events/platform-wheelevent-with-delta-zero-crash.html
fast/repaint/table-overflow-scroll-in-overflow-scroll-scrolled.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html

[Build Bot]

Created attachment 221820 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Brent Fulgham]

Created attachment 222151 [details]
Patch

[WebKit Commit Bot]

Attachment 222151 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 222165 [details]
Completed patch. Passes all tests.

[Simon Fraser (smfr)]

Comment on attachment 222165 [details]
Completed patch. Passes all tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=222165&action=review

> Source/WebCore/page/scrolling/ScrollingTree.cpp:74
> +bool ScrollingTree::shouldHandleWheelEventSynchronously(const PlatformWheelEvent& wheelEvent)

Maybe add a comment saying that this is called from the event handling thread.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:314
> +    MutexLocker locker(m_swipeStateMutex);

These should use m_mutex

> Source/WebCore/page/scrolling/ScrollingTree.h:100
> +    ScrollingNodeID latchTarget();

latchedNode() const

> Source/WebCore/page/scrolling/ScrollingTree.h:102
> +    void clearLatchTarget();

clearLatchedNode()

> Source/WebCore/page/scrolling/ScrollingTree.h:104
> +    bool doesHaveLatchTarget() const { return m_latchTargetWasSet; }

hasLatchedNode() const

> Source/WebCore/page/scrolling/ScrollingTree.h:139
> +    ScrollingNodeID m_latchedWheelEventTarget;
> +    bool m_latchTargetWasSet;

No need for the bool. 0 is an invalid ScrollingNodeID.

Let's call m_latchedWheelEventTarget m_latchedNode

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:137
> +static bool shouldConsiderEndingLatching(const PlatformWheelEvent& wheelEvent)

We don't just consider ending latching, we always end latching. So eventShouldClearLatchedNode()

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:142
> +    if (PlatformWheelEventPhaseNone == wheelPhase && PlatformWheelEventPhaseEnded == momentumPhase)

Please reverse the checks (wheelPhase == PlatformWheelEventPhaseNone etc).

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:158
> +    if (shouldConsiderLatching(wheelEvent) && !scrollingTree().mouseIsOverNonFastScrollableRegion(wheelEvent))

If we got into this code, we're either already latched and in the non-fast scrollable region, or outside of it and latched or not latched. So I don't get this logic.

I think we should also only start latching if we actually manage to scroll something here (i.e. we're not already pinned at the bottom).

[Brent Fulgham]

Comment on attachment 222165 [details]
Completed patch. Passes all tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=222165&action=review

>> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:158
>> +    if (shouldConsiderLatching(wheelEvent) && !scrollingTree().mouseIsOverNonFastScrollableRegion(wheelEvent))
> 
> If we got into this code, we're either already latched and in the non-fast scrollable region, or outside of it and latched or not latched. So I don't get this logic.
> 
> I think we should also only start latching if we actually manage to scroll something here (i.e. we're not already pinned at the bottom).

Without the check of the non-fast scrollable region, the logic always enables latched state for the root node when swiping. We pass through this code before we decide to hand off to the Iframe, so we always end up bypassing the non-fast-scroll checks.

[Brent Fulgham]

Created attachment 222174 [details]
Patch

[Brent Fulgham]

Created attachment 222180 [details]
Landed Version

[Brent Fulgham]

Comment on attachment 222180 [details]
Landed Version

I don't really think we need all this locking for retrieving single-word values.

[Brent Fulgham]

Comment on attachment 222180 [details]
Landed Version

I'm not sure that we need all these mutexes!

[Simon Fraser (smfr)]

Comment on attachment 222180 [details]
Landed Version

View in context: https://bugs.webkit.org/attachment.cgi?id=222180&action=review

> Source/WebCore/page/scrolling/ScrollingTree.cpp:317
> +    MutexLocker locker(m_mutex);
> +
> +    m_latchedNode = node;

Extra blank line.

[Brent Fulgham]

Committed r162755: <http://trac.webkit.org/changeset/162755>

[Brent Fulgham]

New testing to cover scrolling/latching behavior will be added in Bug 127606.

[Brent Fulgham]

Something is wrong. When I tried the nightly with this change, it did not work properly. It prevents the user from clicking into the iFrame at all; you are forever latched to the root node.

The patch "Completed patch. Passes all tests." works properly, but the latest incarnation does not. I must have compiled the new patch but continued to run my original version.

[Brent Fulgham]

Created attachment 222244 [details]
Patch to get correct behavior

[Brent Fulgham]

The patch "Landed Version" does not work properly, while the patch "Completed patch, Passes all tests" does.

I think we must teach ScrollingTreeScrollingNodeMac::handleWheelEvent to clear the latched node when mouse movement (and momentum) stops. If we don't, ScrollingTree::shouldHandleWheelEventSynchronously always short-circuits the test of whether the mouse is over a non-fast-scrollable region, and the event is never relayed on to the main thread.

I think we also need to check mouse position when deciding whether to set the latched node. Consider starting a drag event while over the Iframe. We always pass through ScrollingTreeScrollingNodeMac::handleWheelEvent before drilling down to the level of the iFrame. Because the event node isn't currently considering the mouse position (just the mouse event state), it sees no reason not to assign the scrolling tree's root node as the latched node. This prevents us from even latching onto the Iframe as a scrollable region.

I think my original patch was actually right. I've added another patch "Patch to get correct behavior" that puts the "clear latched node" logic back in, and also adds the mouse position test when deciding whether to assign the root node as the latched node.

[Brent Fulgham]

Comment on attachment 222244 [details]
Patch to get correct behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=222244&action=review

> Source/WebCore/page/scrolling/ScrollingTree.cpp:62
> +{

I think this logic may need to be protected by a mutex.

[Brent Fulgham]

Created attachment 222362 [details]
Patch

[Brent Fulgham]

Created attachment 222364 [details]
Patch

[Tim Horton]

Comment on attachment 222165 [details]
Completed patch. Passes all tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=222165&action=review

>>> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:158
>>> +    if (shouldConsiderLatching(wheelEvent) && !scrollingTree().mouseIsOverNonFastScrollableRegion(wheelEvent))
>> 
>> If we got into this code, we're either already latched and in the non-fast scrollable region, or outside of it and latched or not latched. So I don't get this logic.
>> 
>> I think we should also only start latching if we actually manage to scroll something here (i.e. we're not already pinned at the bottom).
> 
> Without the check of the non-fast scrollable region, the logic always enables latched state for the root node when swiping. We pass through this code before we decide to hand off to the Iframe, so we always end up bypassing the non-fast-scroll checks.

smfr: If we get into this code when we are *not* latched (one of the three cases you mentioned above), the root ScrollingTree node will latch to *itself*, and then later when we go to consider bouncing it to the main thread for the non-fast-scrollable region, we don't do so because the root node is already latched to itself.

[Brent Fulgham]

The additional checks are needed because we get sent an extra mouse move event due to https://bugs.webkit.org/show_bug.cgi?id=78731.

The fast scrolling code sees this, and uses it as justification to consider latching again, even though we had internally bypassed this check.

[Brent Fulgham]

The additional checks are needed because we get sent an extra mouse move event due to https://bugs.webkit.org/show_bug.cgi?id=78731.

The fast scrolling code sees this, and uses it as justification to consider latching again, even though we had internally bypassed this check.

[Brent Fulgham]

If event dispatching works as we thought it did, I still need my "clear the latch state" code, but the whole check for whether the mouse pointer is over the non-fast-scrollable region can be removed.

With the presence of the re-dispatch added by  https://bugs.webkit.org/show_bug.cgi?id=78731., the ScrollingTreeScrollingNode receives an "older" scroll event that looks like the start of a new swipe gesture. This triggers all the latch logic, which sees a valid starting latch event and assigns it to the root node. Under expected operations, this event should never have gotten here.

We need to figure out if we can avoid this re-dispatch, or somehow label the re-dispatch so we can ignore the event for the purpose of assigning latch state.

[Brent Fulgham]

We do a bunch of work to decide where the wheel event should be dispatched, only to be undone by FrameView deciding to override all of that and speak directly to the ScrollingTree.

Maybe we could filter the direct scrollingCoordinator->handleWheelEvent dispatch the same way we do the rest of the wheel events.

E.g., in FrameView::wheelEvent, check if the event should be handled synchronously and avoid dispatching to the scrolling coordinator/scrolling tree if that is the case.

[Simon Fraser (smfr)]

(In reply to comment #28)
> We do a bunch of work to decide where the wheel event should be dispatched, only to be undone by FrameView deciding to override all of that and speak directly to the ScrollingTree.

I don't think that FrameView needs to call into ScrollingTree at all. The intent was the update the scrolling tree based on the scroll that just happened on the main thread, but that update should happen via  ScrollingStateTree commit.

We should figure out if the call into the ScrollingTree from FrameView actually changes any state in the tree.

[Brent Fulgham]

(In reply to comment #29)
> (In reply to comment #28)
> > We do a bunch of work to decide where the wheel event should be dispatched, only to be undone by FrameView deciding to override all of that and speak directly to the ScrollingTree.
> 
> I don't think that FrameView needs to call into ScrollingTree at all. The intent was the update the scrolling tree based on the scroll that just happened on the main thread, but that update should happen via  ScrollingStateTree commit.
> 
> We should figure out if the call into the ScrollingTree from FrameView actually changes any state in the tree.

The call into ScrollingTree triggers us to call ScrollElasticityController::handleWheelEvent, which updates internal state regarding stretch/overflow, and also to dispatch to ScrollingCoordinator::handleWheelEventPhase, which eventually dispatches to ScrollAnimator::handleWheelEvent on the main thread.

It seems like the GUI stuff is already being handled through the normal dispatch process. I'm not sure if the ScrollElasticityController stuff needs to be explicitly invoked here.

[Brent Fulgham]

The crux of the problem after my change is that we are receiving a PlatformWheelEventMayBegin event, which should have been consumed prior to reaching this scroll tree logic.

By correcting the handling of this in ScrollAnimator::handleWheelEvent, we can avoid having to do the region check I thought we needed.

[Brent Fulgham]

Created attachment 222486 [details]
Patch

[Brent Fulgham]

Created attachment 222493 [details]
Patch (Revised to Build on WIndows/GTK/EFL)

[Simon Fraser (smfr)]

Comment on attachment 222493 [details]
Patch (Revised to Build on WIndows/GTK/EFL)

View in context: https://bugs.webkit.org/attachment.cgi?id=222493&action=review

> Source/WebCore/platform/ScrollAnimator.cpp:86
> +    if (e.phase() == PlatformWheelEventPhaseMayBegin)
> +        return true;

Please add a comment describing why this is necessary and note where the call is coming from.

[Brent Fulgham]

We no longer get the re-dispatched event from the main frame, which I was trying to deal with using the following check:

    if (wheelEvent.phase() == PlatformWheelEventPhaseMayBegin && wheelEvent.momentumPhase() == PlatformWheelEventPhaseNone)

I'd like to get rid of it, and handle the PlatformWheelEventPhaseCancelled.

This allows us to properly handle the following test:

1. Scroll in the main frame on the right-hand side of the page until we hit enter the vertical range that contains the Iframe (but we are horizontally displaced from the iFrame).
2. Slide the mouse over to the Iframe.
3. Begin a new swipe event.

We receive a PlatformWheelEventPhaseCancelled event in this case, which we should use to reset our latched state so we cleanly start scrolling from within the iFrame.

[Brent Fulgham]

Created attachment 222527 [details]
Patch

[Brent Fulgham]

Committed r162985: <http://trac.webkit.org/changeset/162985>