Bug 212324 - [GTK4] Add support for touch events
Summary: [GTK4] Add support for touch events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
: 212325 (view as bug list)
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-05-24 05:51 PDT by Carlos Garcia Campos
Modified: 2021-05-24 07:06 PDT (History)
10 users (show)

See Also:


Attachments
Patch, rewrite GTK gesture support to work similarly in GTK3/4 (75.66 KB, patch)
2021-05-03 10:20 PDT, Carlos Garnacho
no flags Details | Formatted Diff | Diff
Updated patch. (77.70 KB, patch)
2021-05-06 04:31 PDT, Carlos Garnacho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (77.84 KB, patch)
2021-05-06 05:43 PDT, Carlos Garnacho
no flags Details | Formatted Diff | Diff
[fast-cq] Updated patch. (77.78 KB, patch)
2021-05-07 03:08 PDT, Carlos Garnacho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-05-24 05:51:18 PDT
.
Comment 1 Carlos Garnacho 2021-05-03 10:20:23 PDT
Created attachment 427570 [details]
Patch, rewrite GTK gesture support to work similarly in GTK3/4

Here's a (somewhat big) patch that rewrites touch and touchpad gesture handling so it's able to work on both GTK3 and GTK4 with the minimal amount of version-specific code. I believed this is the path of least resistance, rather than introducing a larger maze of "#if USE(GTK4)".
Comment 2 Alexander Mikhaylenko 2021-05-04 00:54:48 PDT
Oh awesome! Even the navigation gesture is ported.

I tried to port it at some point and gave up with how touch sequences work with GTK4. I see this indeed required much bigger changes than what I did.
Comment 3 Alexander Mikhaylenko 2021-05-04 00:59:43 PDT
Comment on attachment 427570 [details]
Patch, rewrite GTK gesture support to work similarly in GTK3/4

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

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:417
> +    graphene_point_t translation = { static_cast<float>(swipingLayerOffset), 0 };

Can we use things like &GRAPHENE_POINT_INIT() in WebKit? Then it could be like

`gtk_snapshot_translate(snapshot, &GRAPHENE_POINT_INIT (whatever, 0));`

Also, any reason to use `static_cast<float>` here but `(float)` below?
Comment 4 Alexander Mikhaylenko 2021-05-04 01:00:22 PDT
There are things like not using Cairo to draw the old page and the drop shadow, but I can port those myself later.
Comment 5 Carlos Garcia Campos 2021-05-04 01:59:39 PDT
Comment on attachment 427570 [details]
Patch, rewrite GTK gesture support to work similarly in GTK3/4

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

Looks great, I'm adding just a few comments. Patch needs to be rebased to apply on current trunk.

> Source/WebKit/ChangeLog:27
> +        No new tests, existing tests should catch regressions.

Unfortunately we no longer run touch event tests, because touch events are now enabled at runtime depending on the hardware. So, we will some need manual testing here.

> Source/WebKit/PlatformGTK.cmake:39
> +list(APPEND WebKit_SOURCES
> +    UIProcess/ViewGestureController.cpp
>  
> -        UIProcess/gtk/ViewGestureControllerGtk.cpp
> +    UIProcess/gtk/ViewGestureControllerGtk.cpp

I think these are here only because they're conditionally added to the buil, so they can be moved to SourceGTK.txt

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:756
> +    auto* pageSnapshot = gtk_snapshot_new();
> +    webViewBase->priv->acceleratedBackingStore->snapshot(pageSnapshot);

Can we do this also depending on isShowingNavigationGestureSnapshot? or isn't it worth it?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:759
> +    auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot);
> +    if (pageRenderNode) {

if (auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot))

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:766
> +    }

Where's pageRenderNode freed?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1497
> +static void appendTouchEvent(WebKitWebViewBase* webViewBase, Vector<WebPlatformTouchPoint>& touchPoints, GdkEvent* event, WebPlatformTouchPoint::TouchPointState state)

