Bug 127386

Summary: Improve latching behavior for wheel events
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, bfulgham, buildbot, cmarcelo, commit-queue, jamesr, luiz, rniwa, sam, simon.fraser, thorton, tonikitoo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.7   
Bug Depends on: 127696    
Bug Blocks: 134569    
Attachments:
Description Flags
WIP: Initial proposal to address the problem
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Completed patch. Passes all tests.
none
Patch
none
Landed Version
none
Patch to get correct behavior
none
Patch
none
Patch
none
Patch
none
Patch (Revised to Build on WIndows/GTK/EFL)
none
Patch simon.fraser: review+

Description Brent Fulgham 2014-01-21 17:04:35 PST
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.
Comment 1 Brent Fulgham 2014-01-21 17:04:59 PST
<rdar://problem/12176858>
Comment 2 Brent Fulgham 2014-01-21 17:21:12 PST
Created attachment 221813 [details]
WIP: Initial proposal to address the problem
Comment 3 WebKit Commit Bot 2014-01-21 17:23:49 PST
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.
Comment 4 Build Bot 2014-01-21 18:50:37 PST
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
Comment 5 Build Bot 2014-01-21 18:50:39 PST
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
Comment 6 Brent Fulgham 2014-01-24 13:51:57 PST
Created attachment 222151 [details]
Patch
Comment 7 WebKit Commit Bot 2014-01-24 13:54:37 PST
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.
Comment 8 Brent Fulgham 2014-01-24 16:10:51 PST
Created attachment 222165 [details]
Completed patch. Passes all tests.
Comment 9 Simon Fraser (smfr) 2014-01-24 16:51:31 PST
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).
Comment 10 Brent Fulgham 2014-01-24 17:02:28 PST
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.
Comment 11 Brent Fulgham 2014-01-24 17:40:52 PST
Created attachment 222174 [details]
Patch
Comment 12 Brent Fulgham 2014-01-24 18:02:25 PST
Created attachment 222180 [details]
Landed Version
Comment 13 Brent Fulgham 2014-01-24 18:03:56 PST
Comment on attachment 222180 [details]
Landed Version

I don't really think we need all this locking for retrieving single-word values.
Comment 14 Brent Fulgham 2014-01-24 18:04:28 PST
Comment on attachment 222180 [details]
Landed Version

I'm not sure that we need all these mutexes!
Comment 15 Simon Fraser (smfr) 2014-01-24 18:09:06 PST
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.
Comment 16 Brent Fulgham 2014-01-24 20:29:22 PST
Committed r162755: <http://trac.webkit.org/changeset/162755>
Comment 17 Brent Fulgham 2014-01-24 20:31:44 PST
New testing to cover scrolling/latching behavior will be added in Bug 127606.
Comment 18 Brent Fulgham 2014-01-25 18:41:04 PST
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.
Comment 19 Brent Fulgham 2014-01-25 18:44:57 PST
Created attachment 222244 [details]
Patch to get correct behavior
Comment 20 Brent Fulgham 2014-01-25 19:55:45 PST
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.
Comment 21 Brent Fulgham 2014-01-25 20:01:35 PST
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.
Comment 22 Brent Fulgham 2014-01-27 14:35:13 PST
Created attachment 222362 [details]
Patch
Comment 23 Brent Fulgham 2014-01-27 14:36:44 PST
Created attachment 222364 [details]
Patch
Comment 24 Tim Horton 2014-01-27 16:24:09 PST
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.
Comment 25 Brent Fulgham 2014-01-27 17:58:24 PST
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.
Comment 26 Brent Fulgham 2014-01-27 17:58:24 PST
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.
Comment 27 Brent Fulgham 2014-01-27 18:10:57 PST
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.
Comment 28 Brent Fulgham 2014-01-28 11:37:49 PST
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.
Comment 29 Simon Fraser (smfr) 2014-01-28 11:46:08 PST
(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.
Comment 30 Brent Fulgham 2014-01-28 12:12:22 PST
(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.
Comment 31 Brent Fulgham 2014-01-28 13:45:46 PST
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.
Comment 32 Brent Fulgham 2014-01-28 14:11:30 PST
Created attachment 222486 [details]
Patch
Comment 33 Brent Fulgham 2014-01-28 14:32:04 PST
Created attachment 222493 [details]
Patch (Revised to Build on WIndows/GTK/EFL)
Comment 34 Simon Fraser (smfr) 2014-01-28 14:39:07 PST
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.
Comment 35 Brent Fulgham 2014-01-28 15:46:32 PST
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.
Comment 36 Brent Fulgham 2014-01-28 16:16:45 PST
Created attachment 222527 [details]
Patch
Comment 37 Brent Fulgham 2014-01-28 16:55:17 PST
Committed r162985: <http://trac.webkit.org/changeset/162985>