EventHandler::handleGestureScrollUpdate() should invoke the user-generated scroll routines so its behavior matches other user-initiated scrolls

• Component: UI Events
• Status: RESOLVED FIXED

[Terry Anderson]

Based on the discussion in https://bugs.webkit.org/show_bug.cgi?id=109316#c13, handling a gesture scroll should not use the code path in RenderLayer::scrollBy() / RenderLayer::scrollByRecursively() to do scrolling.

[Terry Anderson]

Created attachment 188219 [details]
Early WIP, not for review

[Terry Anderson]

Comment on attachment 188219 [details]
Early WIP, not for review

As is, this patch appears to work correctly (even with iframes) except that flings still bubble. The functions that have been added have a lot of code copied from their wheel counterparts, so this will need a refactoring.

[WebKit Review Bot]

Comment on attachment 188219 [details]
Early WIP, not for review

Attachment 188219 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16546412

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-iframe-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-page-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-twice-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-propagated.html

[WebKit Review Bot]

Comment on attachment 188219 [details]
Early WIP, not for review

Attachment 188219 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16537401

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-iframe-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-page-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-twice-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-propagated.html

[Terry Anderson]

Created attachment 188663 [details]
Patch

[Darin Adler]

Comment on attachment 188663 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        EventHandler::handleGestureScrollUpdate() should not use RenderLayer::scrollBy() to perform scrolling

Patches need comments that answer the question “Why?”.

The above just describes the code change without explaining what the benefit is. And since there are no test cases along with this, it’s just a “trust me, this needs to be changed like this” patch.

[James Robinson]

A better description of this bug would be something along the lines of "EventHandler::handleGestureScrollUpdate should invoke the user-generated scroll routines, not the programmatic routines, so behavior matches other user-initiated scrolls".  That's maybe a bit too wordy for the bug title, however.

[Terry Anderson]

Comment on attachment 188663 [details]
Patch

WIP, not for review.

This patch appears to work fine for propagated gesture scrolls (including iframes). Fling gestures do not propagate; I was able to very nicely hook into the existing logic in RenderBox::scroll() by passing in |stopNode|, which eliminates the need for using the ScrollPropagation enum.

I have also reverted the changes I made to RenderLayer::scrollByRecursively() in http://trac.webkit.org/changeset/140177 and http://trac.webkit.org/changeset/142195 since this function will no longer be used to gesture-scroll.

Left to do:
- Fix the layout tests. The propagation-specific ones are erroneously failing. Manually running these tests verifies that they work.

Left to do (possibly in a follow-up patch):
- Refactoring to eliminate the more-or-less copied and pasted code in FrameView::gestureScrollUpdateEvent(), ScrollAnimator::handleGestureScrollUpdateEvent(), and ScrollableArea::handleGestureScrollUpdateEvent().

[Terry Anderson]