Make this receive a GtkWidget* and we avoid two casts below

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1520
> +    GdkEventType type = gdk_event_get_event_type(event);
> +    switch (type) {

switch (gdk_event_get_event_type(event)) {

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1567
> +        if (!priv->touchEvents.size())

if (priv->touchEvents.isEmpty())

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1587
> -        break;
> +        return GDK_EVENT_PROPAGATE;

Do we need to chain up in gtk3?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1843
> +    auto* page = webkitWebViewBaseGetPage(webViewBase);
> +    ASSERT(page);

Use priv->pageProxy directly here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1860
> +    auto* page = webkitWebViewBaseGetPage(webViewBase);
> +    ASSERT(page);

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1866
> +    double pageScale = priv->initialZoomScale * scale;
> +    if (pageScale < 1.0)
> +        pageScale = 1.0;
> +    if (pageScale > 3.0)
> +        pageScale = 3.0;

auto pageScale = clampTo<double>(priv->initialZoomScale * scale, 1, 3);

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1881
> +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> +    priv->isLongPressed = true;

webViewBase->priv->isLongPressed = true;

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1887
> +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> +    priv->isLongPressed = false;

Same here, we don't need the local variable just for a single line

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1890
> +static void webkitWebViewBaseTouchRelease(WebKitWebViewBase* webViewBase, int nPress, double x, double y, GtkGesture*gesture)

GtkGesture*gesture -> GtkGesture* gesture

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1913
> +    GdkModifierType modifiers;
> +#if USE(GTK4)
> +    modifiers = gtk_event_controller_get_current_event_state(GTK_EVENT_CONTROLLER(gesture));
> +#else
> +    gtk_get_current_event_state(&modifiers);
> +#endif

Add gtk_event_controller_get_current_event_state() impl for GTK3 in Source/WebCore/platform/gtk/GtkVersioning.h and we don't need ifdefs here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1915
> +    webkitWebViewBaseSynthesizeMouseEvent(webViewBase, MouseEventType::Motion, 0, 0, x, y, modifiers, nPress, "touch");

Use touchPointerEventType() instead of "touch"

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1944
> +        if (!gtk_drag_check_threshold(GTK_WIDGET(webViewBase), 0, 0, (int)offsetX, (int)offsetY))

Use static_cast<int>()

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1971
> +#if USE(GTK4)
> +            eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture)));
> +#else
> +            eventTime = static_cast<int32_t>(gtk_get_current_event_time());
> +#endif

Add a an implementation of gtk_event_controller_get_current_event_time() for gtk3 too.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2083
> +    priv->touchGestureGroup = gtk_gesture_zoom_new(viewWidget);

In gtk4 ownership is transferred to GtkWidget, but where is this freed in gtk3?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2092
> +    auto* gesture = gtk_gesture_long_press_new(viewWidget);

Same for the gesture here.
Comment 6 Carlos Garnacho 2021-05-06 04:31:26 PDT
Created attachment 427869 [details]
Updated patch.
Comment 7 Carlos Garnacho 2021-05-06 05:04:31 PDT
(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 427570 [details]
> Patch, rewrite GTK gesture support to work similarly in GTK3/4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427570&action=review
> 
> Looks great, I'm adding just a few comments. Patch needs to be rebased to
> apply on current trunk.
> 
> > Source/WebKit/ChangeLog:27
> > +        No new tests, existing tests should catch regressions.
> 
> Unfortunately we no longer run touch event tests, because touch events are
> now enabled at runtime depending on the hardware. So, we will some need
> manual testing here.

Oh, unfortunate, reverted that to "oops".

> 
> > Source/WebKit/PlatformGTK.cmake:39
> > +list(APPEND WebKit_SOURCES
> > +    UIProcess/ViewGestureController.cpp
> >  
> > -        UIProcess/gtk/ViewGestureControllerGtk.cpp
> > +    UIProcess/gtk/ViewGestureControllerGtk.cpp
> 
> I think these are here only because they're conditionally added to the buil,
> so they can be moved to SourceGTK.txt

That worked :).

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:756
> > +    auto* pageSnapshot = gtk_snapshot_new();
> > +    webViewBase->priv->acceleratedBackingStore->snapshot(pageSnapshot);
> 
> Can we do this also depending on isShowingNavigationGestureSnapshot? or
> isn't it worth it?

I left it as is so far. We create and throw away a GtkSnapshot this way, but the operation didn't look expensive, and the resulting render node tree is the same if we're not mid-gesture.

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:759
> > +    auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot);
> > +    if (pageRenderNode) {
> 
> if (auto* pageRenderNode = gtk_snapshot_free_to_node(pageSnapshot))
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:766
> > +    }
> 
> Where's pageRenderNode freed?

