Bug 109769 - EventHandler::handleGestureScrollUpdate() should invoke the user-generated scroll routines so its behavior matches other user-initiated scrolls
Summary: EventHandler::handleGestureScrollUpdate() should invoke the user-generated sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Terry Anderson
URL:
Keywords:
Depends on:
Blocks: 109316 111007
  Show dependency treegraph
 
Reported: 2013-02-13 16:30 PST by Terry Anderson
Modified: 2013-03-05 12:01 PST (History)
12 users (show)

See Also:


Attachments
Early WIP, not for review (11.41 KB, patch)
2013-02-13 16:32 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (16.59 KB, patch)
2013-02-15 16:28 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (57.57 KB, patch)
2013-02-25 16:16 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch for review (54.08 KB, patch)
2013-02-26 15:18 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (51.14 KB, patch)
2013-02-27 13:28 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (52.66 KB, patch)
2013-02-27 13:59 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (54.10 KB, patch)
2013-02-28 10:48 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch for landing (54.16 KB, patch)
2013-03-01 15:54 PST, Terry Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Anderson 2013-02-13 16:30:32 PST
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.
Comment 1 Terry Anderson 2013-02-13 16:32:53 PST
Created attachment 188219 [details]
Early WIP, not for review
Comment 2 Terry Anderson 2013-02-13 16:35:34 PST
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.
Comment 3 WebKit Review Bot 2013-02-13 18:42:37 PST
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
Comment 4 WebKit Review Bot 2013-02-13 19:27:06 PST
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
Comment 5 Terry Anderson 2013-02-15 16:28:02 PST
Created attachment 188663 [details]
Patch
Comment 6 Darin Adler 2013-02-15 16:31:25 PST
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.
Comment 7 James Robinson 2013-02-15 16:32:30 PST
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.
Comment 8 Terry Anderson 2013-02-15 16:40:19 PST
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().
Comment 9 Terry Anderson 2013-02-15 16:46:41 PST
(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.
Comment 10 WebKit Review Bot 2013-02-15 17:44:25 PST
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
Comment 11 Terry Anderson 2013-02-25 16:16:50 PST
Created attachment 190150 [details]
Patch
Comment 12 Terry Anderson 2013-02-25 16:30:04 PST
(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.
Comment 13 Terry Anderson 2013-02-26 15:18:03 PST
Created attachment 190372 [details]
Patch for review
Comment 14 Eric Seidel (no email) 2013-02-26 15:21:47 PST
James is probably your best reviewer, but I can also review if he's busy.
Comment 15 Robert Kroeger 2013-02-27 08:41:34 PST
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.
Comment 16 James Robinson 2013-02-27 10:26:30 PST
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.
Comment 17 Terry Anderson 2013-02-27 13:28:04 PST
Created attachment 190592 [details]
Patch
Comment 18 Terry Anderson 2013-02-27 13:31:43 PST
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.
Comment 19 Terry Anderson 2013-02-27 13:39:30 PST
(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.
Comment 20 James Robinson 2013-02-27 13:39:55 PST
(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?
Comment 21 James Robinson 2013-02-27 13:40:07 PST
(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
Comment 22 Terry Anderson 2013-02-27 13:46:26 PST
(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.
Comment 23 Terry Anderson 2013-02-27 13:59:25 PST
Created attachment 190599 [details]
Patch
Comment 24 James Robinson 2013-02-27 14:05:13 PST
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
Comment 25 Terry Anderson 2013-02-27 14:08:39 PST
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.
Comment 26 James Robinson 2013-02-27 14:12:46 PST
(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.
Comment 27 Terry Anderson 2013-02-28 10:48:52 PST
Created attachment 190758 [details]
Patch
Comment 28 Terry Anderson 2013-02-28 10:49:55 PST
Comment on attachment 190758 [details]
Patch

The two stored nodes corresponding to a scroll event are now also cleared after a fling ends.
Comment 29 James Robinson 2013-03-01 15:32:03 PST
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
Comment 30 Terry Anderson 2013-03-01 15:54:59 PST
Created attachment 191056 [details]
Patch for landing
Comment 31 WebKit Review Bot 2013-03-01 17:10:24 PST
Comment on attachment 191056 [details]
Patch for landing

Clearing flags on attachment: 191056

Committed r144519: <http://trac.webkit.org/changeset/144519>
Comment 32 WebKit Review Bot 2013-03-01 17:10:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Rafael Weinstein 2013-03-04 17:19:37 PST
The new tests are failing on mac:

http://trac.webkit.org/changeset/144700
Comment 34 Terry Anderson 2013-03-05 12:01:16 PST
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 .