(In reply to comment #6)
> (From update of attachment 188663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188663&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        EventHandler::handleGestureScrollUpdate() should not use RenderLayer::scrollBy() to perform scrolling
> 
> Patches need comments that answer the question “Why?”.
> 
> The above just describes the code change without explaining what the benefit is. And since there are no test cases along with this, it’s just a “trust me, this needs to be changed like this” patch.

My mistake, I forgot the --no-review when uploading. This is still a WIP patch. Once this goes up for review, I will be adding test coverage and a thorough description of my change in the ChangeLog.

I will change the bug name to something better now though, as suggested by James.

[WebKit Review Bot]

Comment on attachment 188663 [details]
Patch

Attachment 188663 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16592346

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-iframe-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-page-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-twice-propagated.html
fast/events/touch/gesture/touch-gesture-scroll-div-propagated.html

[Terry Anderson]

Created attachment 190150 [details]
Patch

[Terry Anderson]

(In reply to comment #11)
> Created an attachment (id=190150) [details]
> Patch

Not for official review, but I would appreciate some informal feedback. As is, the patch works and I believe that I have sufficient layout test coverage. But I recognize that at the moment this patch adds more-or-less copied/pasted code in FrameView::gestureScrollUpdateEvent(), ScrollAnimator::handleGestureScrollUpdateEvent(), and ScrollableArea::handleGestureScrollUpdateEvent(). These are copied from their mousewheel counterparts FrameView::wheelEvent(), ScrollAnimator::handleWheelEvent(), and ScrollableArea::handleWheelEvent(). 

The functions differ mainly in the type of their parameters (PlatformWheelEvent vs. PlatformGestureEvent). Perhaps the best way to avoid this code duplication would be to create a more general PlatformScrollEvent, which can store the source of the scroll (touch or mousewheel) as an extra attribute.

[Terry Anderson]

Created attachment 190372 [details]
Patch for review

[Eric Seidel (no email)]

James is probably your best reviewer, but I can also review if he's busy.

[Robert Kroeger]

Comment on attachment 190372 [details]
Patch for review

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

> Source/WebCore/ChangeLog:3
> +        EventHandler::handleGestureScrollUpdate() should invoke the user-generated scroll routines so its behavior matches other user-initiated scrolls

I'd line break this.

> Source/WebCore/ChangeLog:9
> +        the scrolling behavior of mousewheel events, use the existing user-generated scroll logic

might be worth emphasizing that this CL uses none of the wheel event event handling code.

> Source/WebCore/page/EventHandler.cpp:2700
> +    PlatformWheelEvent syntheticWheelEvent(point, globalPoint,

I think that this should have preciseScrollingDeltas set to true on it.

> Source/WebCore/rendering/RenderLayer.cpp:2024
>  void RenderLayer::scrollByRecursively(const IntSize& delta, ScrollOffsetClamping clamp)

you don't actually call this function in the new patch right? If so, you could leave this out of this CL and revert in a subsequent patch for a smaller CL.

[James Robinson]

Comment on attachment 190372 [details]
Patch for review

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

Could you explain in a bit more detail how the lifetime of the previous gesture scrolled node is handled?  Is there any reason we need to keep this alive past the end of a gesture scroll sequence (i.e. past the GestureScrollEnd event)?

What platforms have you tested this patch on?

> Source/WebCore/page/EventHandler.cpp:2680
> +    if (scrollShouldNotPropagate)
> +        m_previousGestureScrolledNode = stopNode;

When is this reference cleared?

> Source/WebCore/page/EventHandler.cpp:2697
> +    const float tickDivisor = (float)WheelEvent::TickMultiplier;

We don't use c-style casts in WebKit.  This should probably be a static_cast<> if it needs to be explicit or just implicit.

[Terry Anderson]

Created attachment 190592 [details]
Patch

[Terry Anderson]

Comment on attachment 190372 [details]
Patch for review

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

>> Source/WebCore/ChangeLog:3
>> +        EventHandler::handleGestureScrollUpdate() should invoke the user-generated scroll routines so its behavior matches other user-initiated scrolls
> 
> I'd line break this.

Done.

>> Source/WebCore/ChangeLog:9
>> +        the scrolling behavior of mousewheel events, use the existing user-generated scroll logic
> 
> might be worth emphasizing that this CL uses none of the wheel event event handling code.

Done.

>> Source/WebCore/page/EventHandler.cpp:2680
>> +        m_previousGestureScrolledNode = stopNode;
> 
> When is this reference cleared?

I clear |m_previousGestureScrolledNode| in EventHandler::handleGestureScrollBegin().

>> Source/WebCore/page/EventHandler.cpp:2697
>> +    const float tickDivisor = (float)WheelEvent::TickMultiplier;
> 
> We don't use c-style casts in WebKit.  This should probably be a static_cast<> if it needs to be explicit or just implicit.

Changed to a static_cast<float>.

>> Source/WebCore/page/EventHandler.cpp:2700
>> +    PlatformWheelEvent syntheticWheelEvent(point, globalPoint,
> 
> I think that this should have preciseScrollingDeltas set to true on it.

Done. I had to add the mutator setHasPreciseScrollingDeltas() in PlatformWheelEvent.

>> Source/WebCore/rendering/RenderLayer.cpp:2024
>>  void RenderLayer::scrollByRecursively(const IntSize& delta, ScrollOffsetClamping clamp)
> 
> you don't actually call this function in the new patch right? If so, you could leave this out of this CL and revert in a subsequent patch for a smaller CL.

Agreed. I have filed https://bugs.webkit.org/show_bug.cgi?id=111007 to track the removal of the unused code in RenderLayer.

[Terry Anderson]

(In reply to comment #16)
> (From update of attachment 190372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190372&action=review
> 
> Could you explain in a bit more detail how the lifetime of the previous gesture scrolled node is handled?  Is there any reason we need to keep this alive past the end of a gesture scroll sequence (i.e. past the GestureScrollEnd event)?

The member |m_previousGestureScrolledNode| is cleared on every GestureScrollBegin event. A fling gesture will not send a GestureScrollEnd event, which is why I opted to clear the value on a GestureScrollBegin instead. 

If a GestureScrollEnd event is sent (i.e., when the user pan-scrolls) then there is no reason to keep it alive after this point. It should not be possible for a GestureScrollUpdate to be dispatched without a prerequisite GestureScrollBegin, but to be on the safe side I could also clear the previous scrolled node on a GestureScrollEnd as well. What do you think?
 
> What platforms have you tested this patch on?

This touch-scrolling path is only used by Chromium, and I have done manual testing on Linux and ChromeOS.

[James Robinson]

(In reply to comment #18)
> >> Source/WebCore/page/EventHandler.cpp:2680
> >> +        m_previousGestureScrolledNode = stopNode;
> > 
> > When is this reference cleared?
> 
> I clear |m_previousGestureScrolledNode| in EventHandler::handleGestureScrollBegin().

But that means the Node will be held alive until the next gesture scroll, which may be far in the distant future or never happen.  That's a major memory problem.  Why not clear it at the end of the gesture sequence?

[James Robinson]

(In reply to comment #19)
> (In reply to comment #16)
> > (From update of attachment 190372 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190372&action=review
> > 
> > Could you explain in a bit more detail how the lifetime of the previous gesture scrolled node is handled?  Is there any reason we need to keep this alive past the end of a gesture scroll sequence (i.e. past the GestureScrollEnd event)?
> 
> The member |m_previousGestureScrolledNode| is cleared on every GestureScrollBegin event. A fling gesture will not send a GestureScrollEnd event, which is why I opted to clear the value on a GestureScrollBegin instead. 
> 

It should

[Terry Anderson]

(In reply to comment #20)
> (In reply to comment #18)
> > >> Source/WebCore/page/EventHandler.cpp:2680
> > >> +        m_previousGestureScrolledNode = stopNode;
> > > 
> > > When is this reference cleared?
> > 
> > I clear |m_previousGestureScrolledNode| in EventHandler::handleGestureScrollBegin().
> 
> But that means the Node will be held alive until the next gesture scroll, which may be far in the distant future or never happen.  That's a major memory problem.  Why not clear it at the end of the gesture sequence?

That's a good point, I did not consider the impact on memory. I will clear both |m_previousGestureScrolledNode| and |m_scrollGestureHandlingNode| on a GestureScrollEnd and re-upload.

[Terry Anderson]

Created attachment 190599 [details]
Patch

[James Robinson]

Comment on attachment 190599 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2637
> +    m_previousGestureScrolledNode = 0;

this should just be an ASSERT() - we should never get a GestureScrollBegin after a GestureScrollUpdate without an intervening GestureScrollEnd

[Terry Anderson]

Comment on attachment 190599 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2637
>> +    m_previousGestureScrolledNode = 0;
> 
> this should just be an ASSERT() - we should never get a GestureScrollBegin after a GestureScrollUpdate without an intervening GestureScrollEnd

It is necessary to do this here because flings never send GestureScrollEnd events, so this is the only place where |m_previousGestureScrolledNode| will be cleared.

[James Robinson]

(In reply to comment #25)
> (From update of attachment 190599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190599&action=review
> 
> >> Source/WebCore/page/EventHandler.cpp:2637
> >> +    m_previousGestureScrolledNode = 0;
> > 
> > this should just be an ASSERT() - we should never get a GestureScrollBegin after a GestureScrollUpdate without an intervening GestureScrollEnd
> 
> It is necessary to do this here because flings never send GestureScrollEnd events, so this is the only place where |m_previousGestureScrolledNode| will be cleared.

OK, then you need to fix flings first.

[Terry Anderson]

Created attachment 190758 [details]
Patch

[Terry Anderson]

Comment on attachment 190758 [details]
Patch

The two stored nodes corresponding to a scroll event are now also cleared after a fling ends.

[James Robinson]

Comment on attachment 190758 [details]
Patch

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

Looks great! R=me

> LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-propagated.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

just <!DOCTYPE html> is better

[Terry Anderson]

Created attachment 191056 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191056 [details]
Patch for landing

Clearing flags on attachment: 191056

Committed r144519: <http://trac.webkit.org/changeset/144519>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Weinstein]

The new tests are failing on mac:

http://trac.webkit.org/changeset/144700

[Terry Anderson]

Chromium mac does not use the same gesture scrolling paths, so these failures are expected and it is fine to leave http://trac.webkit.org/changeset/144519 in the tree. The chromium mac baselines will need to be changed, which I am tracking here https://bugs.webkit.org/show_bug.cgi?id=111462 .