707623 - Keyboard events are fired twice inside a panel

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Roland Warmerdam]

Attachment: Test case addon that demonstrates keyboard events firing twice on various elements

Some keyboard events are fired twice on some elements inside a panel. More specifically, all keyboard events are fired twice on a focusable <div> (with a tabindex set) and keyup events are fired twice on both <textarea> and <input> elements.

Also, calling preventDefault() in the event handler stops the second event from being fired.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm pretty sure this is a Core - Widget:Gtk issue...

Comment 3

[Karl Tomlinson (:karlt)]

GTK sends the event first to the grab widget and then, if the handler does not return TRUE, to the focus widget (and their parents if the handler still doesn't return TRUE).  Those are the same widget in this situation.
http://git.gnome.org/browse/gtk+/tree/gtk/gtkmain.c?id=2.24.13#n2452

preventDefault() avoids the behavior because the handler then returns TRUE:
http://hg.mozilla.org/mozilla-central/annotate/1c3e4cb1f754/widget/gtk2/nsWindow.cpp#l3031

I assume the return FALSE there is to allow embedding apps to handle key events not handled by Gecko.

Comment 4

[Karl Tomlinson (:karlt)]

The gtk_grab_add is now no longer necessary for plugins, as the out of process plugin is a different X11 client so it doesn't get mouse events while the parent process has a pointer grab, and the out of process GtkPlug will never forward key events to the parent process (and it seems Flash didn't forward key events anyway).  Only events on a Gecko GtkWidget should cause a popup to open, so Gecko should have widget-focus while a popup is open.
(See bug 750061 comment 11 for a summary of GTK/GDK grab behavior.)

The remaining use that I can imagine for the gtk_grab_add is to ensure that mouse events go to a Gecko GtkWidget even if it is embedded in a non-Gecko GtkWidget, so that popups can roll up on button press while the mouse is over a non-Gecko GtkWidget (using the same Display X connection).

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #3)
> I assume the return FALSE there is to allow embedding apps to handle key
> events not handled by Gecko.

But in the case of the panel, there is no parent widget, so there is no need to do this.  We can always return TRUE from key_press_event_cb if the nsWindow corresponding to the GtkWidget there is of type eWindowType_child.  Note that it is |window| that needs to be tested because |gFocusWindow| is typically not the nsWindow of the panel.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: gtk_grab_add(mShell) instead of (mContainer)

I think this is a bit simpler.

Changing the grab from the MozContainer to the toplevel GtkWindow means that
the "widget != window" condition is false in gtk_propagate_event and so the
key event is dispatched only to the focus widget within the grab GtkWindow
(and we only expect to have one child widget in popup windows).

These methods are now only functional if the nsIWidget is toplevel (including
popups/panels, which are the only widget types expected to need these).

Since bug 793501, Gecko listens for mouse events on the mShell window (except when an embedded child window), so this patch has no effect on how Gecko receives mouse events.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 674976 [details] [diff] [review]
gtk_grab_add(mShell) instead of (mContainer)

I can tell you that this definitely fixes the problem with the Downloads Panel (where hitting the spacebar would rapidly pause and then restart the download). Thanks!

Is this ready for review?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey Karl, I hope you don't mind if I commit this for you.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Landed on inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/33df4d95396a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33df4d95396a