Bug 102857 – Fix a problem that GestureTap event is not converted to mouse event on the top frame

• Status: UNCONFIRMED
• Component: New Bugs

[Min Qin]

Fix a problem that GestureTap event is not converted to mouse event on the top frame

[Min Qin]

Created attachment 175320 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175320 [details]
Patch

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

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

[Min Qin]

Seems more complicated than I thought if the plugins want to handle GestureTap.
The problem is that there is no way of telling from which node the GestureTap event originates unless we put a static variable in EventHandler to record that.

[Min Qin]

Created attachment 175476 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 175476 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2612
>          return handleGestureTap(gestureEvent);

Can you just check to see if this is in the mainFrame, instead of maintaining [mis-styled] gestureTapOriginatingFrame? i.e.

    return (m_frame->page() && m_frame->page()->mainFrame() != m_frame) ? handleGestureTap(gestureEvent) : false;

[Min Qin]

Comment on attachment 175476 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2612
>>          return handleGestureTap(gestureEvent);
> 
> Can you just check to see if this is in the mainFrame, instead of maintaining [mis-styled] gestureTapOriginatingFrame? i.e.
> 
>     return (m_frame->page() && m_frame->page()->mainFrame() != m_frame) ? handleGestureTap(gestureEvent) : false;

No. If javascript send the touch event to a particular frame, the touch event shouldn't be converted to mouse events in the main frame, but should be converted in the frame where the touch events get dispatched.

[Sadrul Habib Chowdhury]

Comment on attachment 175476 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:2612
>>>          return handleGestureTap(gestureEvent);
>> 
>> Can you just check to see if this is in the mainFrame, instead of maintaining [mis-styled] gestureTapOriginatingFrame? i.e.
>> 
>>     return (m_frame->page() && m_frame->page()->mainFrame() != m_frame) ? handleGestureTap(gestureEvent) : false;
> 
> No. If javascript send the touch event to a particular frame, the touch event shouldn't be converted to mouse events in the main frame, but should be converted in the frame where the touch events get dispatched.

Javascript can't send a *gesture* event to a particular frame, can it? Does any of the platforms convert JS-created touch-events into gesture-events (I know that chromium does not)?

[Robert Kroeger]

(In reply to comment #7)
> (From update of attachment 175476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175476&action=review
> 
> >>> Source/WebCore/page/EventHandler.cpp:2612
> >>>          return handleGestureTap(gestureEvent);
> >> 
> >> Can you just check to see if this is in the mainFrame, instead of maintaining [mis-styled] gestureTapOriginatingFrame? i.e.
> >> 
> >>     return (m_frame->page() && m_frame->page()->mainFrame() != m_frame) ? handleGestureTap(gestureEvent) : false;
> > 
> > No. If javascript send the touch event to a particular frame, the touch event shouldn't be converted to mouse events in the main frame, but should be converted in the frame where the touch events get dispatched.
> 
> Javascript can't send a *gesture* event to a particular frame, can it? Does any of the platforms convert JS-created touch-events into gesture-events (I know that chromium does not)?

It would be a p0 bug if they do. synthetic events (i.e. JS created) are forbidden from implementing default actions.

[Robert Kroeger]

Comment on attachment 175476 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2609
> +        if (gestureTapOriginatingFrame != m_frame)

I don't think that this is the right way to do this.

if (m_frame->page()->mainFrame() == m_frame) { ... }

would be better and eliminate a lot of this other code?

[Min Qin]

Comment on attachment 175476 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2609
>> +        if (gestureTapOriginatingFrame != m_frame)
> 
> I don't think that this is the right way to do this.
> 
> if (m_frame->page()->mainFrame() == m_frame) { ... }
> 
> would be better and eliminate a lot of this other code?

I think if the hittest reaches a target inside the plugin, m_frame->page()->mainFrame() will be the plugin's main frame. So we are not converting the gestureTap to mouse events in the top level frame, but inside the plugin.

>>>>> Source/WebCore/page/EventHandler.cpp:2612
>>>>>          return handleGestureTap(gestureEvent);
>>>> 
>>>> Can you just check to see if this is in the mainFrame, instead of maintaining [mis-styled] gestureTapOriginatingFrame? i.e.
>>>> 
>>>>     return (m_frame->page() && m_frame->page()->mainFrame() != m_frame) ? handleGestureTap(gestureEvent) : false;
>>> 
>>> No. If javascript send the touch event to a particular frame, the touch event shouldn't be converted to mouse events in the main frame, but should be converted in the frame where the touch events get dispatched.
>> 
>> Javascript can't send a *gesture* event to a particular frame, can it? Does any of the platforms convert JS-created touch-events into gesture-events (I know that chromium does not)?
> 
> It would be a p0 bug if they do. synthetic events (i.e. JS created) are forbidden from implementing default actions.

Ok, but i think we still need to record the top frame since m_frame->page()->mainFrame() could return the main frame of a plugin.

[Min Qin]

ping. Dimitri, can you take a look at this change?

[Dimitri Glazkov (Google)]

Comment on attachment 175476 [details]
Patch

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

This patch isn't making EventHandler any more healthy.

> Source/WebCore/ChangeLog:17
> +        It is hard to wrote a test case as we need a new test plugin for this. But this change shouldn't affect the
> +        current layout tests

This seems bad -- don't we need some sort of coverage? How would I know if I broke this fix? Also, no period at the end of the sentence.

> Source/WebCore/page/EventHandler.cpp:151
> +#if ENABLE(GESTURE_EVENTS)

Every time you add an ifdef, there's a kitten that dies somewhere :-\

> Source/WebCore/page/EventHandler.h:465
> +    static Frame* gestureTapOriginatingFrame;

Static frame ptr! That seems nasty. Even though you just use it for comparison, it's a bad smell.

[Adam Barth]

Comment on attachment 175476 [details]
Patch

A static frame pointer makes no sense.