Bug 158531 - [GTK] WebKit2GTK+ does not handle touchmove and touchend events correctly
Summary: [GTK] WebKit2GTK+ does not handle touchmove and touchend events correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-08 10:51 PDT by Andre Moreira Magalhaes
Modified: 2017-10-04 10:33 PDT (History)
12 users (show)

See Also:


Attachments
Patch (6.40 KB, patch)
2016-06-08 11:10 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2016-06-13 11:33 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2016-06-14 11:36 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2016-06-14 18:59 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2017-08-21 15:28 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2017-08-21 16:41 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2017-10-04 07:55 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2016-06-08 10:51:16 PDT
- 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.
Comment 1 Andre Moreira Magalhaes 2016-06-08 11:10:49 PDT
Created attachment 280816 [details]
Patch
Comment 2 WebKit Commit Bot 2016-06-08 11:12:12 PDT
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
Comment 3 Andre Moreira Magalhaes 2016-06-08 11:13:02 PDT
(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.
Comment 4 Carlos Garcia Campos 2016-06-08 23:47:15 PDT
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();
Comment 5 Andre Moreira Magalhaes 2016-06-09 09:56:59 PDT
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.
Comment 6 Carlos Garnacho 2016-06-13 10:54:20 PDT
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.
Comment 7 Andre Moreira Magalhaes 2016-06-13 11:33:35 PDT
Created attachment 281183 [details]
Patch
Comment 8 Andre Moreira Magalhaes 2016-06-13 11:34:15 PDT
(In reply to comment #7)
> Created attachment 281183 [details]
> Patch

This new patch should address Carlos Garcia suggestions.
Comment 9 Andre Moreira Magalhaes 2016-06-13 11:43:25 PDT
(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.
Comment 10 Carlos Garnacho 2016-06-13 12:57:38 PDT
(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.
Comment 11 Andre Moreira Magalhaes 2016-06-14 11:36:04 PDT
Created attachment 281268 [details]
Patch
Comment 12 Andre Moreira Magalhaes 2016-06-14 11:44:15 PDT
(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.
Comment 13 Andre Moreira Magalhaes 2016-06-14 18:55:23 PDT
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.
Comment 14 Andre Moreira Magalhaes 2016-06-14 18:59:14 PDT
Created attachment 281312 [details]
Patch
Comment 15 Andre Moreira Magalhaes 2016-06-20 07:59:44 PDT
(In reply to comment #14)
> Created attachment 281312 [details]
> Patch

Hi Carlos, did you have the chance to check this new patch? Thanks
Comment 16 Andre Moreira Magalhaes 2016-06-24 09:00:52 PDT
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).
Comment 17 Michael Catanzaro 2016-07-10 13:38:56 PDT
(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?
Comment 18 Brady Eidson 2017-08-19 16:01:47 PDT
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.
Comment 19 Adrian Perez 2017-08-21 15:28:33 PDT
Created attachment 318679 [details]
Patch

I have updated Andre's patch so ut applies cleanly to trunk
Comment 20 Carlos Alberto Lopez Perez 2017-08-21 16:20:48 PDT
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?
Comment 21 Adrian Perez 2017-08-21 16:40:40 PDT
(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.
Comment 22 Adrian Perez 2017-08-21 16:41:32 PDT
Created attachment 318694 [details]
Patch
Comment 23 Carlos Alberto Lopez Perez 2017-10-03 09:39:32 PDT
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
Comment 24 Carlos Garnacho 2017-10-03 14:56:53 PDT
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 :).
Comment 25 Michael Catanzaro 2017-10-04 03:39:51 PDT
Comment on attachment 318694 [details]
Patch

OK, thanks for working on this Adrian. Please address Garnacho's comments.
Comment 26 Adrian Perez 2017-10-04 07:51:42 PDT
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.
Comment 27 Adrian Perez 2017-10-04 07:55:13 PDT
Created attachment 322671 [details]
Patch
Comment 28 Adrian Perez 2017-10-04 08:19:31 PDT
From #webkitgtk+ in Freenode:

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

\o/
Comment 29 WebKit Commit Bot 2017-10-04 10:33:30 PDT
Comment on attachment 322671 [details]
Patch

Clearing flags on attachment: 322671

Committed r222855: <http://trac.webkit.org/changeset/222855>
Comment 30 WebKit Commit Bot 2017-10-04 10:33:32 PDT
All reviewed patches have been landed.  Closing bug.