Bug 214141 – [WPE] lost touch events, wrong mousedown events

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Bastian Krause]

touchend (and some corresponding events, see below) do not trigger on touch release right after a touch motion (scroll gesture). This seems to be the case because the wpe_input_touch_event_type_up does only end the gesture, but is not handled as a general touch event in this case.

backend: wpebackend-fdo-1.6.1
wpewebkit: 2.28.2 (tarball from https://wpewebkit.org/releases/)
browser: cog 0.7.1, qt-wpe-simple-browser 0.2

How to reproduce:
Visit https://patrickhlauke.github.io/touch/tests/event-listener.html, touch the button, move finger (while still touching) on button (or off button) and release touch.

Expected behavior:
touchstart
mouseover (15ms)
mouseenter (1ms)
mousedown (0ms)
mousemove (35ms)
touchmove (128ms)
touchmove (1ms)
touchend (8ms)
mouseup (10ms)
click (0ms)
mouseout
mouseleave (3ms)

Observed behavior:
touchstart
mouseover (15ms)
mouseenter (1ms)
mousedown (1ms)
mousemove (35ms)
touchmove (128ms)
touchmove (1ms)

Similar bugs:
- https://bugs.webkit.org/show_bug.cgi?id=202527

Proposed fix:
ScrollGestureController::handleEvent (Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp) could always return false for wpe_input_touch_event_type_up cases. This way the wpe_input_touch_event_type_up would not only end the scroll gesture (Source/WebKit/UIProcess/API/wpe/WPEView.cpp) but also be processed as any other touch up event leading to correct touchend events in javascript. I am not sure whether this is really the desired fix though but it produces the desired output (as seen in "Expected behavior" above).

[Bastian Krause]

The proposed fix (which is what I posted as "expected behavior") leads to a click event for "touch the button, move finger (while still touching) on button (or off button) and release touch". This is wrong, such a "gesture" should not lead to a click event. The proposed fix is therefore not correct, the "expected behavior" minus the click event is still correct though.

Any ideas?

[Bastian Krause]

Created attachment 404231 [details]
Patch

[Bastian Krause]

Created attachment 404232 [details]
Patch

[Bastian Krause]

Comment on attachment 404232 [details]
Patch

The submitted patch is still not correct, I'm working on another version of it.

[Bastian Krause]

Some aspects of this bug became clear to me after investigation, so I'll rephrase the bug report and observed/expected behavior:

- touchend does not trigger on scroll gestures
- touchmove/mousemove do not trigger after 200ms on scroll gestures (threshold to detect a scroll motion defined in Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp)
- mousedown triggers on scroll gestures, this is incorrect
- mousedown triggers on simple "click" touches prior to touchend, this is incorrect

Expected behavior for simple "click" touch:
touchstart
mouseover
mouseenter
mousemove
touchend
mousedown
mouseup
click

Observed behavior for simple "click" touch:
touchstart
mouseover
mouseenter
mousedown
mousemove
touchend
mouseup
click

Expected behavior for scroll gesture:
touchstart
mouseover
mouseenter
mousemove
touchmove
touchmove
touchmove
touchend

Observed behavior for scroll gesture:
touchstart
mouseover
mouseenter
mousedown
mousemove
touchmove
touchmove

[Adrian Perez]

In the WPE chat room it has been mentioned (thanks madmo!) that the slider
widget (https://material-ui.com/components/slider/) from the Material-UI
library behaves incorrectly and it seems that this issue could be related.

[Marco Felsch]

Hi,

I took Bastian's patch and prepared another version fixing the bug hopefully the right way :)

Regards,
  Marco

[Marco Felsch]

Created attachment 405283 [details]
Fix touch based gesture handling

tried the most recent patch on wayland with multitouch touchscreen, works for me.

[Adrian Perez]

Comment on attachment 405283 [details]
Fix touch based gesture handling

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

Patch looks correct to me from the point of view of my limited
knowledge about how touch input is handled, so I would prefer if
someone else can also review this—I'll try and fish around another
reviewer 🎣️ 

I have only a couple of comments with minor code style issues,
please check the comments below.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:35
> +WebKit::ScrollGestureController::TouchPoint ScrollGestureController::getMouseClickTouchId()

This is returning a “TouchPoint” instance so maybe something like
“::lastMouseTouchPoint()” would be a better name for this method.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:40
> +bool ScrollGestureController::pendingMouseClicks()

For methods that return booleans in WebKit we put a “has” or “is”
prefix. Here I would rename this to “::hasPendingMouseClicks()”

[Zan Dobersek]

Comment on attachment 405283 [details]
Fix touch based gesture handling

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

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.h:78
> +    Vector<struct TouchPoint, 10> m_mouseClicks;

Just 'Vector<TouchPoint, 10> is enough. struct/class notation (beyond forward declarations and definitions) is ignored in the WebKit C++ coding style.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
> +            scrollGestureController.handleEvent(touchPoint, page);
>              page.handleTouchEvent(touchEvent);

ScrollGestureController::handleEvent() returns a boolean, specifically true if the event was finally handled as a wheel event. If that's the case, should the original event still be handled as a touch event?

[Marco Felsch]

Comment on attachment 405283 [details]
Fix touch based gesture handling

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

>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:35
>> +WebKit::ScrollGestureController::TouchPoint ScrollGestureController::getMouseClickTouchId()
> 
> This is returning a “TouchPoint” instance so maybe something like
> “::lastMouseTouchPoint()” would be a better name for this method.

Yep, I will change that.

>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:40
>> +bool ScrollGestureController::pendingMouseClicks()
> 
> For methods that return booleans in WebKit we put a “has” or “is”
> prefix. Here I would rename this to “::hasPendingMouseClicks()”

Okay, Thanks for the input :)

>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.h:78
>> +    Vector<struct TouchPoint, 10> m_mouseClicks;
> 
> Just 'Vector<TouchPoint, 10> is enough. struct/class notation (beyond forward declarations and definitions) is ignored in the WebKit C++ coding style.

Arg.. Thanks for covering that :)

>> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
>>              page.handleTouchEvent(touchEvent);
> 
> ScrollGestureController::handleEvent() returns a boolean, specifically true if the event was finally handled as a wheel event. If that's the case, should the original event still be handled as a touch event?

Yes we do and this is line is the main reason for this patch. Since the shortcut we don't handle it anymore but we need to.

With this patch applied there are some problems with click events on touchend, even if the element was not target of the touchstart.

Here is a sample site to reproduce: https://jsfiddle.net/h013paz9/show

[Bastian Krause]

I am also able to reproduce the bug mentioned by Moritz, triggered by https://jsfiddle.net/h013paz9/show with the latest patch.

Another click event related difference (probably unrelated to this bug?), unrelated to the latest patch:

On desktop Chromium and Android Chrome I observe no click events after long presses > ~2s (tap and hold, with [1]), on wpewebkit click events occur no matter how long the tap and hold gesture lasts.

I cannot reproduce this on mobile Safari as tap and hold gestures make the select function pop up, leading to the end of the gesture without click.

[1] window.addEventListener("contextmenu", function(e) { e.preventDefault(); });

[Pablo Saavedra]

(In reply to Bastian Krause from comment #5)
> Some aspects of this bug became clear to me after investigation, so I'll
...
> Expected behavior for scroll gesture:
> touchstart
> mouseover
> mouseenter
> mousemove
> touchmove
> touchmove
> touchmove
> touchend
> 
> Observed behavior for scroll gesture:
> touchstart
> mouseover
> mouseenter
> mousedown
> mousemove
> touchmove
> touchmove

Even after applying this patch, I still can reproduce this wrong behavior in https://people.igalia.com/psaavedra/scroll.html

[Marco Felsch]

@[e-mail] @Bastian Krause
Thanks for testing I will fix that remaining issue and will post a new version.

[Marco Felsch]

(In reply to Pablo Saavedra from comment #15)
> (In reply to Bastian Krause from comment #5)
> > Some aspects of this bug became clear to me after investigation, so I'll
> ...
> > Expected behavior for scroll gesture:
> > touchstart
> > mouseover
> > mouseenter
> > mousemove
> > touchmove
> > touchmove
> > touchmove
> > touchend
> > 
> > Observed behavior for scroll gesture:
> > touchstart
> > mouseover
> > mouseenter
> > mousedown
> > mousemove
> > touchmove
> > touchmove
> 
> Even after applying this patch, I still can reproduce this wrong behavior in
> https://people.igalia.com/psaavedra/scroll.html

Hm.. I don't see any wrong behavior here.

[Pablo Saavedra]

(In reply to Marco Felsch from comment #17)
> (In reply to Pablo Saavedra from comment #15)
> > (In reply to Bastian Krause from comment #5)
> > > Some aspects of this bug became clear to me after investigation, so I'll
> > ...
> > > Expected behavior for scroll gesture:
> > > touchstart
> > > mouseover
> > > mouseenter
> > > mousemove
> > > touchmove
> > > touchmove
> > > touchmove
> > > touchend
> > > 
> > > Observed behavior for scroll gesture:
> > > touchstart
> > > mouseover
> > > mouseenter
> > > mousedown
> > > mousemove
> > > touchmove
> > > touchmove
> > 
> > Even after applying this patch, I still can reproduce this wrong behavior in
> > https://people.igalia.com/psaavedra/scroll.html
> 
> Hm.. I don't see any wrong behavior here.

Do you mean in the link? I've tested this in the WPE could be that hit a difference.

[Marco Felsch]

(In reply to Pablo Saavedra from comment #18)
> (In reply to Marco Felsch from comment #17)
> > (In reply to Pablo Saavedra from comment #15)
> > > (In reply to Bastian Krause from comment #5)
> > > > Some aspects of this bug became clear to me after investigation, so I'll
> > > ...
> > > > Expected behavior for scroll gesture:
> > > > touchstart
> > > > mouseover
> > > > mouseenter
> > > > mousemove
> > > > touchmove
> > > > touchmove
> > > > touchmove
> > > > touchend
> > > > 
> > > > Observed behavior for scroll gesture:
> > > > touchstart
> > > > mouseover
> > > > mouseenter
> > > > mousedown
> > > > mousemove
> > > > touchmove
> > > > touchmove
> > > 
> > > Even after applying this patch, I still can reproduce this wrong behavior in
> > > https://people.igalia.com/psaavedra/scroll.html
> > 
> > Hm.. I don't see any wrong behavior here.
> 
> Do you mean in the link? I've tested this in the WPE could be that hit a
> difference.

I've tested it with latest WPE+cog+wayland+fdo too and see no wrong behavior

[Marco Felsch]

(In reply to Bastian Krause from comment #14)
> I am also able to reproduce the bug mentioned by Moritz, triggered by
> https://jsfiddle.net/h013paz9/show with the latest patch.
> 
> Another click event related difference (probably unrelated to this bug?),
> unrelated to the latest patch:
> 
> On desktop Chromium and Android Chrome I observe no click events after long
> presses > ~2s (tap and hold, with [1]), on wpewebkit click events occur no
> matter how long the tap and hold gesture lasts.
> 
> I cannot reproduce this on mobile Safari as tap and hold gestures make the
> select function pop up, leading to the end of the gesture without click.
> 
> [1] window.addEventListener("contextmenu", function(e) { e.preventDefault();
> });

After checking the GTK implementation and the common code, I don't see how we could fix this in short. The problem is that we need to track the element with the touch_down event. Then we need to check if the touch_up event correspond to the same element and then issuing a click() event. Question is also, do we need to synthesize the click event? According https://w3c.github.io/uievents/#click this is not the case.

[Pablo Saavedra]

(In reply to Marco Felsch from comment #19)
> (In reply to Pablo Saavedra from comment #18)
> > (In reply to Marco Felsch from comment #17)
> > > (In reply to Pablo Saavedra from comment #15)
> > > > (In reply to Bastian Krause from comment #5)
> > > > > Some aspects of this bug became clear to me after investigation, so I'll
> > > > ...
> > > > > Expected behavior for scroll gesture:
> > > > > touchstart
> > > > > mouseover
> > > > > mouseenter
> > > > > mousemove
> > > > > touchmove
> > > > > touchmove
> > > > > touchmove
> > > > > touchend
> > > > > 
> > > > > Observed behavior for scroll gesture:
> > > > > touchstart
> > > > > mouseover
> > > > > mouseenter
> > > > > mousedown
> > > > > mousemove
> > > > > touchmove
> > > > > touchmove
> > > > 
> > > > Even after applying this patch, I still can reproduce this wrong behavior in
> > > > https://people.igalia.com/psaavedra/scroll.html
> > > 
> > > Hm.. I don't see any wrong behavior here.
> > 
> > Do you mean in the link? I've tested this in the WPE could be that hit a
> > difference.
> 
> I've tested it with latest WPE+cog+wayland+fdo too and see no wrong behavior

I did the tests weeks ago I don't remember exactly what I observed in that moment. I tested with master/main versions of WPE, wpebackend-fdo and libwpe. I will try it again a get you a more precise analysis.

Something interesting to highlight and something that I remember is that
https://bugs.webkit.org/show_bug.cgi?id=218903 introduces some changes in the logic. That simple change [1] inhibits the ScrollGestureController handling and that makes the kinetic scrolling doesn't work anymore for scrolling using touch gestures. But not sure how that bug it could be related with this one.  

[1] https://bugs.webkit.org/attachment.cgi?id=414342&action=prettypatch

[Marco Felsch]

(In reply to Marco Felsch from comment #20)
> (In reply to Bastian Krause from comment #14)
> > I am also able to reproduce the bug mentioned by Moritz, triggered by
> > https://jsfiddle.net/h013paz9/show with the latest patch.
> > 
> > Another click event related difference (probably unrelated to this bug?),
> > unrelated to the latest patch:
> > 
> > On desktop Chromium and Android Chrome I observe no click events after long
> > presses > ~2s (tap and hold, with [1]), on wpewebkit click events occur no
> > matter how long the tap and hold gesture lasts.
> > 
> > I cannot reproduce this on mobile Safari as tap and hold gestures make the
> > select function pop up, leading to the end of the gesture without click.
> > 
> > [1] window.addEventListener("contextmenu", function(e) { e.preventDefault();
> > });
> 
> After checking the GTK implementation and the common code, I don't see how
> we could fix this in short. The problem is that we need to track the element
> with the touch_down event. Then we need to check if the touch_up event
> correspond to the same element and then issuing a click() event. Question is
> also, do we need to synthesize the click event? According
> https://w3c.github.io/uievents/#click this is not the case.

So.. after checking the GTK Port I assume that it is okay, that presses longer than 500ms should not trigger the click() event. Why 500ms? Because it is default for 'gtk-long-press' [1]. The interesting code can be found here [2]. The .tap() is only fired if not inDrag. Behind the tap() implementation is the synthesized click() event. Since we don't have properties like 'gtk-long-press' I would go with 500ms. Is that okay for everyone?

[1] https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-long-press-time
[2] Source/WebKit/UIProcess/gtk/GestureController.cpp

[Marco Felsch]

Created attachment 424624 [details]
Patch

[Pablo Saavedra]

looks like the patch is failing to be applied:

perl Tools/Scripts/svn-apply --force .buildbot-diff
 in dir /home/ews/worker/WPE-EWS/build (timeout 600 secs)
 watching logfiles {}
 argv: ['perl', 'Tools/Scripts/svn-apply', '--force', '.buildbot-diff']
 using PTY: False
Parsed 5 diffs from patch file(s).
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp
Hunk #1 FAILED at 204.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp.rej
patching file Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 49.
Hunk #3 succeeded at 94 with fuzz 1 (offset 25 lines).
Hunk #4 FAILED at 90.
3 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp.rej
patching file Source/WebKit/UIProcess/API/wpe/ScrollGestureController.h
Hunk #1 succeeded at 27 with fuzz 2.
Hunk #2 FAILED at 47.
Hunk #3 FAILED at 70.
2 out of 3 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/API/wpe/ScrollGestureController.h.rej
patching file Source/WebKit/UIProcess/API/wpe/WPEView.cpp
Hunk #1 succeeded at 194 (offset 22 lines).
program finished with exit code 1
elapsedTime=0.178097

[Marco Felsch]

(In reply to Pablo Saavedra from comment #24)
> looks like the patch is failing to be applied:

You're right. I didn't rebased it against master, since I've tested it on 2.30.6... I wanted to update a new version but there where a few changes on master and I didn't had the time to fix the merge issues.

I will update a rebased version as soon as possible :)

[Bastian Krause]

The bugs mentioned here were fixed by

https://bugs.webkit.org/show_bug.cgi?id=247975
https://bugs.webkit.org/show_bug.cgi?id=248076

Setting this to resolved. Thanks for the feedback.