Bug 48986 – [GTK] Some key-press events can't be handled by WebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

It's easy to reproduce using GtkLauncher, for example, just press CTRL + F1, you can see a critical warning:

(GtkLauncher:6359): Gdk-CRITICAL **: gdk_window_get_device_position: assertion `GDK_IS_WINDOW (window)' failed

The problem was introduced in revision 59158, see http://trac.webkit.org/changeset/59158. In WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp, EditorClient::generateEditorCommands() the line

gtk_bindings_activate_event(GTK_OBJECT(m_nativeWidget.get()), event->keyEvent()->gdkEventKey()); 

redirects the event to the native widget, a GtkTextView, so it never gets to the WebView widget. In this case, when pressing CTRL + F1, the text view handles it and runs GtkWidget::show_help() which tries to show a tooltip, but since the text view widget is not realized it fails with the critial warning. And, of course, WebView doesn't show the tooltip because it doesn't receive the event. 

Removing that line or using m_webView instead of m_nativeWidget fixes the issue, so maybe we could just call gtk_bindings_activate_event() again with m_webView when event is not handled by the text widget.

[Martin Robinson]

Nice catch! In the past, I solved a problem like this one by blocking the signal emission: http://trac.webkit.org/changeset/67566. Perhaps in this case it should block the signal emission and forward it to the WebKitWebView.

[Carlos Garcia Campos]

Created attachment 73981 [details]
Patch to fix the problem

The problem is that "popup-menu" and "show-help" signals are handled by GtkTextView, as they are added to the key bindings set of every widget in gtk_widget_class_init(). For all other bindings handled by GtkTextView we are stopping the signal emission in their callbacks, so we only need to do the same for "popup-menu" and "show-help" signals.

[Martin Robinson]

Comment on attachment 73981 [details]
Patch to fix the problem

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

Thanks for making this fix!

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:141
> +static void popupMenuCallback(GtkWidget* widget, EditorClient* client)
> +{
> +    g_signal_stop_emission_by_name(widget, "popup-menu");
> +}
> +
> +static void showHelpCallback(GtkWidget* widget, EditorClient* client)
> +{
> +    g_signal_stop_emission_by_name(widget, "show-help");
> +}
> +

Since "client" is unused the variable name should be omitted from the argument list. Isn't it possible to forward these signals to the WebView? I believe that show-help at least is important for accessibility.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 73981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73981&action=review
> 
> Thanks for making this fix!
> 
> > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:141
> > +static void popupMenuCallback(GtkWidget* widget, EditorClient* client)
> > +{
> > +    g_signal_stop_emission_by_name(widget, "popup-menu");
> > +}
> > +
> > +static void showHelpCallback(GtkWidget* widget, EditorClient* client)
> > +{
> > +    g_signal_stop_emission_by_name(widget, "show-help");
> > +}
> > +
> 
> Since "client" is unused the variable name should be omitted from the argument list.

Right, I copy-pasted from toggleOverwriteCallback, I'll fix that one too.

> Isn't it possible to forward these signals to the WebView? I believe that show-help at least is important for accessibility.

We are stopping the emission of those signals for the GtkTextView, because it's not realized, but not for any other widgets. That's exactly what this patch is fixing, since the signals don't fail they get to the view, see bug #25525.

[Martin Robinson]

(In reply to comment #4)

> We are stopping the emission of those signals for the GtkTextView, because it's not realized, but not for any other widgets. That's exactly what this patch is fixing, since the signals don't fail they get to the view, see bug #25525.

Great. I think it makes sense to leave a comment explaining that the signal will still bubble up to the web view.

[Carlos Garcia Campos]

Created attachment 74004 [details]
Updated patch fixing coding style issues

[Martin Robinson]

Comment on attachment 74004 [details]
Updated patch fixing coding style issues

I'll land this myself after adding the comment.

[Martin Robinson]

Committed r72113: <http://trac.webkit.org/changeset/72113>