Closed Bug 707623 Opened 10 years ago Closed 9 years ago

Keyboard events are fired twice inside a panel


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






(Reporter: rowno, Assigned: karlt)




(2 files)

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.
Duplicate of this bug: 801011
I'm pretty sure this is a Core - Widget:Gtk issue...
Component: General → Widget: Gtk
Product: Add-on SDK → Core
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.

preventDefault() avoids the behavior because the handler then returns TRUE:

I assume the return FALSE there is to allow embedding apps to handle key events not handled by Gecko.
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).
(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.
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.
Assignee: nobody → karlt
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?
Attachment #674976 - Flags: feedback+
Attachment #674976 - Flags: review?(roc)
Hey Karl, I hope you don't mind if I commit this for you.
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 808114
You need to log in before you can comment on or make changes to this bug.