Bug 103952 – Scroll gestures should not create wheel events

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

[Terry Anderson]

In EventHandler::handleGestureEvent(), scroll gestures should not be handled by synthesizing and dispatching wheel events; we should instead scroll the relevant layer directly.

[Terry Anderson]

Created attachment 177387 [details]
Patch

[Antonio Gomes]

Comment on attachment 177387 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

oops should not be here.

> Source/WebCore/page/EventHandler.cpp:2752
> +    Node* node = m_scrollGestureHandlingNode.get();
> +    if ((!node || !node->renderer()) && !hitTestForScrollGestureHandlingNode(e))
> +        return false;
> +
> +    node = m_scrollGestureHandlingNode.get();
> +    RenderObject* latchedRenderer = node->renderer();
> +
> +    IntSize delta(-e.deltaX(), -e.deltaY());
> +    if (delta.isZero())
> +        return false;
> +    
> +    bool restrictedByLineClamp = false;
> +    if (latchedRenderer->parent())
> +        restrictedByLineClamp = !latchedRenderer->parent()->style()->lineClamp().isNone();
> +
> +    if (latchedRenderer->hasOverflowClip() && !restrictedByLineClamp) {
> +        if (e.type() == PlatformEvent::GestureScrollUpdatePropagated)
> +            latchedRenderer->enclosingLayer()->scrollByRecursively(delta, RenderLayer::ScrollOffsetClamped);
> +        else {
> +            IntSize scrollOffset = latchedRenderer->enclosingLayer()->scrollOffset();
> +            IntSize newScrollOffset = scrollOffset + delta;
> +            latchedRenderer->enclosingLayer()->scrollToOffset(newScrollOffset, RenderLayer::ScrollOffsetClamped);
> +        }
> +        setFrameWasScrolledByUser();
> +        return true;
> +    }

should live in RenderLayer. r- due to that.

> Source/WebCore/page/EventHandler.cpp:2790
> +    return (node && node->renderer());

no outter parentheses needed.

> Source/WebCore/platform/PlatformEvent.h:55
> +        GestureScrollUpdatePropagated,
> +        GestureScrollUpdateNotPropagated,

I am not in love with these names. No better suggestion at the moment though.

[Robert Kroeger]

Comment on attachment 177387 [details]
Patch

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

>> Source/WebCore/platform/PlatformEvent.h:55
>> +        GestureScrollUpdateNotPropagated,
> 
> I am not in love with these names. No better suggestion at the moment though.

tdanderson and I brainstormed about possible names and didn't really come up with anything that we loved either. The "Update" seems redundant.

maybe these do a better job of capturing the intent?

GestureScrollOneTargetedLayer
GestureScrollDeepestStillScrollableLayer

[Terry Anderson]

