Bug 136430

Summary: [GTK] Key press signal fired twice in WebKit2
Product: WebKit Reporter: Ainieco <a1968190>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ap, bugs-noreply, cgarcia, gns, mcatanzaro, mike
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   

Description Ainieco 2014-09-01 09:31:04 PDT
webkit2gtk+ version: 2.4.4(heard that it reproducible on 2.4.5 too)

Can't properly bind key press events with created web view - it will be triggered twice,
doesn't matter if I bind my key press callback to window or to web view itself, reproduce with:

#include <gtk/gtk.h>
#include <webkit2/webkit2.h>

gboolean foo (GtkWidget *widget, GdkEvent *event, gpointer data) {
  printf("%s\n", event->key.string);

  return FALSE;
}

int main (int argc, char *argv[]) {
  gtk_init (&argc, &argv);
  GtkWidget *window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
  GtkWidget *web_view = webkit_web_view_new ();

  g_signal_connect (web_view, "key-press-event", G_CALLBACK (foo), NULL);

  gtk_container_add (GTK_CONTAINER (window), web_view);

  gtk_widget_show_all (window);

  gtk_main ();

  return 0;
}
Comment 1 Carlos Garcia Campos 2014-09-01 10:34:51 PDT
This is complicated . . . The thing is that the event propagation in GTK+ is synchronous, but events are processed asynchronously in WebKit. When a key is pressed, a message is sent to the web process, the key is processed, and a message is sent back to the UI process with a boolean parameter indicating whether the event has been handled or not. That return value is what we need to decide whether to propagate the event or not, so what we do is always returning TRUE from our key press vmethod to stop the event propagation, and when we get the response from the web process, we reinject the event with gtk_main_do_event() and this time we propagate the event to the parent class depending on the value get from the web process. Since GtkWidget::key-press-event is a G_SIGNAL_RUN_LAST signal, the user connected callbacks are called *before* our vmethod, so when the event is reinjected the user provided callbacks are called again too. A possible workaround for you would be to use g_signal_connect_after() to make sure your callback is called after the vmethod. The only problem is that if the web process handles the key, you won't be notified. Ideally we should not emit the user provided callbacks when the event is reinjected, but I don't think it's possible to do that.
Comment 2 Ainieco 2014-09-01 11:05:51 PDT
I heard that it's working(not calling callback twice) in wk1 for some reason. Thanks for workaround!

(In reply to comment #1)
Comment 3 Carlos Garcia Campos 2014-09-02 01:25:42 PDT
(In reply to comment #2)
> I heard that it's working(not calling callback twice) in wk1 for some reason. Thanks for workaround!
> 

The reason is that in WebKit1 there's no web process, so the events were handled synchronously like in GTK+
Comment 4 Michael Gratton 2017-01-05 03:31:56 PST
This the underlying cause of this has been causing problems with the WK2 port of Geary.

We historically have used a number of single keystroke shortcuts, and to ensure they weren't triggered when composing a message, we had effectively had to override gtk_window_key_press_event() since that (1) checks for keyboard shortcuts to activate (by calling gtk_window_activate_key) before (2) sending the event to the focus widget (by calling gtk_window_propagate_key_event). The overridden implementation simply calls gtk_window_propagate_key_event first, then chains up, so gtk_window_propagate_key_event ends up being called twice if the event is not handled before then.

So when a WebView is focused and does not handle the event, the first call to gtk_window_propagate_key_event forwards the event on to WebView, which hands it off to the WebProcess and responds as if it had been handled, and no further processing occurs. The WebView generates a new event when it is notified by the WebProcess that the key event was unhandled, and forwards the event through the same machinery. The first call is not handled by the WebView (I'm not sure why, maybe WK2 has some special code to detect this and avoid re-handling the same event?), but the second call to gtk_window_propagate_key_event causes the same event to be sent to the WebView, which treats it as a new event and the whole thing then repeats. This chain effectively forms a busy-wait loop - the Gtk main loop machinery processes it all as fast as it can. Although the application remains responsive since it's an async process, the CPU will be pinned at 100%. Additional unhanded key strokes form additional chains, adding to the overhead.

This happens for any keystroke sent to a WebView being used to display a HTML document, or when a keystroke like Shift is not handled when a TEXTAREA or content-editable region.

I guess this is a corner case, but took a while to work out what was going on. It would be nice if the underlying issue was documented somewhere, but I wonder if the events that the WebView re-injects after getting the async response back from the WebProcess could be flagged somehow as being the case, so applications could look out for them and operate on them or ignore them as appropriate. Could GdkEventKey somehow be extended, or maybe GdkEventKey::send_event be set to true?
Comment 5 Michael Catanzaro 2017-01-05 07:23:11 PST
(In reply to comment #4)
> or maybe GdkEventKey::send_event be set to true?

Maybe this would work?
Comment 6 Michael Catanzaro 2017-01-07 09:48:12 PST
I'd want confirmation from a GTK+ developer that it would be an appropriate use of the send_event flag and is not going to break anything, but you could try setting that flag in PageClientImpl::doneWithKeyEvent in Source/WebKit2/UIProcess/API/gtk, just before the call to gtk_main_do_event. Then Geary could ignore the events if that flag is not set. Should work as long as the flag is otherwise always unset.
Comment 7 Michael Gratton 2017-01-08 06:01:33 PST
(In reply to comment #5)
> (In reply to comment #4)
> > or maybe GdkEventKey::send_event be set to true?
> 
> Maybe this would work?

(In reply to comment #6)
> I'd want confirmation from a GTK+ developer that it would be an appropriate
> use of the send_event flag and is not going to break anything, but you could
> try setting that flag in PageClientImpl::doneWithKeyEvent in
> Source/WebKit2/UIProcess/API/gtk, just before the call to gtk_main_do_event.
> Then Geary could ignore the events if that flag is not set. Should work as
> long as the flag is otherwise always unset.

Yeah, it sounds about right, and that's basically the idea I had in mind for Geary. I don't know what the net effect of setting it is in GTK+ though, it seems to be used somewhat sporadically across the code base.

This isn't a big dealbreaker for Geary though - we have a reasonable workaround for now.