Bug 158531 – [GTK] WebKit2GTK+ does not handle touchmove and touchend events correctly

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Andre Moreira Magalhaes]

- Open with whatever version of WebKit2GTK+ https://patrickhlauke.github.io/touch/tests/event-listener.html
- Touch the "Test button!" and move the finger while touching
- touchstart is received correctly
- no touchmove nor touchend event is received

The same page works fine in Chrome.

[Andre Moreira Magalhaes]

Created attachment 280816 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Andre Moreira Magalhaes]

(In reply to comment #1)
> Created attachment 280816 [details]
> Patch

With this patch it should work the same as Google Chrome. touchupdate/end events are always received by the page even when the page doesnt not handle (prevent) the touchbegin event.

[Carlos Garcia Campos]

Comment on attachment 280816 [details]
Patch

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

Thanks for the patch! Have you run the layout tests? Does this fix any test previously failing? I'm not an expert on touch events, so I'll leave that part to Carlos Garnacho, and I'll just do a more general review.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:339
> +            // Ignored attempt to cancel an END touch event while a BEGIN touch event was processed
> +            // by the gesture controller (wasn't handled by WebProcess), for example because
> +            // scrolling is in progress and cannot be interrupted.

I'm not sure I understand this comment. I think the idea is that we want to pass END touch events to the gesture controller even when the web process handled the event, right? And here we are checking if this is an END handled by the web process while the BEGIN was not handled, right? How do we know that BEGIN was not handled by the web process?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:341
> +            if (gestureController.handleEvent(event.nativeEvent()))
> +                return;

Don't we need to also remove the sequence id from the touch event list?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:346
> +        } else
> +            return;
> +#else
>          return;
> +#endif

I think this could be simplified, if we remove the #else, then we don't need the } else and we avoid the double return there.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:350
>  #if HAVE(GTK_GESTURES)
>      GestureController& gestureController = webkitWebViewBaseGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget));

And here again we handle the event when not handled by the web process. What about something like this?

#if HAVE(GTK_GESTURES)
GestureController& gestureController = webkitWebViewBaseGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget));
if (!wasEventHandled || (touchEvent->type == GDK_TOUCH_END && gestureController.isProcessingGestures()))
    wasEventHandled = gestureController.handleEvent(touchEvent);
#endif

if (wasEventHandled)
    return;

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-995
> -        // If we are already processing gestures is because the WebProcess didn't handle the
> -        // BEGIN touch event, so pass subsequent events to the GestureController.

oh, I guess this comment answers one of my questions above, let's not lose this useful comment then :-)

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:51
> +    // Received touchupdate/end without touchbegin, ignore events

Nit: Comments should finish with period.

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:54
> +        gtk_event_controller_reset(GTK_EVENT_CONTROLLER(m_dragGesture.gesture()));
> +        gtk_event_controller_reset(GTK_EVENT_CONTROLLER(m_zoomGesture.gesture()));

Instead of exposing the internal gesture, i think this would look better if we add a reset() method to Gesture class, so this would be just:

m_dragGesture.reset();
m_zoomGesture.reset();

[Andre Moreira Magalhaes]

Comment on attachment 280816 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:339
>> +            // scrolling is in progress and cannot be interrupted.
> 
> I'm not sure I understand this comment. I think the idea is that we want to pass END touch events to the gesture controller even when the web process handled the event, right? And here we are checking if this is an END handled by the web process while the BEGIN was not handled, right? How do we know that BEGIN was not handled by the web process?

What happens here is that we always want to pass the touchend event to the gesture controller if touchbegin ever got to it. This is for example to avoid starting handling a gesture (touchbegin - e.g. scroll in progress) and not receiving a touchend.
Note that I got this comment from google-chrome (this comment is spit out on console.log if the user tries to prevent touchend (ev.preventDefault()) but didnt prevent touchbegin.

The conditional (if gestureController.isProcessingGestures) is only true if touchbegin reached the gesture controller.

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:341
>> +                return;
> 
> Don't we need to also remove the sequence id from the touch event list?

No, because the sequence id is now always removed in WebViewBase touchEvent handler (when receiving touchend)

The sequence is now: WebViewBase receives the event (adds/remove from touch event list accordingly) -> PageProxy -> WebProcess -> PageClientImpl -> GestureController (it may not get here depending whether the event was handled by the page or not).

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:346
>> +#endif
> 
> I think this could be simplified, if we remove the #else, then we don't need the } else and we avoid the double return there.

Will update it.

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:350
>>      GestureController& gestureController = webkitWebViewBaseGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget));
> 
> And here again we handle the event when not handled by the web process. What about something like this?
> 
> #if HAVE(GTK_GESTURES)
> GestureController& gestureController = webkitWebViewBaseGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget));
> if (!wasEventHandled || (touchEvent->type == GDK_TOUCH_END && gestureController.isProcessingGestures()))
>     wasEventHandled = gestureController.handleEvent(touchEvent);
> #endif
> 
> if (wasEventHandled)
>     return;

