Bug 67822 – PlatformGestureEvent::handleGestureEvent should hit test while dispatching Scroll* events.

• Component: UI Events
• Status: RESOLVED INVALID

[Robert Kroeger]

Per the FIXME in the code: EventHandler::handleGestureEvent should hit
test and send ScrollBeginType, ScrollUpdateType and ScrollEndType events
to the correct subframe rather than always sending gestures to the main
frame only. It should also ensure that if a frame gets a gesture begin
gesture, it gets the corresponding end gesture as well.

[Robert Kroeger]

Created attachment 106903 [details]
Patch

[Robert Kroeger]

Hi Adam,

The attached patch works for Chrome but still needs some cleanup and multuple layout tests. I'm hoping you could give me some high-level feedback on the approach and suggest who I should get to review the final version.

Thanks.

[Adam Barth]

Comment on attachment 106903 [details]
Patch

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

I didn't review the whole patch, but this needs a bunch of work.  I'm sorry that I don't have time to help you with this patch.  You might try dglazkov.  He's done a bunch of work with event handling.

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

This looks dangerous.  This point could become junk while dispatching events.  It's probably best to grab the document point from the frame each time you need it.

> Source/WebCore/page/EventHandler.cpp:2226
> +        if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->refCount() > 1 && passGestureEventToWidget(gestureEvent, m_gestureEventWidgetTarget.get()))

Crazy.  It's very rare to branch on the refCount of an object.  I suspect this isn't the right thing to do.

> Source/WebCore/page/EventHandler.cpp:2228
> +        if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->hasOneRef())

No need to call get() to convert a RefPtr to a bool.

> Source/WebCore/page/EventHandler.cpp:2232
> +            RenderLayer* layer = ((RenderBox*)m_gestureTargetNode->renderer())->layer();

Please use C++ style casts.  How do you know its a RenderBox??

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

vPoint => please use full words when naming variables.

> Source/WebCore/page/EventHandler.cpp:2251
> +        Node* node;
> +        bool isOverWidget;

Please initialize scalars.

> Source/WebCore/page/EventHandler.cpp:2257
> +        node = result.innerNode();

Why not just declare node at this point?  Then we don't need to worry about someone using it between when it is declared and when it is initialized.  Also, consider taking a reference to the node so that it doesn't get deallocated out from under you.

