Bug 80311 – Handle more Gesture* events by performing scrolls on the correct target ScrollableArea

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Robert Kroeger]

Currently, EventHandler::handleGestureEvent dispatches only GestureScrollUpdate events by converting them into wheel events but does not handle other scrolling-related GestureEvents. This patch corrects this by invoking fling-scroll on the same ScrollableArea as the target of the wheel event.

[Robert Kroeger]

Created attachment 130176 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/EventHandler.cpp', u'S..." exit_code: 1
Source/WebCore/page/EventHandler.h:340:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.h:341:  The parameter name "e" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/page/EventHandler.h:341:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/ScrollTypes.h:109:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:250:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:254:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2383:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/page/EventHandler.cpp:2382:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2383:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2385:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2386:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2387:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2388:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2389:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2390:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2391:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2392:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2393:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2394:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2395:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2396:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2397:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2397:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:2398:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2398:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:2399:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2400:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2401:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2402:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2403:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2404:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2430:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2431:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/page/EventHandler.cpp:2437:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderBox.cpp:677:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/ScrollableArea.cpp:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 36 in 5 files


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

[Early Warning System Bot]

Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11817155

[Gyuyoung Kim]

Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11819133

[Gustavo Noronha (kov)]

Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11808756

[Robert Kroeger]

Created attachment 130743 [details]
Patch

[Robert Kroeger]

jamesr@: we've discussed the previous rev of this CL. I think that this approach is more in keeping with what we discussed. Could you have a look and give me some comments on approach. I would like to think that this code is largely complete -- it works when combined with 79827 -- but I would not consider the patch committable until I add some additional tests.

[James Robinson]

Comment on attachment 130743 [details]
Patch

The additional statefulness in EventHandler feels a bit weird. I see you have a FIXME for it. Perhaps fixing it would be more effort/churn than it's worth at this point, but it's worth looking in to splitting the actual scrolling logic out of handleWheelEvent() so the rest of the world doesn't have to fake out event types and call into that function.

Otherwise at a high level this looks fine.

[Robert Kroeger]

