296002 - drop-down menus fail when gtkmozembed is moved to different toplevel

Status: RESOLVED WONTFIX

(Core :: Widget: Gtk, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

When moving a GtkMozEmbed to a different toplevel window, drop-down elements are
closed immediately after being popped up, making selection impossible.

Steps to reproduce:
(0) Start Epiphany
(1) Load https://bugzilla.mozilla.org/query.cgi
(2) Open another tab
(3) Pop up the "Summary:" drop-down
(4) Right-click the mozilla.org tab's tab label, and choose "Detach tab"
(5) Repeat step (3)

Expected results:
In steps (3) and (5), the drop-down opens and I can choose an option.

Actual results:
In step (3) and (5), the drop-down opens. But I can only choose an option in
step (3), since the drop-down immediately closes itself in step (5).

Analysis:
In widget/src/gtk2/nsWindow.cpp: nsWindow::NativeCreate with type
eWindowType_popup the toplevel the widget is _currently_ in is stored in
mTransientParent, and used later for adding a keyboard grab when opening the
drop-down. But the mTransientParent variable is not updated when the GtkMozEmbed
widget containing the web page owning the popup widget is moved to a different
toplevel, resulting in the keyboard grab being against the wrong window.

(The same thing probably applies in the eWindowType_dialog case, but I cannot
test it since I have no idea under what circumstances a widget of this type will
be created in an embedding scenario.)

Patch follows.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: fix

We need to keep the mTransientParent variable updated when moving the parent
container to another toplevel.

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 184879 [details] [diff] [review]
fix

This crashes when the parent container is reparented after the popup is
destroyed. New patch coming up later.

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

This patch fixes the crash, by disconnecting the signal from the parent
container when the popup is destroyed.

Comment 4

[Marco Pesenti Gritti]

Comment on attachment 184942 [details] [diff] [review]
updated patch

Some comments:

- The approach looks correct to me. The toplevel window is not specified by the
caller but nsWindow is determining it, so I think it's nsWindow job to keep it
updated. Though, I'd like someone with more knowledge of the code to take a
look.

- While we are at it I think we should fix also the dialog case. We should
probably connect the signal just after get_toplevel and have SetToplevelWindow
deal with both cases.

+	     mParentContainer = parentMozContainer;

Would probably be cleaner to just store this as class member.

+  GtkWidget *toplevelCandidate = gtk_widget_get_toplevel (container);
+  GtkWidget *toplevel = GTK_WIDGET_TOPLEVEL (toplevelCandidate) ?
toplevelCandidate : nsnull;
+
+  window->SetTransientParent (GTK_WINDOW (toplevel));

Using something like if (GTK_WIDGET_TOPLEVEL (toplevel)) ... else... would be
probably more readable.

Comment 5

[Sebastian Dröge]

Any progress on this bug?

Comment 6

[Chris Lord [:cwiiis]]

Again, any progress on this?

Comment 7

[Sebastien Bacher]

Ubuntu bug about that: https://launchpad.net/distros/ubuntu/+source/firefox/+bug/21753

Comment 8

[Mike Hommey [:glandium]]

Again, any progress on this?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Marco minused the patch. Someone needs to submit a new patch.

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 184942 [details] [diff] [review]
updated patch

This patch is also buggy;  see http://bugs.debian.org/432672  (they ship this patch in their xulrunner package).

Comment 11

[Martin Stránský [:stransky] (ni? me)]

We're not going to fix that.