Bug 212324 – [GTK4] Add support for touch events

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

.

[Carlos Garnacho]

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)".

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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?

[Alice Mikhaylenko]

There are things like not using Cairo to draw the old page and the drop shadow, but I can port those myself later.

[Carlos Garcia Campos]

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.

[Carlos Garnacho]

Created attachment 427869 [details]
Updated patch.

[Carlos Garnacho]

(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.

[EWS Watchlist]

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

[Carlos Garnacho]

Created attachment 427876 [details]
Updated patch.

[Carlos Garcia Campos]

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

[Carlos Garnacho]

(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.

[Carlos Garcia Campos]

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();

[Carlos Garnacho]

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.

[Carlos Garcia Campos]

Comment on attachment 427984 [details]
[fast-cq] Updated patch.

This looks good to me. Alexander, can you confirm everything is working for you?

[Alice Mikhaylenko]

I don't have a build right now, I can just file issues later instead.

[EWS]

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].

[Adrian Perez]

*** Bug 212325 has been marked as a duplicate of this bug. ***