Bug 48986 - [GTK] Some key-press events can't be handled by WebView
Summary: [GTK] Some key-press events can't be handled by WebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25525
  Show dependency treegraph
 
Reported: 2010-11-04 03:52 PDT by Carlos Garcia Campos
Modified: 2010-11-18 11:03 PST (History)
1 user (show)

See Also:


Attachments
Patch to fix the problem (2.51 KB, patch)
2010-11-16 04:19 PST, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch fixing coding style issues (2.91 KB, patch)
2010-11-16 09:38 PST, Carlos Garcia Campos
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 2010-11-04 03:52:00 PDT
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.
Comment 1 Martin Robinson 2010-11-04 07:02:36 PDT
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.
Comment 2 Carlos Garcia Campos 2010-11-16 04:19:38 PST
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.
Comment 3 Martin Robinson 2010-11-16 08:14:36 PST
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.
Comment 4 Carlos Garcia Campos 2010-11-16 08:51:33 PST
(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.
Comment 5 Martin Robinson 2010-11-16 09:06:18 PST
(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.
Comment 6 Carlos Garcia Campos 2010-11-16 09:38:27 PST
Created attachment 74004 [details]
Updated patch fixing coding style issues
Comment 7 Martin Robinson 2010-11-16 09:56:08 PST
Comment on attachment 74004 [details]
Updated patch fixing coding style issues

I'll land this myself after adding the comment.
Comment 8 Martin Robinson 2010-11-16 09:57:09 PST
Committed r72113: <http://trac.webkit.org/changeset/72113>