48986 2010-11-04 03:52:00 -0700 [GTK] Some key-press events can't be handled by WebView 2010-11-18 11:03:37 -0800 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 25525 0 cgarcia webkit-unassigned mrobinson oldest_to_newest 304570 0 cgarcia 2010-11-04 03:52:00 -0700 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. 304631 1 mrobinson 2010-11-04 07:02:36 -0700 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. 309671 2 73981 cgarcia 2010-11-16 04:19:38 -0800 Created attachment 73981 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. 309728 3 73981 mrobinson 2010-11-16 08:14:36 -0800 Comment on attachment 73981 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. 309745 4 cgarcia 2010-11-16 08:51:33 -0800 (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. 309755 5 mrobinson 2010-11-16 09:06:18 -0800 (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. 309776 6 74004 cgarcia 2010-11-16 09:38:27 -0800 Created attachment 74004 Updated patch fixing coding style issues 309785 7 74004 mrobinson 2010-11-16 09:56:08 -0800 Comment on attachment 74004 Updated patch fixing coding style issues I'll land this myself after adding the comment. 309786 8 mrobinson 2010-11-16 09:57:09 -0800 Committed r72113: <http://trac.webkit.org/changeset/72113> 73981 2010-11-16 04:19:38 -0800 2010-11-16 09:38:27 -0800 Patch to fix the problem key-events.diff text/plain 2570 cgarcia ZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvQ2hhbmdlTG9nIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK aW5kZXggYWEwYzBkYS4uYTQ1NmI2YyAxMDA2NDQKLS0tIGEvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMiBAQAorMjAxMC0xMS0xNiAg Q2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gU29tZSBrZXktcHJlc3Mg ZXZlbnRzIGNhbid0IGJlIGhhbmRsZWQgYnkgV2ViVmlldworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDg5ODYKKworICAgICAgICBUaGUgcHJvYmxlbSBp cyB0aGF0ICJwb3B1cC1tZW51IiBhbmQgInNob3ctaGVscCIgc2lnbmFscyBhcmUKKyAgICAgICAg aGFuZGxlZCBieSBHdGtUZXh0VmlldywgYXMgdGhleSBhcmUgYWRkZWQgdG8gdGhlIGtleSBiaW5k aW5ncyBzZXQKKyAgICAgICAgb2YgZXZlcnkgd2lkZ2V0IGluIGd0a193aWRnZXRfY2xhc3NfaW5p dCgpLiBGb3IgYWxsIG90aGVyIGJpbmRpbmdzCisgICAgICAgIGhhbmRsZWQgYnkgR3RrVGV4dFZp ZXcgd2UgYXJlIHN0b3BwaW5nIHRoZSBzaWduYWwgZW1pc3Npb24gaW4KKyAgICAgICAgdGhlaXIg Y2FsbGJhY2tzLCBzbyB3ZSBvbmx5IG5lZWQgdG8gZG8gdGhlIHNhbWUgZm9yICJwb3B1cC1tZW51 IgorICAgICAgICBhbmQgInNob3ctaGVscCIgc2lnbmFscy4KKworICAgICAgICAqIFdlYkNvcmVT dXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6OnBvcHVwTWVudUNh bGxiYWNrKToKKyAgICAgICAgKFdlYktpdDo6c2hvd0hlbHBDYWxsYmFjayk6CisgICAgICAgIChX ZWJLaXQ6OkVkaXRvckNsaWVudDo6RWRpdG9yQ2xpZW50KToKKwogMjAxMC0xMS0xMSAgSm9vbmUg SHVyICA8am9vbmVAa2xkcC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW50b25pbyBHb21l cy4KZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50R3Rr LmNwcCBiL1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50R3RrLmNwcAppbmRl eCBjN2IzMGIxLi5iYTBmMjk4IDEwMDY0NAotLS0gYS9XZWJLaXQvZ3RrL1dlYkNvcmVTdXBwb3J0 L0VkaXRvckNsaWVudEd0ay5jcHAKKysrIGIvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0 b3JDbGllbnRHdGsuY3BwCkBAIC0xMjksNiArMTI5LDE2IEBAIHN0YXRpYyB2b2lkIHRvZ2dsZU92 ZXJ3cml0ZUNhbGxiYWNrKEd0a1dpZGdldCogd2lkZ2V0LCBFZGl0b3JDbGllbnQqIGNsaWVudCkK ICAgICBnX3NpZ25hbF9zdG9wX2VtaXNzaW9uX2J5X25hbWUod2lkZ2V0LCAidG9nZ2xlLW92ZXJ3 cml0ZSIpOwogfQogCitzdGF0aWMgdm9pZCBwb3B1cE1lbnVDYWxsYmFjayhHdGtXaWRnZXQqIHdp ZGdldCwgRWRpdG9yQ2xpZW50KiBjbGllbnQpCit7CisgICAgZ19zaWduYWxfc3RvcF9lbWlzc2lv bl9ieV9uYW1lKHdpZGdldCwgInBvcHVwLW1lbnUiKTsKK30KKworc3RhdGljIHZvaWQgc2hvd0hl bHBDYWxsYmFjayhHdGtXaWRnZXQqIHdpZGdldCwgRWRpdG9yQ2xpZW50KiBjbGllbnQpCit7Cisg ICAgZ19zaWduYWxfc3RvcF9lbWlzc2lvbl9ieV9uYW1lKHdpZGdldCwgInNob3ctaGVscCIpOwor fQorCiBzdGF0aWMgY29uc3QgY2hhciogY29uc3QgZ3RrRGVsZXRlQ29tbWFuZHNbXVsyXSA9IHsK ICAgICB7ICJEZWxldGVCYWNrd2FyZCIsICAgICAgICAgICAgICAgIkRlbGV0ZUZvcndhcmQiICAg ICAgICAgICAgICAgICAgICAgICAgfSwgLy8gQ2hhcmFjdGVycwogICAgIHsgIkRlbGV0ZVdvcmRC YWNrd2FyZCIsICAgICAgICAgICAiRGVsZXRlV29yZEZvcndhcmQiICAgICAgICAgICAgICAgICAg ICB9LCAvLyBXb3JkIGVuZHMKQEAgLTc4OSw2ICs3OTksOCBAQCBFZGl0b3JDbGllbnQ6OkVkaXRv ckNsaWVudChXZWJLaXRXZWJWaWV3KiB3ZWJWaWV3KQogICAgIGdfc2lnbmFsX2Nvbm5lY3QobV9u YXRpdmVXaWRnZXQuZ2V0KCksICJtb3ZlLWN1cnNvciIsIEdfQ0FMTEJBQ0sobW92ZUN1cnNvckNh bGxiYWNrKSwgdGhpcyk7CiAgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQo KSwgImRlbGV0ZS1mcm9tLWN1cnNvciIsIEdfQ0FMTEJBQ0soZGVsZXRlRnJvbUN1cnNvckNhbGxi YWNrKSwgdGhpcyk7CiAgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQoKSwg InRvZ2dsZS1vdmVyd3JpdGUiLCBHX0NBTExCQUNLKHRvZ2dsZU92ZXJ3cml0ZUNhbGxiYWNrKSwg dGhpcyk7CisgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQoKSwgInBvcHVw LW1lbnUiLCBHX0NBTExCQUNLKHBvcHVwTWVudUNhbGxiYWNrKSwgdGhpcyk7CisgICAgZ19zaWdu YWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQoKSwgInNob3ctaGVscCIsIEdfQ0FMTEJBQ0so c2hvd0hlbHBDYWxsYmFjayksIHRoaXMpOwogfQogCiBFZGl0b3JDbGllbnQ6On5FZGl0b3JDbGll bnQoKQo= 74004 2010-11-16 09:38:27 -0800 2010-11-18 11:03:37 -0800 Updated patch fixing coding style issues key-events2.diff text/plain 2975 cgarcia ZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvQ2hhbmdlTG9nIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK aW5kZXggYWEwYzBkYS4uNGFiODIwZSAxMDA2NDQKLS0tIGEvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMyBAQAorMjAxMC0xMS0xNiAg Q2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gU29tZSBrZXktcHJlc3Mg ZXZlbnRzIGNhbid0IGJlIGhhbmRsZWQgYnkgV2ViVmlldworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDg5ODYKKworICAgICAgICBUaGUgcHJvYmxlbSBp cyB0aGF0ICJwb3B1cC1tZW51IiBhbmQgInNob3ctaGVscCIgc2lnbmFscyBhcmUKKyAgICAgICAg aGFuZGxlZCBieSBHdGtUZXh0VmlldywgYXMgdGhleSBhcmUgYWRkZWQgdG8gdGhlIGtleSBiaW5k aW5ncyBzZXQKKyAgICAgICAgb2YgZXZlcnkgd2lkZ2V0IGluIGd0a193aWRnZXRfY2xhc3NfaW5p dCgpLiBGb3IgYWxsIG90aGVyIGJpbmRpbmdzCisgICAgICAgIGhhbmRsZWQgYnkgR3RrVGV4dFZp ZXcgd2UgYXJlIHN0b3BwaW5nIHRoZSBzaWduYWwgZW1pc3Npb24gaW4KKyAgICAgICAgdGhlaXIg Y2FsbGJhY2tzLCBzbyB3ZSBvbmx5IG5lZWQgdG8gZG8gdGhlIHNhbWUgZm9yICJwb3B1cC1tZW51 IgorICAgICAgICBhbmQgInNob3ctaGVscCIgc2lnbmFscy4KKworICAgICAgICAqIFdlYkNvcmVT dXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6OnRvZ2dsZU92ZXJ3 cml0ZUNhbGxiYWNrKToKKyAgICAgICAgKFdlYktpdDo6cG9wdXBNZW51Q2FsbGJhY2spOgorICAg ICAgICAoV2ViS2l0OjpzaG93SGVscENhbGxiYWNrKToKKyAgICAgICAgKFdlYktpdDo6RWRpdG9y Q2xpZW50OjpFZGl0b3JDbGllbnQpOgorCiAyMDEwLTExLTExICBKb29uZSBIdXIgIDxqb29uZUBr bGRwLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbnRvbmlvIEdvbWVzLgpkaWZmIC0tZ2l0 IGEvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwIGIvV2ViS2l0 L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwCmluZGV4IGM3YjMwYjEuLjY2 YjhjZmYgMTAwNjQ0Ci0tLSBhL1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50 R3RrLmNwcAorKysgYi9XZWJLaXQvZ3RrL1dlYkNvcmVTdXBwb3J0L0VkaXRvckNsaWVudEd0ay5j cHAKQEAgLTEyMiwxMyArMTIyLDIzIEBAIHN0YXRpYyB2b2lkIHBhc3RlQ2xpcGJvYXJkQ2FsbGJh Y2soR3RrV2lkZ2V0KiB3aWRnZXQsIEVkaXRvckNsaWVudCogY2xpZW50KQogICAgIGNsaWVudC0+ YWRkUGVuZGluZ0VkaXRvckNvbW1hbmQoIlBhc3RlIik7CiB9CiAKLXN0YXRpYyB2b2lkIHRvZ2ds ZU92ZXJ3cml0ZUNhbGxiYWNrKEd0a1dpZGdldCogd2lkZ2V0LCBFZGl0b3JDbGllbnQqIGNsaWVu dCkKK3N0YXRpYyB2b2lkIHRvZ2dsZU92ZXJ3cml0ZUNhbGxiYWNrKEd0a1dpZGdldCogd2lkZ2V0 LCBFZGl0b3JDbGllbnQqKQogewogICAgIC8vIFdlIGRvbid0IHN1cHBvcnQgdG9nZ2xpbmcgdGhl IG92ZXJ3cml0ZSBtb2RlLCBidXQgdGhlIGRlZmF1bHQgY2FsbGJhY2sgZXhwZWN0cwogICAgIC8v IHRoZSBHdGtUZXh0VmlldyB0byBoYXZlIGEgbGF5b3V0LCBzbyB3ZSBoYW5kbGUgdGhpcyBzaWdu YWwganVzdCB0byBzdG9wIGl0LgogICAgIGdfc2lnbmFsX3N0b3BfZW1pc3Npb25fYnlfbmFtZSh3 aWRnZXQsICJ0b2dnbGUtb3ZlcndyaXRlIik7CiB9CiAKK3N0YXRpYyB2b2lkIHBvcHVwTWVudUNh bGxiYWNrKEd0a1dpZGdldCogd2lkZ2V0LCBFZGl0b3JDbGllbnQqKQoreworICAgIGdfc2lnbmFs X3N0b3BfZW1pc3Npb25fYnlfbmFtZSh3aWRnZXQsICJwb3B1cC1tZW51Iik7Cit9CisKK3N0YXRp YyB2b2lkIHNob3dIZWxwQ2FsbGJhY2soR3RrV2lkZ2V0KiB3aWRnZXQsIEVkaXRvckNsaWVudCop Cit7CisgICAgZ19zaWduYWxfc3RvcF9lbWlzc2lvbl9ieV9uYW1lKHdpZGdldCwgInNob3ctaGVs cCIpOworfQorCiBzdGF0aWMgY29uc3QgY2hhciogY29uc3QgZ3RrRGVsZXRlQ29tbWFuZHNbXVsy XSA9IHsKICAgICB7ICJEZWxldGVCYWNrd2FyZCIsICAgICAgICAgICAgICAgIkRlbGV0ZUZvcndh cmQiICAgICAgICAgICAgICAgICAgICAgICAgfSwgLy8gQ2hhcmFjdGVycwogICAgIHsgIkRlbGV0 ZVdvcmRCYWNrd2FyZCIsICAgICAgICAgICAiRGVsZXRlV29yZEZvcndhcmQiICAgICAgICAgICAg ICAgICAgICB9LCAvLyBXb3JkIGVuZHMKQEAgLTc4OSw2ICs3OTksOCBAQCBFZGl0b3JDbGllbnQ6 OkVkaXRvckNsaWVudChXZWJLaXRXZWJWaWV3KiB3ZWJWaWV3KQogICAgIGdfc2lnbmFsX2Nvbm5l Y3QobV9uYXRpdmVXaWRnZXQuZ2V0KCksICJtb3ZlLWN1cnNvciIsIEdfQ0FMTEJBQ0sobW92ZUN1 cnNvckNhbGxiYWNrKSwgdGhpcyk7CiAgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdl dC5nZXQoKSwgImRlbGV0ZS1mcm9tLWN1cnNvciIsIEdfQ0FMTEJBQ0soZGVsZXRlRnJvbUN1cnNv ckNhbGxiYWNrKSwgdGhpcyk7CiAgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5n ZXQoKSwgInRvZ2dsZS1vdmVyd3JpdGUiLCBHX0NBTExCQUNLKHRvZ2dsZU92ZXJ3cml0ZUNhbGxi YWNrKSwgdGhpcyk7CisgICAgZ19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQoKSwg InBvcHVwLW1lbnUiLCBHX0NBTExCQUNLKHBvcHVwTWVudUNhbGxiYWNrKSwgdGhpcyk7CisgICAg Z19zaWduYWxfY29ubmVjdChtX25hdGl2ZVdpZGdldC5nZXQoKSwgInNob3ctaGVscCIsIEdfQ0FM TEJBQ0soc2hvd0hlbHBDYWxsYmFjayksIHRoaXMpOwogfQogCiBFZGl0b3JDbGllbnQ6On5FZGl0 b3JDbGllbnQoKQo=