I will check this one.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-995
>> -        // BEGIN touch event, so pass subsequent events to the GestureController.
> 
> oh, I guess this comment answers one of my questions above, let's not lose this useful comment then :-)

Yep, I will move this comment to PageClientImpl instead.

>> Source/WebKit2/UIProcess/gtk/GestureController.cpp:51
>> +    // Received touchupdate/end without touchbegin, ignore events
> 
> Nit: Comments should finish with period.

Will update :).

>> Source/WebKit2/UIProcess/gtk/GestureController.cpp:54
>> +        gtk_event_controller_reset(GTK_EVENT_CONTROLLER(m_zoomGesture.gesture()));
> 
> Instead of exposing the internal gesture, i think this would look better if we add a reset() method to Gesture class, so this would be just:
> 
> m_dragGesture.reset();
> m_zoomGesture.reset();

Good idea, I thought about it but the code was already there and I left it as it was :). I will update.

[Carlos Garnacho]

Comment on attachment 280816 [details]
Patch

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

> Source/WebKit2/UIProcess/gtk/GestureController.cpp:58
> +    // Received touchbegin while still processing another touch sequence, cancel previous sequence
> +    if (wasProcessingGestures && event->type == GDK_TOUCH_BEGIN) {

This code branch concerns me somewhat (and it feels pretty central to this approach afaics :( ). 

If I'm reading the current code and your patch correctly, this will just reset gestures if a second/further touch is received, which sounds undesirable for the zoom gesture. It is worth pointing out that there's no right check that we can do here to prevent this case, there's basically no valid assumptions about what the next GdkEventSequence might be.

Also, I find this a bit backwards. If we let webpages capture random events from an specific touchpoint event stream, we'll be potentially left with an inconsistent state in the GestureController until a next TOUCH_BEGIN event is received, which might be much later, or just never.

[Andre Moreira Magalhaes]

Created attachment 281183 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #7)
> Created attachment 281183 [details]
> Patch

This new patch should address Carlos Garcia suggestions.

[Andre Moreira Magalhaes]

(In reply to comment #6)
> Comment on attachment 280816 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280816&action=review
> 
> > Source/WebKit2/UIProcess/gtk/GestureController.cpp:58
> > +    // Received touchbegin while still processing another touch sequence, cancel previous sequence
> > +    if (wasProcessingGestures && event->type == GDK_TOUCH_BEGIN) {
> 
> This code branch concerns me somewhat (and it feels pretty central to this
> approach afaics :( ). 
> 
> If I'm reading the current code and your patch correctly, this will just
> reset gestures if a second/further touch is received, which sounds
> undesirable for the zoom gesture. It is worth pointing out that there's no
> right check that we can do here to prevent this case, there's basically no
> valid assumptions about what the next GdkEventSequence might be.
> 
As for resetting the gestures, there are 2 cases here:
- 1: the gesture controller did not receive a BEGIN touch event (handled by webprocess/page - gesture controller was not processing events) and we ignore any new touch event (UPDATE/END)
- 2: the gesture controller received a BEGIN touch event but did not receive a END touch event (e.g. gesture controller received BEGIN and then another BEGIN event)

> Also, I find this a bit backwards. If we let webpages capture random events
> from an specific touchpoint event stream, we'll be potentially left with an
> inconsistent state in the GestureController until a next TOUCH_BEGIN event
> is received, which might be much later, or just never.
The idea here is actually to not leave the gesture controller in a inconsistent state. We reset the gestures if we get into an inconsistent state as described above.
From what I see, with these changes the gesture controller will never be in a inconsitent state.

From what I see we have the following options:
- The webpage prevents touch BEGIN - in this case the gesture controller will not receive any touch event.
- the webpage do not prevent touch BEGIN but prevents touch UPDATE - in this case the gesture controller will not receive UPDATE events, but will receive the END event
- the webpage cannot prevent a END touch event if it did not prevent a BEGIN touch event - END touch events always reach the gesture controller if a BEGIN touch event was received by it

Please let me know if I am missing something.

[Carlos Garnacho]

(In reply to comment #9)
> (In reply to comment #6)
> > Comment on attachment 280816 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280816&action=review
> > 
> > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:58
> > > +    // Received touchbegin while still processing another touch sequence, cancel previous sequence
> > > +    if (wasProcessingGestures && event->type == GDK_TOUCH_BEGIN) {
> > 
> > This code branch concerns me somewhat (and it feels pretty central to this
> > approach afaics :( ). 
> > 
> > If I'm reading the current code and your patch correctly, this will just
> > reset gestures if a second/further touch is received, which sounds
> > undesirable for the zoom gesture. It is worth pointing out that there's no
> > right check that we can do here to prevent this case, there's basically no
> > valid assumptions about what the next GdkEventSequence might be.
> > 
> As for resetting the gestures, there are 2 cases here:
> - 1: the gesture controller did not receive a BEGIN touch event (handled by
> webprocess/page - gesture controller was not processing events) and we
> ignore any new touch event (UPDATE/END)
> - 2: the gesture controller received a BEGIN touch event but did not receive
> a END touch event (e.g. gesture controller received BEGIN and then another
> BEGIN event)

Again, think about multitouch :), each touchpoint has its own BEGIN/UPDATE.../END lifetime, just with different GdkEventSequences. Receiving a second BEGIN event is not a good indication that any previous touch handling is now stale.

> 
> > Also, I find this a bit backwards. If we let webpages capture random events
> > from an specific touchpoint event stream, we'll be potentially left with an
> > inconsistent state in the GestureController until a next TOUCH_BEGIN event
> > is received, which might be much later, or just never.
> The idea here is actually to not leave the gesture controller in a
> inconsistent state. We reset the gestures if we get into an inconsistent
> state as described above.
> From what I see, with these changes the gesture controller will never be in
> a inconsitent state.
> 
> From what I see we have the following options:
> - The webpage prevents touch BEGIN - in this case the gesture controller
> will not receive any touch event.
> - the webpage do not prevent touch BEGIN but prevents touch UPDATE - in this
> case the gesture controller will not receive UPDATE events, but will receive
> the END event
> - the webpage cannot prevent a END touch event if it did not prevent a BEGIN
> touch event - END touch events always reach the gesture controller if a
> BEGIN touch event was received by it
> 
> Please let me know if I am missing something.

According to http://www.w3.org/TR/touch-events/ :

- The touchstart event: "If the preventDefault method is called on this event, it should prevent any default actions caused by any touch events associated with the same active touch point, including mouse events or scrolling."

- The touchmove event: "If the preventDefault method is called on the *first* touchmove event of an active touch point, it should prevent any default action caused by any touchmove event associated with the same active touch point, such as scrolling."

So, presumably we should honor this last point about the touchmove event, although as far as I can see in https://patrickhlauke.github.io/touch/tests/report.js, that just won't help for the testcase you pasted in the bug description, as there's no trace of preventDefault there. I'd argue that's a bug in the testcase, not WebKit's.

[Andre Moreira Magalhaes]

Created attachment 281268 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #10)
> (In reply to comment #9)
> Again, think about multitouch :), each touchpoint has its own
> BEGIN/UPDATE.../END lifetime, just with different GdkEventSequences.
> Receiving a second BEGIN event is not a good indication that any previous
> touch handling is now stale.
> 
Of course, I made a change that may fix this issue. I am now checking if the same sequence is already being handled by the gesture, and only if so we reset the gesture.

Note that I dont have a touchscreen device here, so I am testing by injecting fake touch events directly and cant test the zoom gesture.

> > 
> > > Also, I find this a bit backwards. If we let webpages capture random events
> > > from an specific touchpoint event stream, we'll be potentially left with an
> > > inconsistent state in the GestureController until a next TOUCH_BEGIN event
> > > is received, which might be much later, or just never.
> > The idea here is actually to not leave the gesture controller in a
> > inconsistent state. We reset the gestures if we get into an inconsistent
> > state as described above.
> > From what I see, with these changes the gesture controller will never be in
> > a inconsitent state.
> > 
> > From what I see we have the following options:
> > - The webpage prevents touch BEGIN - in this case the gesture controller
> > will not receive any touch event.
> > - the webpage do not prevent touch BEGIN but prevents touch UPDATE - in this
> > case the gesture controller will not receive UPDATE events, but will receive
> > the END event
> > - the webpage cannot prevent a END touch event if it did not prevent a BEGIN
> > touch event - END touch events always reach the gesture controller if a
> > BEGIN touch event was received by it
> > 
> > Please let me know if I am missing something.
> 
> According to http://www.w3.org/TR/touch-events/ :
> 
> - The touchstart event: "If the preventDefault method is called on this
> event, it should prevent any default actions caused by any touch events
> associated with the same active touch point, including mouse events or
> scrolling."
> 
> - The touchmove event: "If the preventDefault method is called on the
> *first* touchmove event of an active touch point, it should prevent any
> default action caused by any touchmove event associated with the same active
> touch point, such as scrolling."
> 
The touchstart should already be correct. As for the touchmove, I believe this is (or should be) handled by WebProcess itself (WebCore/page/EventHandler.cpp) and signal that the subsequent touchmove events (if the first one was prevented) are handled by it when invoking the PageClient::doneWithTouchEvent.

> So, presumably we should honor this last point about the touchmove event,
> although as far as I can see in
> https://patrickhlauke.github.io/touch/tests/report.js, that just won't help
> for the testcase you pasted in the bug description, as there's no trace of
> preventDefault there. I'd argue that's a bug in the testcase, not WebKit's.

This test is basically to print the events when using touch sequences on the "Test button". It does not invoke preventDefault on any handler. The problem here is that currently (master) only touchbegin arrives on the page, while the same test works fine (we get touchupdate/end on the page) on both firefox and chrome.

If I understood correctly, the proper behaviour is that all touch events should reach the page but some of them may be prevented and thus not reach the gesture controller.

[Andre Moreira Magalhaes]

Now that I am thinking about it, I remembered why I added the conditional "+    if (wasProcessingGestures && event->type == GDK_TOUCH_BEGIN) {" on GestureController::handleEvent.

This was actually to fix an issue I was having where I was calling ev.preventDefault on touchend from the page and then I would get touchbegin reaching the gesture controller but no touchend. This was later fixed by the change in PlageClientImpl::doneWithTouchEvent where the touchend event gets to the gesture controller even if ev.preventDefault was called on it, as long as touchbegin reached the gesture controller.

In other words, this conditional is only needed in case 2 touchbegin events of the *same* sequence gets to the gesture controller, which should never happen afaik - unless we have a bogus touch device...

I will update the patch to remove this conditional, please let me know what you think.

[Andre Moreira Magalhaes]

Created attachment 281312 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #14)
> Created attachment 281312 [details]
> Patch

Hi Carlos, did you have the chance to check this new patch? Thanks

[Andre Moreira Magalhaes]

Note that bug 155750 changes affect this patch here. We need to also reset the kinetic gesture if the changes there get merged before this one (or update the patch there if this one gets in first).

[Michael Catanzaro]

(In reply to comment #15) 
> Hi Carlos, did you have the chance to check this new patch? Thanks

Hey Carlos Garnacho, got time for another look?

[Brady Eidson]

Comment on attachment 281312 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.

[Adrian Perez]

Created attachment 318679 [details]
Patch

I have updated Andre's patch so ut applies cleanly to trunk

[Carlos Alberto Lopez Perez]

Comment on attachment 318679 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:349
> +    if (!wasEventHandled || (touchEvent->type == GTK_TOUCH_END && gestureController.isProcessingGestures())) {

It fail to build on the EWS, maybe you mean GDK_TOUCH_END here?

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #20)
> Comment on attachment 318679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318679&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:349
> > +    if (!wasEventHandled || (touchEvent->type == GTK_TOUCH_END && gestureController.isProcessingGestures())) {
> 
> It fail to build on the EWS, maybe you mean GDK_TOUCH_END here?

Indeed, I'll fix this and re-upload.

FWIW, it would be good to have Carlos Garnacho review the patch,
my experience with touch events handling is very limited, and while
the changes introduced sound logical in my mind, I would prefer
someone else to check this patch.

[Adrian Perez]

Created attachment 318694 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 318694 [details]
Patch

I confirm this fixes touch events on WebKitGTK+.
I tested this succesfully here: https://people.igalia.com/clopez/wkbug/touchevents/test3.html (that test is broken without this)

r=me

[Carlos Garnacho]

Comment on attachment 318694 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:356
> +        wasEventHandled = gestureController.handleEvent(event.nativeEvent());

IIUC here we bypass wasEventHandled for touchend so the gestureController can undo state in the GtkGestures underneath, even if they were entirely/partially handled by the webpage.

Wouldn't it be clearer to add a reset() method to GestureController that resets all 3 contained gestures and just change this function to have a beginning like this?:

if (wasEventHandled) {
    gestureController.reset();
    return;
}

i.e. resetting the gestureController altogether whenever the webpage handles a touch event. This way all gestures are reset at once instead of touch sequence by touch sequence.

I'm afraid relaying touchend events like this patch does may trigger further action from the GestureController, as eg. GtkGestureSwipe will trigger kinetic scroll upon receiving GDK_TOUCH_END (more on that below).

> Source/WebKit/UIProcess/gtk/GestureController.cpp:53
> +    if (!wasProcessingGestures && (event->type == GDK_TOUCH_UPDATE || event->type == GDK_TOUCH_END)) {

I don't quite understand here that this branch checks for !isProcessingGestures() as opposed to the PageClientImpl::doneWithTouchEvent() change that checks for isProcessingGestures(). Is this unintended? a leftover from the previous approach? I have the impression that the touchend given from doneWithTouchEvent() would not fall through this branch, thus would be fed to the GtkGestures.

And anyway... This looks kinda unnecessary if you go with the GestureController::reset() approach I suggest :)

> Source/WebKit/UIProcess/gtk/GestureController.cpp:55
> +        m_zoomGesture.reset();

FWIW, there is now a m_swipeGesture that should be reset too :).

[Michael Catanzaro]

Comment on attachment 318694 [details]
Patch

OK, thanks for working on this Adrian. Please address Garnacho's comments.

[Adrian Perez]

First of all, thanks a lot again for letting us drag you back to this
patch for reviews — I feel like we are getting closer to landing it

(In reply to Carlos Garnacho from comment #24)
> Comment on attachment 318694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318694&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:356
> > +        wasEventHandled = gestureController.handleEvent(event.nativeEvent());
> 
> IIUC here we bypass wasEventHandled for touchend so the gestureController
> can undo state in the GtkGestures underneath, even if they were
> entirely/partially handled by the webpage.
> 
> Wouldn't it be clearer to add a reset() method to GestureController that
> resets all 3 contained gestures and just change this function to have a
> beginning like this?:
> 
> if (wasEventHandled) {
>     gestureController.reset();
>     return;
> }
> 
> i.e. resetting the gestureController altogether whenever the webpage handles
> a touch event. This way all gestures are reset at once instead of touch
> sequence by touch sequence.

Adding a GestureController::reset() method does indeed sound like a good
idea, and if/when additional gestures are added later there will be a clear
location where to reset its state.

> I'm afraid relaying touchend events like this patch does may trigger further
> action from the GestureController, as eg. GtkGestureSwipe will trigger
> kinetic scroll upon receiving GDK_TOUCH_END (more on that below).
> 
> > Source/WebKit/UIProcess/gtk/GestureController.cpp:53
> > +    if (!wasProcessingGestures && (event->type == GDK_TOUCH_UPDATE || event->type == GDK_TOUCH_END)) {
> 
> I don't quite understand here that this branch checks for
> !isProcessingGestures() as opposed to the
> PageClientImpl::doneWithTouchEvent() change that checks for
> isProcessingGestures(). Is this unintended? a leftover from the previous
> approach? I have the impression that the touchend given from
> doneWithTouchEvent() would not fall through this branch, thus would be fed
> to the GtkGestures.
> 
> And anyway... This looks kinda unnecessary if you go with the
> GestureController::reset() approach I suggest :)

I have tried locally to remove this snippet after adding the ::reset()
method, and it works. IMHO with this simplification the code is more
obvious and doesn't raise eyebrows as much — which is good.

> > Source/WebKit/UIProcess/gtk/GestureController.cpp:55
> > +        m_zoomGesture.reset();
> 
> FWIW, there is now a m_swipeGesture that should be reset too :).

Great catch!

I'll upload a patch in a few minutes with the suggestions applied.

[Adrian Perez]

Created attachment 322671 [details]
Patch

[Adrian Perez]

From #webkitgtk+ in Freenode:

  [17:03] <garnacho> aperezdc: it does look perfect to me code-wise :).

\o/

[WebKit Commit Bot]

Comment on attachment 322671 [details]
Patch

Clearing flags on attachment: 322671

Committed r222855: <http://trac.webkit.org/changeset/222855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.