Oops, wrong kind of "auto" :), this is handled now.

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1497
> > +static void appendTouchEvent(WebKitWebViewBase* webViewBase, Vector<WebPlatformTouchPoint>& touchPoints, GdkEvent* event, WebPlatformTouchPoint::TouchPointState state)
> 
> Make this receive a GtkWidget* and we avoid two casts below
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1520
> > +    GdkEventType type = gdk_event_get_event_type(event);
> > +    switch (type) {
> 
> switch (gdk_event_get_event_type(event)) {
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1567
> > +        if (!priv->touchEvents.size())
> 
> if (priv->touchEvents.isEmpty())
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1587
> > -        break;
> > +        return GDK_EVENT_PROPAGATE;
> 
> Do we need to chain up in gtk3?
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1843
> > +    auto* page = webkitWebViewBaseGetPage(webViewBase);
> > +    ASSERT(page);
> 
> Use priv->pageProxy directly here.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1860
> > +    auto* page = webkitWebViewBaseGetPage(webViewBase);
> > +    ASSERT(page);
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1866
> > +    double pageScale = priv->initialZoomScale * scale;
> > +    if (pageScale < 1.0)
> > +        pageScale = 1.0;
> > +    if (pageScale > 3.0)
> > +        pageScale = 3.0;
> 
> auto pageScale = clampTo<double>(priv->initialZoomScale * scale, 1, 3);
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1881
> > +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> > +    priv->isLongPressed = true;
> 
> webViewBase->priv->isLongPressed = true;
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1887
> > +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> > +    priv->isLongPressed = false;
> 
> Same here, we don't need the local variable just for a single line
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1890
> > +static void webkitWebViewBaseTouchRelease(WebKitWebViewBase* webViewBase, int nPress, double x, double y, GtkGesture*gesture)
> 
> GtkGesture*gesture -> GtkGesture* gesture
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1913
> > +    GdkModifierType modifiers;
> > +#if USE(GTK4)
> > +    modifiers = gtk_event_controller_get_current_event_state(GTK_EVENT_CONTROLLER(gesture));
> > +#else
> > +    gtk_get_current_event_state(&modifiers);
> > +#endif
> 
> Add gtk_event_controller_get_current_event_state() impl for GTK3 in
> Source/WebCore/platform/gtk/GtkVersioning.h and we don't need ifdefs here.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1915
> > +    webkitWebViewBaseSynthesizeMouseEvent(webViewBase, MouseEventType::Motion, 0, 0, x, y, modifiers, nPress, "touch");
> 
> Use touchPointerEventType() instead of "touch"
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1944
> > +        if (!gtk_drag_check_threshold(GTK_WIDGET(webViewBase), 0, 0, (int)offsetX, (int)offsetY))
> 
> Use static_cast<int>()
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1971
> > +#if USE(GTK4)
> > +            eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture)));
> > +#else
> > +            eventTime = static_cast<int32_t>(gtk_get_current_event_time());
> > +#endif
> 
> Add a an implementation of gtk_event_controller_get_current_event_time() for
> gtk3 too.

Did all these.

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2083
> > +    priv->touchGestureGroup = gtk_gesture_zoom_new(viewWidget);
> 
> In gtk4 ownership is transferred to GtkWidget, but where is this freed in
> gtk3?
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2092
> > +    auto* gesture = gtk_gesture_long_press_new(viewWidget);
> 
> Same for the gesture here.

