Skip to content

Commit

Permalink
Cherry-pick 273922@main (fd1f0b7). https://bugs.webkit.org/show_bug.c…
Browse files Browse the repository at this point in the history
…gi?id=261348

    [GTK] Remove key event reinjection
    https://bugs.webkit.org/show_bug.cgi?id=261348

    Reviewed by Carlos Garcia Campos.

    Event processing in GTK is synchronous, but in WebKit it is asynchronous
    because we don't want to block the UI process waiting for the web
    process to decide whether a DOM event has been handled (e.g. by using
    Event.stopPropagation). Currently we use a complicated scheme to
    synthesize and reinject new key and wheel events to continue event
    processing when the event is not handled by the web content, which the
    GTK developers do not approve of, and which is causing serious problems
    where Epiphany has to choose between processing events in an infinite
    loop vs. handling events too soon and blocking the web view from
    receiving them. Neither option is great.

    https://gitlab.gnome.org/GNOME/epiphany/-/issues/1915
    https://gitlab.gnome.org/GNOME/epiphany/-/issues/2173

    The solution is to just never reinject events, and instead always handle
    them. We do not need event reinjection anymore if we activate
    application accelerators manually. See the long comment embedded in this
    commit for full details of why this is necessary.

    This patch removes the key event reinjection. Wheel event reinjection is
    not yet removed because this would break when WebKit is used within
    scrolled windows.

     * Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:
     (WebKit::PageClientImpl::doneWithKeyEvent):
     * Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:
     (shouldForwardKeyEvent):
     (webkitWebViewBaseProcessAcceleratorsForKeyPressEvent):
     (webkitWebViewBaseKeyPressed):
     (handleScroll):
     (webkitWebViewBasePropagateKeyEvent):
     * Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:

    Canonical link: https://commits.webkit.org/273922@main
  • Loading branch information
mcatanzaro authored and aperezdc committed Feb 6, 2024
1 parent 1ddd227 commit 3d84c94
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 24 deletions.
4 changes: 4 additions & 0 deletions Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,11 @@ void PageClientImpl::doneWithKeyEvent(const NativeWebKeyboardEvent& event, bool
}

WebKitWebViewBase* webkitWebViewBase = WEBKIT_WEB_VIEW_BASE(m_viewWidget);
#if USE(GTK4)
webkitWebViewBaseProcessAcceleratorsForKeyPressEvent(webkitWebViewBase, event.nativeEvent());
#else
webkitWebViewBasePropagateKeyEvent(webkitWebViewBase, event.nativeEvent());
#endif
}

RefPtr<WebPopupMenuProxy> PageClientImpl::createPopupMenuProxy(WebPageProxy& page)
Expand Down
81 changes: 57 additions & 24 deletions Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,6 @@ struct _WebKitWebViewBasePrivate {
RefPtr<WebPageProxy> pageProxy;
IntSize viewSize { };
#if USE(GTK4)
Vector<GRefPtr<GdkEvent>> keyEventsToPropagate;
Vector<GRefPtr<GdkEvent>> wheelEventsToPropagate;
#else
bool shouldForwardNextKeyEvent { false };
Expand Down Expand Up @@ -994,15 +993,6 @@ static void webkitWebViewBaseUnmap(GtkWidget* widget)
webkitWebViewBaseScheduleUpdateActivityState(webViewBase, ActivityState::IsVisible);
}

static bool shouldForwardKeyEvent(WebKitWebViewBase* webViewBase, GdkEvent* event)
{
#if USE(GTK4)
return event && webViewBase->priv->keyEventsToPropagate.removeFirst(event);
#else
return std::exchange(webViewBase->priv->shouldForwardNextKeyEvent, false);
#endif
}

