Bug 92914 – Apply target fuzzing when sending a context menu event

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

[Terry Anderson]

If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly adjust the location of a context menu event. This change uses the same set of candidates for touch adjustment as is used for a GestureTap event (which admittedly is a simplifying assumption).

[Terry Anderson]

Created attachment 155902 [details]
Patch

[Terry Anderson]

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

I have not yet added tests, so this patch is only for initial discussion.

[Terry Anderson]

Created attachment 155920 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #0)
> If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly adjust the location of a context menu event. This change uses the same set of candidates for touch adjustment as is used for a GestureTap event (which admittedly is a simplifying assumption).

That is a good start, if we want better targets later we can use the same algorithm but with a more specific filter. 

Specifically though, I guess the current implementation will miss elements with oncontextmenu event handlers.

[Antonio Gomes]

Comment on attachment 155920 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly
> +        adjust the location of a context menu event. This change uses the same set of
> +        candidates for touch adjustment as is used for a GestureTap event (which
> +        admittedly is a simplifying assumption).

I am going to tell you: if you do care about touch and hold entering text selection (i.e. make a #text a target) that won't work as is.

[Allan Sandfeld Jensen]

Comment on attachment 155920 [details]
Patch

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

> Source/WebCore/page/EventHandler.h:173
> +    bool adjustGesturePosition(const PlatformGestureEvent&, IntPoint& adjustedPoint);

You might want to add a Gesture Type parameter to adjustGesturePosition, so it later will be able to choose the correct touch-adjustment algorithm or filter to use.

The parameter might not be used yet, but you could explain that in a FIXME.

[Terry Anderson]

(In reply to comment #6)
> (From update of attachment 155920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155920&action=review
> 
> > Source/WebCore/page/EventHandler.h:173
> > +    bool adjustGesturePosition(const PlatformGestureEvent&, IntPoint& adjustedPoint);
> 
> You might want to add a Gesture Type parameter to adjustGesturePosition, so it later will be able to choose the correct touch-adjustment algorithm or filter to use.
> 
> The parameter might not be used yet, but you could explain that in a FIXME.

Good suggestion. I will add a FIXME to explain this but will hold off on adding the parameter until it is actually going to be used.

[Terry Anderson]

Created attachment 156155 [details]
Patch

[Antonio Gomes]

Comment on attachment 156155 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2542
> +    return (targetNode);

no () needed

> LayoutTests/touchadjustment/touch-links-longpress.html:42
> +    function testLongPress(touchpoint)
> +    {
> +        if (eventSender.gestureLongPress)
> +            eventSender.gestureLongPress(touchpoint.left, touchpoint.top);
> +        else
> +            debug("gestureLongPress not implemented by this platform.");
> +    }

does it mean it will pass on chromium only?

[Terry Anderson]

(In reply to comment #9)
> (From update of attachment 156155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156155&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2542
> > +    return (targetNode);
> 
> no () needed

I will update this in my next patch.

> 
> > LayoutTests/touchadjustment/touch-links-longpress.html:42
> > +    function testLongPress(touchpoint)
> > +    {
> > +        if (eventSender.gestureLongPress)
> > +            eventSender.gestureLongPress(touchpoint.left, touchpoint.top);
> > +        else
> > +            debug("gestureLongPress not implemented by this platform.");
> > +    }
> 
> does it mean it will pass on chromium only?

Since it is in touchadjustment/ it will only be run by chromium and qt. But since I believe qt does not implement gestureLongPress, I expect it to fail. So I will go ahead and skip this in the qt TestExpectations file.

[Terry Anderson]

Created attachment 156164 [details]
Patch

[Antonio Gomes]

Comment on attachment 156164 [details]
Patch

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

> LayoutTests/touchadjustment/touch-links-longpress.html:38
> +        if (eventSender.gestureLongPress)

my previous question was more:

is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc?

[Terry Anderson]

(In reply to comment #12)
> (From update of attachment 156164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review
> 
> > LayoutTests/touchadjustment/touch-links-longpress.html:38
> > +        if (eventSender.gestureLongPress)
> 
> my previous question was more:
> 
> is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc?

I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms.

[Antonio Gomes]

Comment on attachment 156164 [details]
Patch

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

Looks good. I have two questions left:

> Source/WebCore/page/EventHandler.cpp:2473
> +    // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node.

this comment reads  a bit strange to me.

> Source/WebCore/page/EventHandler.cpp:2661
> +    PlatformMouseEvent mouseEvent(adjustedPoint, event.globalPosition(), RightButton, eventType, 1, false, false, false, false, WTF::currentTime());

do we care about adjusting the globalPosition as well?

>>> LayoutTests/touchadjustment/touch-links-longpress.html:38
>>> +        if (eventSender.gestureLongPress)
>> 
>> my previous question was more:
>> 
>> is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc?
> 
> I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms.

Ok.

[Terry Anderson]

(In reply to comment #14)
> (From update of attachment 156164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review
> 
> Looks good. I have two questions left:
> 
> > Source/WebCore/page/EventHandler.cpp:2473
> > +    // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node.
> 
> this comment reads  a bit strange to me.

This refers to the FIXME in EventHandler::handleGestureTap. I have moved the comment and changed the wording to hopefully make it a bit clearer.

> 
> > Source/WebCore/page/EventHandler.cpp:2661
> > +    PlatformMouseEvent mouseEvent(adjustedPoint, event.globalPosition(), RightButton, eventType, 1, false, false, false, false, WTF::currentTime());
> 
> do we care about adjusting the globalPosition as well?

Based on an IRC chat with Allan Jensen (carewolf), so far as we know the global position is not used, so it is not set in this patch or when creating the synthetic mouse events in EventHandler::handleGestureTap.

> 
> >>> LayoutTests/touchadjustment/touch-links-longpress.html:38
> >>> +        if (eventSender.gestureLongPress)
> >> 
> >> my previous question was more:
> >> 
> >> is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc?
> > 
> > I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms.
> 
> Ok.

[Terry Anderson]

Created attachment 156391 [details]
Patch

[Terry Anderson]

Comment on attachment 156391 [details]
Patch

The previous patch received an r+ from tonikitoo. Since there is a chromium change I will also need approval from abarth.

[Terry Anderson]

Comment on attachment 156391 [details]
Patch

I will need to make some small changes now that https://bugs.webkit.org/show_bug.cgi?id=91012 is in the commit queue.

[Adam Barth]

Comment on attachment 156391 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:168
> +        m_area = IntSize(e.boundingBox.width, e.boundingBox.height);

LGTM

[WebKit Review Bot]

Comment on attachment 156391 [details]
Patch

Clearing flags on attachment: 156391

Committed r124642: <http://trac.webkit.org/changeset/124642>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.