Ah good point, seems it was leaked previously too. I made all these gestures dispose with the view via weak refs on GTK3.
Comment 8 EWS Watchlist 2021-05-06 05:06:17 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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 9 Carlos Garnacho 2021-05-06 05:43:26 PDT
Created attachment 427876 [details]
Updated patch.
Comment 10 Carlos Garcia Campos 2021-05-07 00:18:15 PDT
Comment on attachment 427876 [details]
Updated patch.

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593
> -        break;
> +        return GDK_EVENT_PROPAGATE;

Do we need to chain up in gtk3?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1954
> +#if USE(GTK4)
> +            eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture)));
> +#else
> +            eventTime = static_cast<int32_t>(gtk_get_current_event_time());
> +#endif

We can remove the ifdef here now

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2065
> +    g_object_weak_ref(G_OBJECT(viewWidget), gestureWeakNotify, priv->touchGestureGroup);

Since we are ignoring the oldLocation parameter, could we use g_object_unref here directly? You probably need a reinterpret_cast. Other solution would be to use g_object_set_data_full
Comment 11 Carlos Garnacho 2021-05-07 02:38:05 PDT
(In reply to Carlos Garcia Campos from comment #10)
> Comment on attachment 427876 [details]
> Updated patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427876&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593
> > -        break;
> > +        return GDK_EVENT_PROPAGATE;
> 
> Do we need to chain up in gtk3?

Oh, missed replying to that in the previous round. In GTK3 this is the handler of the touch_event vmethod in GtkWidgetClass, it should always receive touch events and it shouldn't ever fall through this case. Perhaps it should assert there?

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1954
> > +#if USE(GTK4)
> > +            eventTime = static_cast<int32_t>(gtk_event_controller_get_current_event_time(GTK_EVENT_CONTROLLER(gesture)));
> > +#else
> > +            eventTime = static_cast<int32_t>(gtk_get_current_event_time());
> > +#endif
> 
> We can remove the ifdef here now

Oops, true.

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2065
> > +    g_object_weak_ref(G_OBJECT(viewWidget), gestureWeakNotify, priv->touchGestureGroup);
> 
> Since we are ignoring the oldLocation parameter, could we use g_object_unref
> here directly? You probably need a reinterpret_cast. Other solution would be
> to use g_object_set_data_full

Sure, I passed g_object_unref first but wondered about function casts :). Let me try that.
Comment 12 Carlos Garcia Campos 2021-05-07 02:41:38 PDT
Comment on attachment 427876 [details]
Updated patch.

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

>>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1593
>>> +        return GDK_EVENT_PROPAGATE;
>> 
>> Do we need to chain up in gtk3?
> 
> Oh, missed replying to that in the previous round. In GTK3 this is the handler of the touch_event vmethod in GtkWidgetClass, it should always receive touch events and it shouldn't ever fall through this case. Perhaps it should assert there?

Yes, add ASSERT_NOT_REACHED();
Comment 13 Carlos Garnacho 2021-05-07 03:08:52 PDT
Created attachment 427984 [details]
[fast-cq] Updated patch.

reinterpret_cast still triggered -Wcast-function-type with GWeakNotify, so I went for g_object_set_data_full() instead. This now does the assert and removed the unnecessary #if switch.
Comment 14 Carlos Garcia Campos 2021-05-07 04:15:38 PDT
Comment on attachment 427984 [details]
[fast-cq] Updated patch.

This looks good to me. Alexander, can you confirm everything is working for you?
Comment 15 Alexander Mikhaylenko 2021-05-07 06:59:28 PDT
I don't have a build right now, I can just file issues later instead.
Comment 16 EWS 2021-05-07 07:55:58 PDT
Committed r277172 (237458@main): <https://commits.webkit.org/237458@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427984 [details].
Comment 17 Adrian Perez 2021-05-24 07:06:12 PDT
*** Bug 212325 has been marked as a duplicate of this bug. ***