(In reply to comment #8)
> (From update of attachment 130743 [details])
> The additional statefulness in EventHandler feels a bit weird. I see you have a FIXME for it. Perhaps fixing it would be more effort/churn than it's worth at this point, but it's worth looking in to splitting the actual scrolling logic out of handleWheelEvent() so the rest of the world doesn't have to fake out event types and call into that function.

I think we are in agreement with there's something "unclean" with the existing code (even without this patch I think.)

I think the ScrollableArea::scroll() interface is too restrictive to support trackpad or touchscreen scrolling. And the historical solution to this would seem to be to have been to overload PlatformWheelEvent with extra state (e.g. m_phase) that tells the downstream recipient how to scroll. This could probably stand to be fixed: there could be a richer "scroll" interface that tells the ScrollableArea how to scroll. 

I don't really want to do this in this patch. I'm not even sure how it should be done. I added https://bugs.webkit.org/show_bug.cgi?id=80596 to track this concept.


I think that overloading PlatformWheelEvent would make this patch cleaner and eliminate the additional state in EventHandler.


> 
> Otherwise at a high level this looks fine.

Excellent. I'll clean it up further.

[Robert Kroeger]

Created attachment 131095 [details]
Patch

[Robert Kroeger]

jamesr@: perhaps you could take a look please?

[James Robinson]

Comment on attachment 131095 [details]
Patch

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

I think this'll work fine for now. Left a number of nits to deal with.

> Source/WebCore/page/EventHandler.cpp:2370
> +    TemporaryChange<PlatformEvent::Type> baseEventType(m_baseEventType, gestureEvent.type());

can you put a FIXME here linked to the ::scroll() bug, if that is what'll resolve this?

> Source/WebCore/page/EventHandler.cpp:2417
> +    PlatformWheelEvent syntheticWheelEvent(point, globalPoint, gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / tickDivisor, gestureEvent.deltaY() / tickDivisor, granularity, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());

WebKit doesn't have a fixed line limit length but I'm finding this one pretty crazy. Would you mind breaking it up into a few lines - maybe one for the point, one for all the deltas, one for granularity, and one for all the modifier keys?

> Source/WebCore/platform/ScrollAnimator.cpp:102
> +                scroll(VerticalScrollbar, ScrollByPixelVelocity, 0, -deltaY);

this is indented too far, should be 4 spaces

> Source/WebCore/platform/ScrollAnimatorNone.cpp:382
> +    , m_firstVelocity(0.f)

this should just be 0 (see http://www.webkit.org/coding/coding-style.html#float-suffixes)

> Source/WebCore/platform/ScrollAnimatorNone.cpp:393
> +void ScrollAnimatorNone::fireUpAnAnimation(float x, float y)

this might get you an unused variable warning on some platforms (I forget which set the compile flags that generate this). The standard ways to avoid this are to either omit the parameter names, comment them out, or use the UNUSED_PARAM() macro in the function body

> Source/WebCore/platform/ScrollAnimatorNone.h:142
> +    virtual void fireUpAnAnimation(float, float);

should this take a FloatPoint? most things that take an x/y pair do

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:85
> +    float m_f1;
> +    float m_f2;

are these an X/Y pair? if so can they be a FloatPoint or FloatSize? That will also give you zero-initialization for free

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:145
> +    scrollAnimatorNone.scroll(HorizontalScrollbar, ScrollByPixelVelocity, 111.f, -42.f);
> +    scrollAnimatorNone.scroll(VerticalScrollbar, ScrollByPixelVelocity, 222.f, 42.f);

drop the .f's here and in the rest of the test files

[Robert Kroeger]

Comment on attachment 131095 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2370
>> +    TemporaryChange<PlatformEvent::Type> baseEventType(m_baseEventType, gestureEvent.type());
> 
> can you put a FIXME here linked to the ::scroll() bug, if that is what'll resolve this?

yes.  done.

>> Source/WebCore/page/EventHandler.cpp:2417
>> +    PlatformWheelEvent syntheticWheelEvent(point, globalPoint, gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / tickDivisor, gestureEvent.deltaY() / tickDivisor, granularity, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());
> 
> WebKit doesn't have a fixed line limit length but I'm finding this one pretty crazy. Would you mind breaking it up into a few lines - maybe one for the point, one for all the deltas, one for granularity, and one for all the modifier keys?

thank you. I have suffered through this since the first time I got told in review to un-break it.

>> Source/WebCore/platform/ScrollAnimator.cpp:102
>> +                scroll(VerticalScrollbar, ScrollByPixelVelocity, 0, -deltaY);
> 
> this is indented too far, should be 4 spaces

oops. done, next patch.

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:382
>> +    , m_firstVelocity(0.f)
> 
> this should just be 0 (see http://www.webkit.org/coding/coding-style.html#float-suffixes)

done.

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:393
>> +void ScrollAnimatorNone::fireUpAnAnimation(float x, float y)
> 
> this might get you an unused variable warning on some platforms (I forget which set the compile flags that generate this). The standard ways to avoid this are to either omit the parameter names, comment them out, or use the UNUSED_PARAM() macro in the function body

thanks. done next patch.

>> Source/WebCore/platform/ScrollAnimatorNone.h:142
>> +    virtual void fireUpAnAnimation(float, float);
> 
> should this take a FloatPoint? most things that take an x/y pair do

done, next patch.

>> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:85
>> +    float m_f2;
> 
> are these an X/Y pair? if so can they be a FloatPoint or FloatSize? That will also give you zero-initialization for free

cool. done.

>> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:145
>> +    scrollAnimatorNone.scroll(VerticalScrollbar, ScrollByPixelVelocity, 222.f, 42.f);
> 
> drop the .f's here and in the rest of the test files

done, next patch.

[Robert Kroeger]

Created attachment 131134 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131134 [details]
Patch

Clearing flags on attachment: 131134

Committed r110371: <http://trac.webkit.org/changeset/110371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to comment #16)
> All reviewed patches have been landed.  Closing bug.

Looks like this patch broke a GTK layout test: platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r110371%20(19734)/results.html

[Philippe Normand]

(In reply to comment #17)
> (In reply to comment #16)
> > All reviewed patches have been landed.  Closing bug.
> 
> Looks like this patch broke a GTK layout test: platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html
> 
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r110371%20(19734)/results.html

See bug 80825

[James Robinson]

Now that I know more about how this system works I'm thinking this patch is wrong and at least some of it needs to be reverted:

*) For wheel interactions, we need to hit test + scroll on every intermediate point, not just the beginning of the interaction. Given that, there's no point in distinguishing a begin for a scroll type that's really a wheel.  That said, for gestures that really are touch interactions (like finger-dragging on a touchpad) you definitely do want to hit test only on the start of the event and retain knowledge of the hit target since that is the hit target 

*) Overriding GestureEnd to mean fling is an ugly ugly hack and not really compatible with what android does. We have a separate gesture type for fling start and fling cancel in the chromium WebKit API - GestureFlingStart / GestureFlingCancel.  We should use those.

I'll take a closer look tomorrow with a fresh set of eyes and see what should be done.

[Robert Kroeger]

(In reply to comment #19)
> Now that I know more about how this system works I'm thinking this patch is wrong and at least some of it needs to be reverted:
> 
> *) For wheel interactions, we need to hit test + scroll on every intermediate point, not just the beginning of the interaction. Given that, there's no point in distinguishing a begin for a scroll type that's really a wheel.  That said, for gestures that really are touch interactions (like finger-dragging on a touchpad) you definitely do want to hit test only on the start of the event and retain knowledge of the hit target since that is the hit target 
> 
> *) Overriding GestureEnd to mean fling is an ugly ugly hack and not really compatible with what android does. We have a separate gesture type for fling start and fling cancel in the chromium WebKit API - GestureFlingStart / GestureFlingCancel.  We should use those.
> 
> I'll take a closer look tomorrow with a fresh set of eyes and see what should be done.

I think we discussed. This bug is obsolete. I will clean up the code.

[Robert Kroeger]

Cleanup sufficient to close the bug is in http://trac.webkit.org/changeset/135638