> Source/WebCore/page/EventHandler.cpp:2260
> +        if (node) {

Prefer early return.

> Source/WebCore/page/EventHandler.cpp:2269
> +            node = node->shadowAncestorNode();

Why are we re-using the same variable?  /me is confused.

> Source/WebCore/page/EventHandler.cpp:2281
> +            while (target && !target->isRoot()) {
> +                if (target->isBox()) {
> +                    RenderBox* rb = (RenderBox*)target;
> +                    if (rb->canBeScrolledAndHasScrollableArea()) {
> +                        m_gestureTargetNode = rb->node();
> +                        rb->layer()->handleGestureEvent(gestureEvent);
> +                        return true; // FIXME: Prove that this is correct.
> +                    }
> +                }
> +                target = target->parent();

This looks like a lot of render tree manipulation to be doing outside of the rendering directory.

[Robert Kroeger]

Hi Adam,

Thanks for the useful feedback. I know it still needs a lot of work -- I was looking for this kind of early feedback that will stop me going down the wrong rabbit hole.

I'll ask Dimitri about taking a look but maybe you'd have time for some quick follow up questions to your review -- inlined in your comments.

[Adam Barth]

> inlined in your comments.

I don't see your comments.  You might need to "publish" them for other folks to be able to see them.

[Robert Kroeger]

(In reply to comment #5)
> > inlined in your comments.
> 
> I don't see your comments.  You might need to "publish" them for other folks to be able to see them.

Indeed. I forgot to hit publish in another browser window.

[Robert Kroeger]

Comment on attachment 106903 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2226
>> +        if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->refCount() > 1 && passGestureEventToWidget(gestureEvent, m_gestureEventWidgetTarget.get()))
> 
> Crazy.  It's very rare to branch on the refCount of an object.  I suspect this isn't the right thing to do.

my aim was to deal with the case where JS execution disappeared the node between successive PlatformGestureEvents. Is it reasonable to follow the example of  how m_latchedWheelEventNode is handled (I plan on having a layout test for the case of ScrollStart ScrollUpdate* <remove target> ScrollEnd)

>> Source/WebCore/page/EventHandler.cpp:2232
>> +            RenderLayer* layer = ((RenderBox*)m_gestureTargetNode->renderer())->layer();
> 
> Please use C++ style casts.  How do you know its a RenderBox??

Because I only set m_gestureTargetNode if its renderer() was a RenderBox instance.

>> Source/WebCore/page/EventHandler.cpp:2269
>> +            node = node->shadowAncestorNode();
> 
> Why are we re-using the same variable?  /me is confused.

I'll ask Dimitri about the correct way of shadowdom. I was copying from EventHandler::handleWheelEvent.

>> Source/WebCore/page/EventHandler.cpp:2281
>> +                target = target->parent();
> 
> This looks like a lot of render tree manipulation to be doing outside of the rendering directory.

Having a GestureEventWithHitTestResult similar to MouseEventWithHitTestResult would be the right encapsulation of this search? It would reside in /rendering and would appear to get most/all of the necessary functionality from HitTestResult.

[Robert Kroeger]

Created attachment 111852 [details]
Patch

[Robert Kroeger]

Hi Dimitri,

per Adam's suggestion, could you have a look at this and give me some feedback on approach/further issues to address?

[Dimitri Glazkov (Google)]

Comment on attachment 111852 [details]
Patch

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

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

This can move below the !node check.

> Source/WebCore/page/EventHandler.cpp:2238
> +    node = node->shadowAncestorNode();

What are you trying to do there? Can you explain a bit more?

> Source/WebCore/page/EventHandler.cpp:2259
> +    RenderBox* enclosingBox = renderer->enclosingBox();
> +    while (enclosingBox && !enclosingBox->canBeScrolledAndHasScrollableArea()) {
> +        if (!enclosingBox->parent())
> +            return false;
> +        enclosingBox = enclosingBox->parent()->enclosingBox();
> +    }
> +    
> +    if (enclosingBox && enclosingBox->node() != enclosingBox->document()) {
> +        RenderLayer* layer = enclosingBox->layer();
> +        if (layer) {
> +            layer->handleGestureEvent(gestureEvent);
> +            return true;
> +       }
> +    }

looks like this should be something like enclosingScrollableLayer. I bet we already have code that does this.

> Source/WebCore/page/EventHandler.cpp:2271
> +// REVIEWER: I hunted for something like this in wtf::. Does it exist? If not,
> +// is it worth adding a generic version?
> +// Helper to auto-unlatch on end of scope.

Can you explain why you would need something like this?

[Robert Kroeger]

Comment on attachment 111852 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2225
>> +    bool isOverWidget = result.isOverWidget();
> 
> This can move below the !node check.

done, next patch.

>> Source/WebCore/page/EventHandler.cpp:2238
>> +    node = node->shadowAncestorNode();
> 
> What are you trying to do there? Can you explain a bit more?

I was following along with how mouse wheel events would seem to be sent into the shadow DOM -- http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&exact_package=chromium&q=EventHandler&type=cs&l=2184 was the inspiration.

Reasonable or foolish?

>> Source/WebCore/page/EventHandler.cpp:2259
>> +    }
> 
> looks like this should be something like enclosingScrollableLayer. I bet we already have code that does this.

I tried using enclosingScrollableLayer and went on a voyage of discovery. In particular: it does not want to be used from EventHandler. (I would think I was attempting to create a layering violation.) So, I ended up dispatching the events a different way that seems "more correct" -- the code ended up shorter and simpler in the next patch.

>> Source/WebCore/page/EventHandler.cpp:2271
>> +// Helper to auto-unlatch on end of scope.
> 
> Can you explain why you would need something like this?

to always clear the RefPtr on leaving the enclosing scope in handleGestureEvent so that I don't preserve an unnecessary reference to the target node for the Scroll* sequence after the concluding ScrollEndType event.

[Robert Kroeger]

Created attachment 112607 [details]
Patch

[Robert Kroeger]

Please take another look.

Notes:

* rniwa: added per Dimitri's suggestion. Would you be a more appropriate reviewer for this change?

* patch is not complete. Am looking for feedback on the approach/C++ code. The layout tests are not done yet to my satisfaction (they so far only show that the implementation is correct on Chromium) and there are still some missing bits for non-Chromium code paths.

[Ryosuke Niwa]

Comment on attachment 112607 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

You need to explain what kind of changes you're making.

> Source/WebCore/ChangeLog:16
> +        (WebCore::wheelGranularityToScrollGranularity):
> +        Helper method.
> +        (WebCore::scrollNode):
> +        Refactored.

Please put the description on the same line as the function name after :.

> Source/WebCore/page/EventHandler.cpp:178
> +static inline bool gestureNode(const PlatformGestureEvent& e, Node* node)

Please don't use one-letter variable like e.

> Source/WebCore/page/EventHandler.cpp:2238
> +bool EventHandler::passGestureEventToWidgetHelper(PassRefPtr<Node> targetNode, const PlatformGestureEvent& gestureEvent, const HitTestResult& result)

Why does this function need to be a member function of Event Handler? It can be a static non-member function, no?

Why is this function called "helper"? We need a more descriptive name.

> Source/WebCore/page/EventHandler.cpp:2249
> +    if (isOverWidget && target && target->isWidget()) {
> +       Widget* widget = toRenderWidget(target)->widget();
> +        if (widget && passGestureEventToWidget(gestureEvent, widget))

We prefer early exit over nested ifs.

> Source/WebCore/page/EventHandler.cpp:2255
> +bool EventHandler::passGestureEventToFameView(const PlatformGestureEvent& gestureEvent)

Ditto. Also, typo: Fame -> Frame.

> Source/WebCore/page/EventHandler.cpp:2267
> +// Helper to always unlatch target node on leaving scope.
> +class Unlatcher {
> +public:

What's the point of this class? I don't understand what you mean by "unlatch".

Can't you just use RefPtr? What's the point of wrapping it with a class?

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

Please don't use abbreviations like doc.

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

What does v stand for? Please don't use abbreviations.

> Source/WebCore/page/EventHandler.cpp:2303
> +        // FIXME: Refactor this code to not hit test multiple times by using the code
> +        // written for scroll.

Why are you splitting this comment into 2 lines? It fit the line perfectly.

> Source/WebCore/page/EventHandler.cpp:2320
> +        bool xscroll = node && scrollNode(gestureEvent.deltaX(), ScrollByPixel, ScrollLeft, ScrollRight, node, (Node**)0);

Nit: camelCase. xScroll, and s/(Node**)0/0/;

> Source/WebCore/page/EventHandler.cpp:2321
> +        bool yscroll = node && scrollNode(gestureEvent.deltaY(), ScrollByPixel, ScrollUp, ScrollDown, node, (Node**)0);

Ditto.

> Source/WebCore/platform/ScrollAnimator.cpp:138
> +        float deltaX = (event.deltaX() < 0.f) ? max(event.deltaX(),  -float(maxForwardScrollDelta.width())) : min(event.deltaX(), float(maxBackwardScrollDelta.width()));

Nit: two spaces before -float. Also, please use C++-style casting e.g. static_cast<float>().

> Source/WebCore/rendering/RenderBox.cpp:677
> +    fprintf(stderr, "RenderBox::gesture\n");

What's up with this fprintf?

> Source/WebCore/rendering/RenderBox.cpp:678
> +    RenderLayer* layer = layer();

I'm surprised that this didn't cause a compilation error. r- because this is certainly going to cause a compilation failure on one of ports.

[Robert Kroeger]

Created attachment 112759 [details]
Patch

[Robert Kroeger]

Comment on attachment 112607 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> You need to explain what kind of changes you're making.

done in p4

>> Source/WebCore/ChangeLog:16
>> +        Refactored.
> 
> Please put the description on the same line as the function name after :.

done in p4

>> Source/WebCore/page/EventHandler.cpp:178
>> +static inline bool gestureNode(const PlatformGestureEvent& e, Node* node)
> 
> Please don't use one-letter variable like e.

done, p4

>> Source/WebCore/page/EventHandler.cpp:2238
>> +bool EventHandler::passGestureEventToWidgetHelper(PassRefPtr<Node> targetNode, const PlatformGestureEvent& gestureEvent, const HitTestResult& result)
> 
> Why does this function need to be a member function of Event Handler? It can be a static non-member function, no?
> 
> Why is this function called "helper"? We need a more descriptive name.

done, p4

>> Source/WebCore/page/EventHandler.cpp:2249
>> +        if (widget && passGestureEventToWidget(gestureEvent, widget))
> 
> We prefer early exit over nested ifs.

done, p4

>> Source/WebCore/page/EventHandler.cpp:2255
>> +bool EventHandler::passGestureEventToFameView(const PlatformGestureEvent& gestureEvent)
> 
> Ditto. Also, typo: Fame -> Frame.

done, p4. autocomplete for the non-win.

>> Source/WebCore/page/EventHandler.cpp:2267
>> +public:
> 
> What's the point of this class? I don't understand what you mean by "unlatch".
> 
> Can't you just use RefPtr? What's the point of wrapping it with a class?

Background: handleGestureEvent receives a sequence of ScrollDownType, ScrollUpdateType*, ScrollEndType PGE sub-events. The ScrollDownType event chooses the target node and "latches" it in m_gestureTargetNode. Why "latch": from the language in the WheelEvent handling code. The following ScrollUpdateType and ScrollEndType events all go to the original latching node (assuming it still exists.)

The latching node (stored in m_gestureTargetNode) should always be cleared on an ScrollEndType event even in the presence of early return from the containing block. Unlatcher does that. If there is a better way, please let me know.

>> Source/WebCore/page/EventHandler.cpp:2283
>> +    Document* doc = m_frame->document();
> 
> Please don't use abbreviations like doc.

fixed p4

>> Source/WebCore/page/EventHandler.cpp:2292
>> +    LayoutPoint vPoint = view->windowToContents(gestureEvent.position());
> 
> What does v stand for? Please don't use abbreviations.

done in p4

>> Source/WebCore/page/EventHandler.cpp:2303
>> +        // written for scroll.
> 
> Why are you splitting this comment into 2 lines? It fit the line perfectly.

done in p4

>> Source/WebCore/page/EventHandler.cpp:2320
>> +        bool xscroll = node && scrollNode(gestureEvent.deltaX(), ScrollByPixel, ScrollLeft, ScrollRight, node, (Node**)0);
> 
> Nit: camelCase. xScroll, and s/(Node**)0/0/;

done in p4. yScroll too.

>> Source/WebCore/platform/ScrollAnimator.cpp:138
>> +        float deltaX = (event.deltaX() < 0.f) ? max(event.deltaX(),  -float(maxForwardScrollDelta.width())) : min(event.deltaX(), float(maxBackwardScrollDelta.width()));
> 
> Nit: two spaces before -float. Also, please use C++-style casting e.g. static_cast<float>().

done, p4

>> Source/WebCore/rendering/RenderBox.cpp:677
>> +    fprintf(stderr, "RenderBox::gesture\n");
> 
> What's up with this fprintf?

removed.

>> Source/WebCore/rendering/RenderBox.cpp:678
>> +    RenderLayer* layer = layer();
> 
> I'm surprised that this didn't cause a compilation error. r- because this is certainly going to cause a compilation failure on one of ports.

it did. fixed in p4

[Robert Kroeger]

Please take another look.

[Robert Kroeger]

Created attachment 120475 [details]
Patch

[Early Warning System Bot]

Comment on attachment 120475 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 120475 [details]
Patch

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

[Sam Weinig]

Comment on attachment 120475 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.h:120
> +#if ENABLE(GESTURE_EVENTS)
> +    int m_gestureEventTestRecord;
> +#endif

It doesn't seem like a good idea to be adding state to the implementation that is only used for testing.  Perhaps I misunderstand what this is being used for.

[Gustavo Noronha (kov)]

Comment on attachment 120475 [details]
Patch

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

[Collabora GTK+ EWS bot]

Comment on attachment 120475 [details]
Patch

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

[Robert Kroeger]

Comment on attachment 120475 [details]
Patch

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

>> Source/WebCore/platform/ScrollAnimator.h:120
>> +#endif
> 
> It doesn't seem like a good idea to be adding state to the implementation that is only used for testing.  Perhaps I misunderstand what this is being used for.

Sam:

This is what it was for.  Since gesture events don't all reach JavaScript, I needed some way to prove that the implementation was correct.

A solution that occurred to me would be to create a testing subclass of ScrollAnimator for use in DumpRenderTree. Does this sound reasonable? Or is there an easier way? 

BTW: given that this patch addresses your FIXME, do you have any other general advice for improving the patch?

[Robert Kroeger]

Created attachment 121315 [details]
Patch

[Robert Kroeger]

Ryosuke: could you take another look please?

I've addressed weinig's comment but not perhaps in the nicest of ways.

[Gyuyoung Kim]

Comment on attachment 121315 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 121315 [details]
Patch

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

[Robert Kroeger]

Created attachment 121327 [details]
Patch

[Robert Kroeger]

Improved include guards.

[Gyuyoung Kim]

Comment on attachment 121327 [details]
Patch

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

[Robert Kroeger]

Created attachment 121332 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 121332 [details]
Patch

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

[James Robinson]

Comment on attachment 121332 [details]
Patch

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

At least some of the code this patch is patching was deleted a bit back, so this doesn't apply.  If we plan to proceed with this patch I think you need to figure out how it fits in now.

> Source/WebCore/platform/ScrollAnimator.cpp:136
> +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& event)

this was deleted in http://trac.webkit.org/changeset/107823

[Robert Kroeger]

Created attachment 129738 [details]
Patch

[Build Bot]

Comment on attachment 129738 [details]
Patch

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

[James Robinson]

Comment on attachment 129738 [details]
Patch

We discussed this offline. I think we'll be better off if ScrollView / ScrollAnimator / etc do not have to be explicitly aware of GestureEvents, but that we translate the gesture into a logical scroll earlier in the pipeline and then pass that through.

[Robert Kroeger]

per a discussion with jamesr: I'm going to simplify this patch and divide it into three sub-patches that:

*  improve chromium EventSender::gestureEvent to send fully-populated GestureScroll* gestures.

*  show that interim version in ToT using mouse wheel events works correctly for GestureScrollUpdate messages.

*  change to EventHandler::handleGestureEvent to dispatch fling as an additional kind of scroll rather than the approach that I took here.

I'll leave this bug to track.

[Robert Kroeger]

Obsolete bug.