(In reply to comment #3)
> (From update of attachment 177387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177387&action=review
> 
> >> Source/WebCore/platform/PlatformEvent.h:55
> >> +        GestureScrollUpdateNotPropagated,
> > 
> > I am not in love with these names. No better suggestion at the moment though.
> 
> tdanderson and I brainstormed about possible names and didn't really come up with anything that we loved either. The "Update" seems redundant.
> 
> maybe these do a better job of capturing the intent?
> 
> GestureScrollOneTargetedLayer
> GestureScrollDeepestStillScrollableLayer

I like keeping "propagate" in the name. How about these instead?

GestureScrollSingleLayer
GestureScrollPropagateFromDeepestLayer

[Alexey Proskuryakov]

(In reply to comment #0)
> In EventHandler::handleGestureEvent(), scroll gestures should not be handled by synthesizing and dispatching wheel events; we should instead scroll the relevant layer directly.

Two questions:

1. Did you research why it was done this way?

2. Will this break Google Maps, which currently zooms when I make a scroll gesture on my trackpad?

[Terry Anderson]

(In reply to comment #5)
> (In reply to comment #0)
> > In EventHandler::handleGestureEvent(), scroll gestures should not be handled by synthesizing and dispatching wheel events; we should instead scroll the relevant layer directly.
> 
> Two questions:
> 
> 1. Did you research why it was done this way?

It was put in place by @rjkroege but was only meant to be temporary.
 
> 2. Will this break Google Maps, which currently zooms when I make a scroll gesture on my trackpad?

Trackpad scrolling still sends wheel events, so this will not break zoom on Google Maps.

[Simon Fraser (smfr)]

Comment on attachment 177387 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        In EventHandler::handleGestureEvent(), scroll gestures should not be handled by synthesizing
> +        and dispatching wheel events; we should instead scroll the relevant layer directly.

Which platforms make use of scroll gestures?

[Terry Anderson]

(In reply to comment #7)
> (From update of attachment 177387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177387&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        In EventHandler::handleGestureEvent(), scroll gestures should not be handled by synthesizing
> > +        and dispatching wheel events; we should instead scroll the relevant layer directly.
> 
> Which platforms make use of scroll gestures?

AFAIK, only chromium and qt.

[Terry Anderson]

Created attachment 181723 [details]
Patch

[Terry Anderson]

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

Work in progress. Scroll events do not propagate to parent iframes.

[Ryosuke Niwa]

Please don’t set r- on patches. Instead, simplify clear the review flag.

[Terry Anderson]

Created attachment 181735 [details]
WIP, added in change to handle webkit.org/b/106040

[James Robinson]

Comment on attachment 181735 [details]
WIP, added in change to handle webkit.org/b/106040

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

> Source/WebCore/page/EventHandler.cpp:2461
> +        if (gestureEvent.type() == PlatformEvent::GestureScrollBegin
> +            || gestureEvent.type() == PlatformEvent::GestureScrollEnd) {

I probably wouldn't line wrap this

> Source/WebCore/page/EventHandler.cpp:2607
> +bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& e)

use a real variable name, not 'e'. In WebKit style the only one-letter variable names allowed in normal code are i/j/k for loop counters. I think the name 'gestureEvent' was pretty good, although I could see shortening it to 'event'

> Source/WebCore/page/EventHandler.cpp:2609
> +    if (!hitTestForScrollGestureHandlingNode(e))

Why would we hit test on a gesture scroll update?  That doesn't make sense to me - my understanding of the gesture scroll model is as follows:

GestureScrollBegin: hit test at point, pick a scrolling target
GestureScrollUpdate: move the scrolling target in indicated direction
GestureScrollEnd: clear scrolling target

> Source/WebCore/page/EventHandler.cpp:2641
> +bool EventHandler::hitTestForScrollGestureHandlingNode(const PlatformGestureEvent& e)

Do not abbreviate

> Source/WebCore/page/EventHandler.cpp:2643
> +    Document* doc = m_frame->document();

Do not abbreviate

> Source/WebCore/page/EventHandler.cpp:2644
> +    RenderObject* docRenderer = doc->renderer();

Do not abbreviate

> Source/WebCore/page/EventHandler.cpp:2657
> +    LayoutPoint vPoint = view->windowToContents(e.position());

Do not abbreviate

[Terry Anderson]

Created attachment 181823 [details]
Patch

[Terry Anderson]

(In reply to comment #13)
> (From update of attachment 181735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181735&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2461
> > +        if (gestureEvent.type() == PlatformEvent::GestureScrollBegin
> > +            || gestureEvent.type() == PlatformEvent::GestureScrollEnd) {
> 
> I probably wouldn't line wrap this
> 
> > Source/WebCore/page/EventHandler.cpp:2607
> > +bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& e)
> 
> use a real variable name, not 'e'. In WebKit style the only one-letter variable names allowed in normal code are i/j/k for loop counters. I think the name 'gestureEvent' was pretty good, although I could see shortening it to 'event'
> 
> > Source/WebCore/page/EventHandler.cpp:2609
> > +    if (!hitTestForScrollGestureHandlingNode(e))
> 
> Why would we hit test on a gesture scroll update?  That doesn't make sense to me - my understanding of the gesture scroll model is as follows:
> 
> GestureScrollBegin: hit test at point, pick a scrolling target
> GestureScrollUpdate: move the scrolling target in indicated direction
> GestureScrollEnd: clear scrolling target
> 

This is done in case there are changes to the DOM while scrolling is taking place. A hit test is only done when necessary, not every time.

> > Source/WebCore/page/EventHandler.cpp:2641
> > +bool EventHandler::hitTestForScrollGestureHandlingNode(const PlatformGestureEvent& e)
> 
> Do not abbreviate
> 
> > Source/WebCore/page/EventHandler.cpp:2643
> > +    Document* doc = m_frame->document();
> 
> Do not abbreviate
> 
> > Source/WebCore/page/EventHandler.cpp:2644
> > +    RenderObject* docRenderer = doc->renderer();
> 
> Do not abbreviate
> 
> > Source/WebCore/page/EventHandler.cpp:2657
> > +    LayoutPoint vPoint = view->windowToContents(e.position());
> 
> Do not abbreviate

[Terry Anderson]

Comment on attachment 181823 [details]
Patch

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

> LayoutTests/platform/chromium/fast/events/touch/gesture/touch-gesture-scroll-div-scaled-expected.txt:11
> +PASS movingdiv.scrollTop is 47

@aelias, is this an acceptable change to the expectations for your test?

[Alexandre Elias]

Scaling changes LGTM, thanks.

[Build Bot]

Comment on attachment 181823 [details]
Patch

Attachment 181823 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15754553

[Build Bot]

Comment on attachment 181823 [details]
Patch

Attachment 181823 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15762322

[EFL EWS Bot]

Comment on attachment 181823 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 181823 [details]
Patch

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

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-div-scaled.html

[WebKit Review Bot]

Comment on attachment 181823 [details]
Patch

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

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-div-scaled.html

[Terry Anderson]

Created attachment 181938 [details]
Patch

[Antonio Gomes]

Comment on attachment 181938 [details]
Patch

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

Looks generally good, although I did not complete review it. I have some comments and requests, and I think it is worth it another interaction. r- due to that.

> Source/WebCore/page/EventHandler.cpp:2497
> +            RenderObject* latchedRenderer = node->renderer();
> +            if (m_lastHitTestResultOverWidget && latchedRenderer && latchedRenderer->isWidget()) {
> +                Widget* widget = toRenderWidget(latchedRenderer)->widget();
> +                if (widget)
> +                    passGestureEventToWidget(gestureEvent, widget);
> +            }

there is another similar block of code below. Should we create a static local shouldPassGestureToWidget method, and avoid duplications?

> Source/WebCore/page/EventHandler.cpp:2626
> +    bool shouldPropagate = (gestureEvent.type() == PlatformEvent::GestureScrollUpdatePropagated) ? true : false;

nit: no " a? b :c;" needed here.

> Source/WebCore/page/EventHandler.cpp:2670
> +        m_scrollGestureHandlingNode = closestScrollableNodeInDocumentIfPossible(result.innerNode());

I remember that the BlackBerry port has a recursive version of "closestScrollableNodeInDocumentIfPossible" that returns a list of scrollable layers, from the deepest to the "shallowest", whereas given the initial user scroll direction and the amount of offset left to be scrolled in that direction, it could pick the proper layer.

see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp#L202 - void InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint(const WebCore::IntPoint& documentPoint)

Also, once a layer was picked, it would stick with it for the whole scrolling action.

How does it behavior in your case?

> Source/WebCore/page/EventHandler.h:379
> +    bool hitTestForScrollGestureHandlingNode(const PlatformGestureEvent&);

I guess a "IfNeeded" suffix to the method name would have made it clearer to James that it might bail out earlier without hit testing.

> Source/WebCore/page/EventHandler.h:470
> +    bool m_lastHitTestResultOverWidget;

do you need to manually default initialize it in the ctor?

> Source/WebCore/page/chromium/EventHandlerChromium.cpp:97
> +    return static_cast<FrameView*>(widget)->frame()->eventHandler()->handleGestureEvent(gestureEvent);

do not you need to convert the gestureEvent position to the widget's coordinates?

> Source/WebCore/page/gtk/EventHandlerGtk.cpp:86
> +#if ENABLE(GESTURE_EVENTS)
> +bool EventHandler::passGestureEventToWidget(const PlatformGestureEvent&, Widget*)
> +{
> +    notImplemented();
> +    return false;
> +}
> +#endif
> +

have you considered to add a default stub/empty implementation to EventHandler.h instead of adding stubs to all non-chromium port specific files?

What do other similar cases do?

> Source/WebCore/rendering/RenderLayer.cpp:1965
> +    scrollBy(delta, clamp, true);

bools are to be avoided. At the very least add a comment: /*whatIsThisBool*/, but an enum is highly recommended.

[James Robinson]

Comment on attachment 181938 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2488
> +    case PlatformEvent::GestureScrollBegin:
> +        break;

I don't understand this - the GestureScrollBegin event has to pick the target of scrolls for the rest of the gesture.  Is there handling for GestureScrollBegin somewhere else in this file?

[Antonio Gomes]

> Looks generally good, although I did not complete review it. I have some comments and requests, and I think it is worth it another interaction. r- due to that.

s/interaction/review round/g :)

[Terry Anderson]

Comment on attachment 181938 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2488
>> +        break;
> 
> I don't understand this - the GestureScrollBegin event has to pick the target of scrolls for the rest of the gesture.  Is there handling for GestureScrollBegin somewhere else in this file?

The hit testing on a GestureScrollBegin would have been handled when the first GestureScrollUpdate* event was received. But you're right, we should do the initial hit testing here, so I will change this.

>> Source/WebCore/page/EventHandler.cpp:2497
>> +            }
> 
> there is another similar block of code below. Should we create a static local shouldPassGestureToWidget method, and avoid duplications?

Done. Given the way I have done the refactoring, the most appropriate name seemed to be passGestureEventToWidgetIfPossible().

>> Source/WebCore/page/EventHandler.cpp:2626
>> +    bool shouldPropagate = (gestureEvent.type() == PlatformEvent::GestureScrollUpdatePropagated) ? true : false;
> 
> nit: no " a? b :c;" needed here.

Switched to using an enum instead of a bool here.

>> Source/WebCore/page/EventHandler.cpp:2670
>> +        m_scrollGestureHandlingNode = closestScrollableNodeInDocumentIfPossible(result.innerNode());
> 
> I remember that the BlackBerry port has a recursive version of "closestScrollableNodeInDocumentIfPossible" that returns a list of scrollable layers, from the deepest to the "shallowest", whereas given the initial user scroll direction and the amount of offset left to be scrolled in that direction, it could pick the proper layer.
> 
> see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp#L202 - void InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint(const WebCore::IntPoint& documentPoint)
> 
> Also, once a layer was picked, it would stick with it for the whole scrolling action.
> 
> How does it behavior in your case?

Our version (http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.cpp#L279) finds the deepest ancestor node whose corresponding RenderBox can be scrolled. (If no such node exists or we reach the root node, it just returns the node that was passed in.) The available offset left to be scrolled is not used in this function. It is instead handled when we call RenderLayer::scrollBy() which recursively propagates the remaining scroll delta up the layer tree, so a single scrolling action does not necessarily stay with the same layer.

>> Source/WebCore/page/EventHandler.h:379
>> +    bool hitTestForScrollGestureHandlingNode(const PlatformGestureEvent&);
> 
> I guess a "IfNeeded" suffix to the method name would have made it clearer to James that it might bail out earlier without hit testing.

Done.

>> Source/WebCore/page/EventHandler.h:470
>> +    bool m_lastHitTestResultOverWidget;
> 
> do you need to manually default initialize it in the ctor?

Done.

>> Source/WebCore/page/chromium/EventHandlerChromium.cpp:97
>> +    return static_cast<FrameView*>(widget)->frame()->eventHandler()->handleGestureEvent(gestureEvent);
> 
> do not you need to convert the gestureEvent position to the widget's coordinates?

No, because the conversion is done before we perform the hit test in hitTestForScrollGestureHandlingNode(). This is the same as how it works for passWheelEventToWidget() / handleWheelEvent().

>> Source/WebCore/page/gtk/EventHandlerGtk.cpp:86
>> +
> 
> have you considered to add a default stub/empty implementation to EventHandler.h instead of adding stubs to all non-chromium port specific files?
> 
> What do other similar cases do?

That way would be a lot cleaner, thanks for the suggestion. Changed.

>> Source/WebCore/rendering/RenderLayer.cpp:1965
>> +    scrollBy(delta, clamp, true);
> 
> bools are to be avoided. At the very least add a comment: /*whatIsThisBool*/, but an enum is highly recommended.

Done.

[Terry Anderson]

Created attachment 182021 [details]
Patch

[Early Warning System Bot]

Comment on attachment 182021 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 182021 [details]
Patch

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

[Terry Anderson]

Created attachment 182028 [details]
Patch

[James Robinson]

Comment on attachment 182028 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2674
> +    // Don't do a hit test if we already have a node targeted and it has a renderer.
> +    Node* node = m_scrollGestureHandlingNode.get();
> +    if (node && node->renderer())
> +        return true;

This isn't needed for GestureScrollBegin, since at that point we definitely won't have a targetted node.  On the flip side, I don't think the rest of this code is relevant for GestureScrollUpdate since we only want to continue scrolling the already-selected node.  I think you should split this up so we only hit test in GestureScrollBegin and then only do the update-relevant checks in GestureScrollUpdate.

[Antonio Gomes]

(In reply to comment #32)
> (From update of attachment 182028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182028&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2674
> > +    // Don't do a hit test if we already have a node targeted and it has a renderer.
> > +    Node* node = m_scrollGestureHandlingNode.get();
> > +    if (node && node->renderer())
> > +        return true;
> 
> This isn't needed for GestureScrollBegin, since at that point we definitely won't have a targetted node.  On the flip side, I don't think the rest of this code is relevant for GestureScrollUpdate since we only want to continue scrolling the already-selected node.  I think you should split this up so we only hit test in GestureScrollBegin and then only do the update-relevant checks in GestureScrollUpdate.

Agreed. So, if the scroll target is removed from the DOM in the middle of the scrolling action, UA should likely stop scrolling. Is that what you are asking for, James?

[Antonio Gomes]

Comment on attachment 182028 [details]
Patch

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

> Source/WebCore/page/chromium/EventHandlerChromium.cpp:89
> +bool EventHandler::passGestureEventToWidget(const PlatformGestureEvent& gestureEvent, Widget* widget)

Maybe  "Widget" in the name here is misleading: you pass it if it is over any widget, but only handles if the widget is a frameview. Scrollbars for example are no-op's.

[James Robinson]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 182028 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182028&action=review
> > 
> > > Source/WebCore/page/EventHandler.cpp:2674
> > > +    // Don't do a hit test if we already have a node targeted and it has a renderer.
> > > +    Node* node = m_scrollGestureHandlingNode.get();
> > > +    if (node && node->renderer())
> > > +        return true;
> > 
> > This isn't needed for GestureScrollBegin, since at that point we definitely won't have a targetted node.  On the flip side, I don't think the rest of this code is relevant for GestureScrollUpdate since we only want to continue scrolling the already-selected node.  I think you should split this up so we only hit test in GestureScrollBegin and then only do the update-relevant checks in GestureScrollUpdate.
> 
> Agreed. So, if the scroll target is removed from the DOM in the middle of the scrolling action, UA should likely stop scrolling. Is that what you are asking for, James?

Yes, I think that makes the most sense.

[James Robinson]

Comment on attachment 182028 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2597
> +#if !PLATFORM(CHROMIUM)

I don't understand this guard - could you explain?

[Terry Anderson]

Created attachment 182215 [details]
WIP, not for review

[Terry Anderson]

Comment on attachment 182028 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2597
>> +#if !PLATFORM(CHROMIUM)
> 
> I don't understand this guard - could you explain?

This stub was added after Antonio's comment in https://bugs.webkit.org/show_bug.cgi?id=103952#c24. It is a cleaner way of forcing all non-chromium platforms to have an implementation of passGestureEventToWidget() that just returns false (as opposed to modifying all of the platform-specific EventHandler*.cpp files).

>>>> Source/WebCore/page/EventHandler.cpp:2674
>>>> +        return true;
>>> 
>>> This isn't needed for GestureScrollBegin, since at that point we definitely won't have a targetted node.  On the flip side, I don't think the rest of this code is relevant for GestureScrollUpdate since we only want to continue scrolling the already-selected node.  I think you should split this up so we only hit test in GestureScrollBegin and then only do the update-relevant checks in GestureScrollUpdate.
>> 
>> Agreed. So, if the scroll target is removed from the DOM in the middle of the scrolling action, UA should likely stop scrolling. Is that what you are asking for, James?
> 
> Yes, I think that makes the most sense.

Changed.

>> Source/WebCore/page/chromium/EventHandlerChromium.cpp:89
>> +bool EventHandler::passGestureEventToWidget(const PlatformGestureEvent& gestureEvent, Widget* widget)
> 
> Maybe  "Widget" in the name here is misleading: you pass it if it is over any widget, but only handles if the widget is a frameview. Scrollbars for example are no-op's.

I chose this name for consistency with the existing EventHandler::passWheelEventToWidget(), which also returns false if the argument |widget| is not a FrameView. Do you think passGestureEventToFrameView() would be a more suitable name for this function?

[Terry Anderson]

Created attachment 182634 [details]
patch for review, but some layout tests still to be added

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Benjamin Poulain]

I think this is very confusing.

I think I would prefer to see a completely different type of events instead of abusing the existing gesture events.

[James Robinson]

I don't think apple ports use any of this currently, so I'm not sure I understand the confusion.  What would you like changed?

[James Robinson]

Comment on attachment 182634 [details]
patch for review, but some layout tests still to be added

Ah, the new GestureScroll* types are pretty weird.  It looks like you're trying to preserve the wheel-like behavior.  I would nuke it and your ENABLE() - the wheel-based behavior is busted.  If you get rid of that, do you need any new Gesture types?

[Benjamin Poulain]

(In reply to comment #42)
> I don't think apple ports use any of this currently, so I'm not sure I understand the confusion.  What would you like changed?

Gesture Events were created by Apple to support OS X native gestures.

I think there is some confusion about what are Gesture Events.

(In reply to comment #43)
> Ah, the new GestureScroll* types are pretty weird.  It looks like you're trying to preserve the wheel-like behavior.  I would nuke it and your ENABLE() - the wheel-based behavior is busted.  If you get rid of that, do you need any new Gesture types?

Yep, those are really weird.

[Darin Fisher (:fishd, Google)]

(In reply to comment #44)
> (In reply to comment #42)
> > I don't think apple ports use any of this currently, so I'm not sure I understand the confusion.  What would you like changed?
> 
> Gesture Events were created by Apple to support OS X native gestures.
> 
> I think there is some confusion about what are Gesture Events.

Hi Benjamin,

It looks like WebCore/dom/GestureEvent.{h,cpp} were added as part of this revision:
http://trac.webkit.org/changeset/124098

This patch aims to modify those files.  What "existing gesture events" are you referring to?  As far as I can tell, Apple didn't contribute anything related to gesture events to WebKit.

[Benjamin Poulain]

> It looks like WebCore/dom/GestureEvent.{h,cpp} were added as part of this revision:
> http://trac.webkit.org/changeset/124098
> 
> This patch aims to modify those files.  What "existing gesture events" are you referring to?  As far as I can tell, Apple didn't contribute anything related to gesture events to WebKit.

Nice attempt at rewriting history: http://trac.webkit.org/changeset/76745 :)

It is somewhat unfortunate Chromium Web Gesture Events share the same enum and some classes (like PlatformGestureEvent). The concepts are somewhat different.

[Antonio Gomes]

(In reply to comment #46)
> > It looks like WebCore/dom/GestureEvent.{h,cpp} were added as part of this revision:
> > http://trac.webkit.org/changeset/124098
> > 
> > This patch aims to modify those files.  What "existing gesture events" are you referring to?  As far as I can tell, Apple didn't contribute anything related to gesture events to WebKit.
> 
> Nice attempt at rewriting history: http://trac.webkit.org/changeset/76745 :)
> 
> It is somewhat unfortunate Chromium Web Gesture Events share the same enum and some classes (like PlatformGestureEvent). The concepts are somewhat different.

I think there is an overlap in concepts as well, but it could very well be addressed as a follow up, since this patch does not make the situation *much* worse.

[Alexey Proskuryakov]

Well, this patch adds a lot more gesture event code to cross-platform files, and changes how gesture events relate to other event types, so it's not a small thing.

[Terry Anderson]

Created attachment 182818 [details]
WIP, not for review

[James Robinson]

(In reply to comment #48)
> Well, this patch adds a lot more gesture event code to cross-platform files, and changes how gesture events relate to other event types, so it's not a small thing.

I'm confident we can accomplish the goals of this bug without adding any new event types.

[Terry Anderson]

Created attachment 183036 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183036 [details]
Patch

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

New failing tests:
platform/chromium/plugins/gesture-events.html
platform/chromium/plugins/gesture-events-scrolled.html
platform/chromium/plugins/transformed-events.html

[Terry Anderson]

Created attachment 183047 [details]
Patch

[James Robinson]

Comment on attachment 183036 [details]
Patch

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

The cross-platform changes look pretty good, but some of the chromium-specific ones aren't right.  I think it would be better bug management (and easier for other reviewers to understand) if you'd split the chromium-specific bits into a separate bug + patch.  We would probably want to land both patches at the same time, or in very close succession, but it'd make the review process easier.

> Source/WebCore/page/EventHandler.cpp:2638
> +        passGestureEventToWidgetIfPossible(gestureEvent, node->renderer());

this function returns a bool, but the return value appears to be ignored here - why?

> Source/WebCore/page/EventHandler.cpp:2645
> +    RefPtr<FrameView> protector(m_frame->view());

please move this down to where it's needed - we don't need a ref to null-check members, for instance. i think the first call that could lead to dereffing the FrameView would be the passGestureEventTo..() call

> Source/WebCore/page/EventHandler.cpp:2675
> +    delta.scale(1 / scaleFactor, 1 / scaleFactor);

do we have tests covering the scaling behavior for page zoom and frame scale?  Where are they?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:794
> +
> +    WebMouseWheelEventBuilder wheelEvent(this, m_element->renderer(), *event);
> +    if (m_webPlugin->handleInputEvent(wheelEvent, cursorInfo)) {
> +        event->setDefaultHandled();
> +        return;
> +    }

No - this isn't right.  We never want to synthesize mousewheels for plugins (I checked with cpu and brettw).  There are a few cases for plugins we care about in chromium:

Plugins that use the WebScrollbarGroup system (pdf, for instance).  Detect these by checking if m_scrollGroup is NULL.  If the m_scrollbarGroup is not null, we should handle the scroll directly in this file by adjusting the appropriate scrollbar offset(s).  That's all the pdf plugin actually ends up doing with mousewheel events.

Plugins that don't use this system - for these, we want to pass the touch events to the plugin but not pass any gesture or wheel events for scrolling.  Flash, for instance, never handles mousewheels.

[James Robinson]

Comment on attachment 183047 [details]
Patch

Sorry, made a mess of the flags due to bugzilla's lovely conflict resolution.

[Terry Anderson]

Comment on attachment 183036 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2638
>> +        passGestureEventToWidgetIfPossible(gestureEvent, node->renderer());
> 
> this function returns a bool, but the return value appears to be ignored here - why?

I am purposely calling this function here for its side effects only. In the case where node->renderer() is actually a FrameView, I need to pass it the GestureScrollBegin so that its |m_scrollGestureHandlingNode| is set. But at the same time, I want this invocation of handleGestureScrollBegin() to return true if and only if we've found a valid target, and the return value of passGestureEventToWidgetIfPossible() is unrelated to this.

>> Source/WebCore/page/EventHandler.cpp:2645
>> +    RefPtr<FrameView> protector(m_frame->view());
> 
> please move this down to where it's needed - we don't need a ref to null-check members, for instance. i think the first call that could lead to dereffing the FrameView would be the passGestureEventTo..() call

Done.

>> Source/WebCore/page/EventHandler.cpp:2675
>> +    delta.scale(1 / scaleFactor, 1 / scaleFactor);
> 
> do we have tests covering the scaling behavior for page zoom and frame scale?  Where are they?

fast/events/touch/gesture/touch-gesture-scroll-div-scaled.html was checked in along with this scaling code in https://bugs.webkit.org/show_bug.cgi?id=106263 .

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:794
>> +    }
> 
> No - this isn't right.  We never want to synthesize mousewheels for plugins (I checked with cpu and brettw).  There are a few cases for plugins we care about in chromium:
> 
> Plugins that use the WebScrollbarGroup system (pdf, for instance).  Detect these by checking if m_scrollGroup is NULL.  If the m_scrollbarGroup is not null, we should handle the scroll directly in this file by adjusting the appropriate scrollbar offset(s).  That's all the pdf plugin actually ends up doing with mousewheel events.
> 
> Plugins that don't use this system - for these, we want to pass the touch events to the plugin but not pass any gesture or wheel events for scrolling.  Flash, for instance, never handles mousewheels.

I have pulled the chromium-specific changes out of this patch. rjkroege and I will instead use https://bugs.webkit.org/show_bug.cgi?id=106589 to track those.

[Terry Anderson]

Created attachment 183069 [details]
Patch

[James Robinson]

Comment on attachment 183069 [details]
Patch

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

OK, R=me.

For folks following along who maybe don't want to dig through the patch, this patch doesn't introduce any new event types or platform-specific code, it just fixes the handling of already existing events.

> LayoutTests/fast/events/touch/gesture/resources/scroll-inside-editable-iframe.html:1
> +<html>

add <!DOCTYPE html> above this (same goes for any other new test files) so the file is in standards mode, not quirks mode

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

just <!DOCTYPE html>

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

just <!DOCTYPE html>

> LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-editable.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

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

<!DOCTYPE html>

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

ditto

[Antonio Gomes]

Comment on attachment 183069 [details]
Patch

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

I won't override James' r+, but I have feel comments.

> Source/WebCore/page/EventHandler.cpp:2605
> +static Node* closestScrollableNodeInDocumentIfPossible(Node* node)

const Node* ?

> Source/WebCore/page/EventHandler.cpp:2613
> +    for (Node* scrollableNode = node; scrollableNode; scrollableNode = scrollableNode->parentNode()) {
> +        if (scrollableNode->isDocumentNode())
> +            break;
> +        RenderObject* renderer = scrollableNode->renderer();
> +        if (renderer && renderer->isBox() && toRenderBox(renderer)->canBeScrolledAndHasScrollableArea())
> +            return scrollableNode;
> +    }

It would be faster (sometimes measurably faster) if instead of traversing the Render tree upwards, you traversed the Layer tree, given that any scrollable renderer will have an associated  RenderLayer.

What is instead of breaking at a "document" node, do do not continue upwards to the parent document (if any) looking for a scrollable.

if you have nested levels of inner frames (say 3), being the inner one non scrollable and the middle one scrollable, your method would not be able to get the middle scrollable one. See http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp#L202

> Source/WebCore/page/EventHandler.cpp:2633
> +    m_lastHitTestResultOverWidget = result.isOverWidget(); 

I do not think you need this m_lastHitTestResultOverWidget boolean. The Renderer can tell you this as well no? RenderObject::isWidget()

> Source/WebCore/page/EventHandler.cpp:2661
> +    if (passGestureEventToWidgetIfPossible(gestureEvent, latchedRenderer))

IfPossible could be IfNeeded?

> Source/WebCore/page/EventHandler.cpp:2666
> +    node = closestScrollableNodeInDocumentIfPossible(node);

closestScrollableNodeInDocumentOrSelfIfNotScrollable() ? :)

[Antonio Gomes]

(In reply to comment #59)
> (From update of attachment 183069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183069&action=review
> 
> I won't override James' r+, but I have feel comments.

%s/feel/few/gc even

[Terry Anderson]

Comment on attachment 183069 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2605
>> +static Node* closestScrollableNodeInDocumentIfPossible(Node* node)
> 
> const Node* ?

Changed.

>> Source/WebCore/page/EventHandler.cpp:2613
>> +    }
> 
> It would be faster (sometimes measurably faster) if instead of traversing the Render tree upwards, you traversed the Layer tree, given that any scrollable renderer will have an associated  RenderLayer.
> 
> What is instead of breaking at a "document" node, do do not continue upwards to the parent document (if any) looking for a scrollable.
> 
> if you have nested levels of inner frames (say 3), being the inner one non scrollable and the middle one scrollable, your method would not be able to get the middle scrollable one. See http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/InRegionScroller.cpp#L202

I will address your comment about traversing the layer tree in the follow-up patch https://bugs.webkit.org/show_bug.cgi?id=107165

I was unable to reproduce the problem you suggest with three nested iframes. I don't think this will be an issue because closestScrollableNodeInDocumentIfPossible() is meant to only consider the node hierarchy in a single FrameView. The correct targeting of nodes across nested FrameViews is handled when we call passGestureEventToWidgetIfPossible() in handleGestureScrollBegin(), and then the propagation will take effect when we call the same function again in handleGestureScrollUpdate().

>> Source/WebCore/page/EventHandler.cpp:2633
>> +    m_lastHitTestResultOverWidget = result.isOverWidget(); 
> 
> I do not think you need this m_lastHitTestResultOverWidget boolean. The Renderer can tell you this as well no? RenderObject::isWidget()

I agree that this seems redundant. But I am hesitant to remove it since handleWheelEvent() also performs both of these checks (isOverWidget() as well as isWidget()) before calling its passWheelEventToWidget(). I can re-visit this in the follow-up patch https://bugs.webkit.org/show_bug.cgi?id=107165.

>> Source/WebCore/page/EventHandler.cpp:2661
>> +    if (passGestureEventToWidgetIfPossible(gestureEvent, latchedRenderer))
> 
> IfPossible could be IfNeeded?

I think IfPossible is a better description of the function's return value than IfNeeded. I'm going to leave this as is.

>> Source/WebCore/page/EventHandler.cpp:2666
>> +    node = closestScrollableNodeInDocumentIfPossible(node);
> 
> closestScrollableNodeInDocumentOrSelfIfNotScrollable() ? :)

That's a mouthful, but it is indeed a more accurate name. Changed :)

>> LayoutTests/fast/events/touch/gesture/resources/scroll-inside-editable-iframe.html:1
>> +<html>
> 
> add <!DOCTYPE html> above this (same goes for any other new test files) so the file is in standards mode, not quirks mode

Changed all six files.

[Terry Anderson]

Created attachment 183272 [details]
patch to be landed by tdanderson along with bug 106589

[Terry Anderson]

Created attachment 183279 [details]
patch to be landed by tdanderson along with bug 106589

[Terry Anderson]

Created attachment 183294 [details]
patch to be landed by tdanderson along with bug 106589

[WebKit Review Bot]

Comment on attachment 183294 [details]
patch to be landed by tdanderson along with bug 106589

Clearing flags on attachment: 183294

Committed r140177: <http://trac.webkit.org/changeset/140177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.