static bool shouldForwardWheelEvent(WebKitWebViewBase* webViewBase, GdkEvent* event)
{
#if USE(GTK4)
Expand Down Expand Up @@ -1051,6 +1041,11 @@ static gboolean webkitWebViewBaseFocusOutEvent(GtkWidget* widget, GdkEventFocus*
return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->focus_out_event(widget, event);
}

static bool shouldForwardKeyEvent(WebKitWebViewBase* webViewBase, GdkEvent* event)
{
return std::exchange(webViewBase->priv->shouldForwardNextKeyEvent, false);
}

static gboolean webkitWebViewBaseKeyPressEvent(GtkWidget* widget, GdkEventKey* keyEvent)
{
WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(widget);
Expand Down Expand Up @@ -1133,18 +1128,60 @@ static void webkitWebViewBaseFocusLeave(WebKitWebViewBase* webViewBase, GtkEvent
webViewBase->priv->inputMethodFilter.notifyFocusedOut();
}

void webkitWebViewBaseProcessAcceleratorsForKeyPressEvent(WebKitWebViewBase* webViewBase, GdkEvent* event)
{
// Event processing in GTK is synchronous, but in WebKit it is asynchronous because we don't
// want to block the UI process waiting for the web process to decide whether a DOM event has
// been handled (e.g. by using Event.stopPropagation). In GTK 3, we used a complicated scheme to
// synthesize and reinject new key and wheel events to continue event processing when the event
// is not handled by the web content, which the GTK developers did not approve of. We originally
// attempted this for GTK 4 as well, but it never worked properly, and GTK's support for event
// injection has been deprecated. Instead, the WebKitWebViewBase now always handles all events
// (by always returning GDK_EVENT_STOP).
//
// Web browsers must forward most key events to the web view for processing before they are
// processed by the window, because processing key press events on the window will activate
// action accelerators, but web content should be allowed to override most accelerators. E.g.
// on Google Docs Ctrl+O should open Google's document chooser, not the browser's file chooser,
// and Ctrl+F should open Google's find, not the browser's. See key_pressed_cb and
// should_view_receive_key_press_event_first in Epiphany's ephy-window.c for what this looks
// like on the browser side. (Those functions contain a comment that refers to this code in
// WebKit, so please update the comment there if needed when changing the code here.)
//
// This all creates a new problem with key events. If the browser implements the above strategy
// properly, then the browser's GtkWindow will never see the events and accelerators will not
// run, so we need to manually activate them here. (This function is called by PageClientImpl
// only after determining that the web process has not handled the event.) It's safe to do this
// even if the window already processed the events before WebKit, because in that case, no
// accelerator was activated (or it would have handled the event), so we'll fail to find any
// matching accelerator here. This clever strategy was proposed by Benjamin Otte.

GApplication* app = g_application_get_default();
if (!app || !GTK_IS_APPLICATION(app))
return;

ASSERT(gdk_event_get_event_type(event) == GDK_KEY_PRESS);
if (gdk_key_event_is_modifier(event))
return;

GUniquePtr<char> accelerator(gtk_accelerator_name(gdk_key_event_get_keyval(event), gdk_event_get_modifier_state(event)));
GUniquePtr<char*> actions(gtk_application_get_actions_for_accel(GTK_APPLICATION(app), accelerator.get()));
for (int i = 0; actions.get()[i]; ++i) {
const char* detailedAction = actions.get()[i];
GUniqueOutPtr<char> actionName;
GRefPtr<GVariant> targetValue;
GUniqueOutPtr<GError> error;
if (g_action_parse_detailed_name(detailedAction, &actionName.outPtr(), &targetValue.outPtr(), &error.outPtr()))
gtk_widget_activate_action_variant(GTK_WIDGET(webViewBase), actionName.get(), targetValue.get());
else
g_warning("Failed to parse detailed action %s: %s", detailedAction, error->message);
}
}

static gboolean webkitWebViewBaseKeyPressed(WebKitWebViewBase* webViewBase, unsigned keyval, unsigned, GdkModifierType state, GtkEventController* controller)
{
WebKitWebViewBasePrivate* priv = webViewBase->priv;
auto* event = gtk_event_controller_get_current_event(controller);

// Since WebProcess key event handling is not synchronous, handle the event in two passes.
// When WebProcess processes the input event, it will call PageClientImpl::doneWithKeyEvent
// with event handled status which determines whether to pass the input event to parent or not
// using gdk_display_put_event().
if (shouldForwardKeyEvent(webViewBase, event))
return GDK_EVENT_PROPAGATE;

bool isAutoRepeat = priv->keyAutoRepeatHandler.keyPress(gdk_key_event_get_keycode(event));

#if ENABLE(DEVELOPER_MODE) && OS(LINUX)
Expand Down Expand Up @@ -2583,17 +2620,13 @@ void webkitWebViewBaseDidPerformDragControllerAction(WebKitWebViewBase* webViewB
}
#endif // ENABLE(DRAG_SUPPORT)

#if !USE(GTK4)
void webkitWebViewBasePropagateKeyEvent(WebKitWebViewBase* webkitWebViewBase, GdkEvent* event)
{
#if USE(GTK4)
webkitWebViewBase->priv->keyEventsToPropagate.append(event);
// Note: the docs for gdk_display_put_event lie - this adds to the end of the queue, not the front.
gdk_display_put_event(gtk_widget_get_display(GTK_WIDGET(webkitWebViewBase)), event);
#else
webkitWebViewBase->priv->shouldForwardNextKeyEvent = true;
gtk_main_do_event(event);
#endif
}
#endif

void webkitWebViewBasePropagateWheelEvent(WebKitWebViewBase* webkitWebViewBase, GdkEvent* event)
{
Expand Down
4 changes: 4 additions & 0 deletions Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ void webkitWebViewBaseCreateWebPage(WebKitWebViewBase*, Ref<API::PageConfigurati
void webkitWebViewBaseSetTooltipText(WebKitWebViewBase*, const char*);
void webkitWebViewBaseSetTooltipArea(WebKitWebViewBase*, const WebCore::IntRect&);
void webkitWebViewBaseSetMouseIsOverScrollbar(WebKitWebViewBase*, WebKit::WebHitTestResultData::IsScrollbar);
#if USE(GTK4)
void webkitWebViewBaseProcessAcceleratorsForKeyPressEvent(WebKitWebViewBase*, GdkEvent*);
#else
void webkitWebViewBasePropagateKeyEvent(WebKitWebViewBase*, GdkEvent*);
#endif
void webkitWebViewBasePropagateWheelEvent(WebKitWebViewBase*, GdkEvent*);
void webkitWebViewBaseChildMoveResize(WebKitWebViewBase*, GtkWidget*, const WebCore::IntRect&);
void webkitWebViewBaseEnterFullScreen(WebKitWebViewBase*);
Expand Down

0 comments on commit 3d84c94

Please sign in to comment.