The default bug view has changed. See this FAQ.

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

NEW
Unassigned

Status

()

Core
Widget: Gtk
--
major
12 years ago
5 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 obsolete attachments)

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.
Created attachment 184879 [details] [diff] [review]
fix

We need to keep the mTransientParent variable updated when moving the parent
container to another toplevel.
Attachment #184879 - Flags: superreview?(blizzard)
Attachment #184879 - Flags: review?(mpgritti)
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.
Attachment #184879 - Attachment is obsolete: true
Attachment #184879 - Flags: superreview?(blizzard)
Attachment #184879 - Flags: review?(mpgritti)
Created attachment 184942 [details] [diff] [review]
updated patch

This patch fixes the crash, by disconnecting the signal from the parent
container when the popup is destroyed.
Attachment #184942 - Flags: review?(mpgritti)

Comment 4

12 years ago
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.
Attachment #184942 - Flags: review?(mpgritti) → review-

Comment 5

12 years ago
Any progress on this bug?

Comment 6

11 years ago
Again, any progress on this?
Assignee: blizzard → nobody

Comment 7

11 years ago
Ubuntu bug about that: https://launchpad.net/distros/ubuntu/+source/firefox/+bug/21753
Again, any progress on this?
Marco minused the patch. Someone needs to submit a new patch.
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).
Attachment #184942 - Attachment is obsolete: true

Updated

7 years ago
You need to log in before you can comment on or make changes to this bug.