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).
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?
Created attachment 404231 [details] Patch
Created attachment 404232 [details] Patch
Comment on attachment 404232 [details] Patch The submitted patch is still not correct, I'm working on another version of it.
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
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.
Hi, I took Bastian's patch and prepared another version fixing the bug hopefully the right way :) Regards, Marco
Created attachment 405283 [details] Fix touch based gesture handling
tried the most recent patch on wayland with multitouch touchscreen, works for me.
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()”
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?
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
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(); });
(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
@9moritz@h6t.eu @Bastian Krause Thanks for testing I will fix that remaining issue and will post a new version.
(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.
(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.
(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
(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.
(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
(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
Created attachment 424624 [details] Patch
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
(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 :)
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.