Bug 80311 - Handle more Gesture* events by performing scrolls on the correct target ScrollableArea
Summary: Handle more Gesture* events by performing scrolls on the correct target Scrol...
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: Robert Kroeger
URL:
Keywords:
Depends on: 79827 80201
Blocks: 67822
  Show dependency treegraph
 
Reported: 2012-03-05 12:07 PST by Robert Kroeger
Modified: 2012-11-30 09:51 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.22 KB, patch)
2012-03-05 12:11 PST, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (18.84 KB, patch)
2012-03-07 18:18 PST, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (23.26 KB, patch)
2012-03-09 13:48 PST, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (23.60 KB, patch)
2012-03-09 16:37 PST, Robert Kroeger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Kroeger 2012-03-05 12:07:55 PST
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.
Comment 1 Robert Kroeger 2012-03-05 12:11:24 PST
Created attachment 130176 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-05 12:15:15 PST
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.
Comment 3 Early Warning System Bot 2012-03-05 12:34:46 PST
Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11817155
Comment 4 Gyuyoung Kim 2012-03-05 12:38:45 PST
Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11819133
Comment 5 Gustavo Noronha (kov) 2012-03-05 14:45:54 PST
Comment on attachment 130176 [details]
Patch

Attachment 130176 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11808756
Comment 6 Robert Kroeger 2012-03-07 18:18:08 PST
Created attachment 130743 [details]
Patch
Comment 7 Robert Kroeger 2012-03-07 18:28:00 PST
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.
Comment 8 James Robinson 2012-03-07 18:56:16 PST
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.
Comment 9 Robert Kroeger 2012-03-08 07:58:41 PST
(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.
Comment 10 Robert Kroeger 2012-03-09 13:48:15 PST
Created attachment 131095 [details]
Patch
Comment 11 Robert Kroeger 2012-03-09 13:49:32 PST
jamesr@: perhaps you could take a look please?
Comment 12 James Robinson 2012-03-09 14:20:58 PST
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
Comment 13 Robert Kroeger 2012-03-09 16:09:53 PST
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.
Comment 14 Robert Kroeger 2012-03-09 16:37:59 PST
Created attachment 131134 [details]
Patch
Comment 15 WebKit Review Bot 2012-03-09 21:45:40 PST
Comment on attachment 131134 [details]
Patch

Clearing flags on attachment: 131134

Committed r110371: <http://trac.webkit.org/changeset/110371>
Comment 16 WebKit Review Bot 2012-03-09 21:45:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Philippe Normand 2012-03-11 03:48:46 PDT
(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
Comment 18 Philippe Normand 2012-03-12 01:55:37 PDT
(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
Comment 19 James Robinson 2012-03-16 21:23:01 PDT
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.
Comment 20 Robert Kroeger 2012-03-18 14:38:25 PDT
(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.
Comment 21 Robert Kroeger 2012-11-30 09:51:10 PST
Cleanup sufficient to close the bug is in http://trac.webkit.org/changeset/135638