Keyboard events are fired twice inside a panel

RESOLVED FIXED in mozilla19

Status

()

P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: rowno, Assigned: karlt)

Tracking

unspecified
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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.
Priority: -- → P2
Blocks: 697775
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
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
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).
(Assignee)

Comment 5

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
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+
(Assignee)

Updated

7 years ago
Attachment #674976 - Flags: review?(roc)
Hey Karl, I hope you don't mind if I commit this for you.
https://hg.mozilla.org/mozilla-central/rev/33df4d95396a
Status: NEW → RESOLVED
Last Resolved: 7 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.