522956 - Clicking Larry's "More Information" button opens Page Info dialog but doesn't close Larry

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Elmar Ludwig]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091010 Minefield/3.7a1pre ID:20091010030715

When I click the "More Information" button on Larry's UI, the Page Info dialog opens but Larry doesn't close (and is, therefore, in front of the Page Info dialog). This is basically the same as bug 402415, which disappeared (or was fixed?) around the beginning of 2008.

This seems to have regressed again in the 2008-08-18 trunk build:

firefox-3.1a2pre-2008-08-17-02:   works fine
firefox-3.1a2pre-2008-08-18-02:   shows the bug

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-17+02%3A00%3A00&enddate=2008-08-18+02%3A00%3A00

Strangely, I cannot reproduce this on Firefox 3.5.3, which is also based on the 1.9.1 branch...

Comment 1

[Reed Loden [:reed]]

Indeed, this has been bugging me for months. Can we just take johnath's patch in bug 402415 to wallpaper this?

I know this affects Minefield (3.7) on 1.9.3. Does it also affect Namoroka (3.6) on 1.9.2?

Comment 3

[u279076]

(In reply to comment #1)
> Indeed, this has been bugging me for months. Can we just take johnath's patch
> in bug 402415 to wallpaper this?
> 
> I know this affects Minefield (3.7) on 1.9.3. Does it also affect Namoroka
> (3.6) on 1.9.2?

I ran across this today during 3.6rc1 testing

Comment 6

[Dietrich Ayala (:dietrich)]

Neil, is this a problem with panels on Linux?

Comment 7

[Neil Deakin]

Yes, similar to bug 545429.

Comment 9

[Michael Monreal [:micmon]]

I just marked #540096 as dupe. Only strange thing is that the reporter claims that it works fine in Fx 3.5.7 (but not after that).

I still see this on the latest minefield nightly and think there should be a priority to finally fix this for 4.0

Comment 10

[Dietrich Ayala (:dietrich)]

Blocking+, since this is pretty broken looking, and affects multiple different pieces of UI.

Assigning to Neil for now. If there's a more appropriate owner, please re-assign.

Comment 11

[Neil Deakin]

Attachment: something like so

This should roll up open menus/popups upon move, resize, activate, deactivate, close, minimize and maximize on a toplevel window or dialog.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 464484 [details] [diff] [review]
something like so

If it were solely this bug report, attachment 287401 [details] [diff] [review] seemed an appropriate solution here (rather than checking every possible event).
However, bug 545429 indicates that there is a general problem that popups are
not closed when unrelated commands are executed, so doing this in
OnContainerFocusOutEvent sounds good to me.

(I hope it will also mean that CaptureRollupEvents will no longer need to grab
pointer and keyboard when !aConsumeRollupEvent - currently it is consuming
rollup events on other apps, stealing Alt-TAB for window switch, etc. - but
that's another bug.)

I can't think why this should be necessary on
activate/OnContainerFocusInEvent, and I don't think the changes to OnWindowStateEvent are necessary as the window would lose focus when minimized or hidden, or are we opening popups when we don't have focus?
Detecting maximize seems unnecessary if rollup is going to happen on resize.

I'm not clear why rollup should also happen on move/resize but I think that
should be fine.
OnConfigureEvent is currently not detecting resizes when the top-left stays
the same, though.

Comment 13

[Neil Deakin]

(In reply to comment #12)
> I can't think why this should be necessary on
> activate/OnContainerFocusInEvent

Probably not needed, but isn't harmful. Would you prefer I removed it from focusin?


> and I don't think the changes to OnWindowStateEvent are necessary as
> the window would lose focus when minimized or hidden

I wasn't sure if minimizing removed the focus from a window on all Linux environments (on Windows, focus isn't removed and is on the minimized button)


, or are we opening popups when we don't have focus?

Popups can be opened in background windows, tooltips for instance.


> Detecting maximize seems unnecessary if rollup is going to happen on resize.

I can check for that state.

> I'm not clear why rollup should also happen on move/resize but I think that
> should be fine.

Either that or the visible popups would need to be moved relative to the new position of the window. Both Windows and Mac close popups when a popup is moved or resized.

> OnConfigureEvent is currently not detecting resizes when the top-left stays
> the same, though.

Ah, I should move the rollup check before the bounds check, or some other event is needed here?

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to comment #13)
> (In reply to comment #12)
> > I can't think why this should be necessary on
> > activate/OnContainerFocusInEvent
> 
> Probably not needed, but isn't harmful. Would you prefer I removed it from
> focusin?

I would prefer it is removed thanks.  If it's not necessary then I'd feel
safer from (perhaps unlikely) unexpected order of focus and mouse events.

> > and I don't think the changes to OnWindowStateEvent are necessary as
> > the window would lose focus when minimized or hidden
> 
> I wasn't sure if minimizing removed the focus from a window on all Linux
> environments (on Windows, focus isn't removed and is on the minimized button)

I think we should be safe there.  Focus is lost when the window becomes not
viewable (according to man XSetInputFocus).  (Not viewable means it or one of
its ancestors is unmapped, rather than any indication of being obscured by
other windows, etc.)  GDK assumes that iconified (minimized) windows are
unmapped (and wouldn't notify us if they were mapped) and this is consistent
with ICCCM http://tronche.com/gui/x/icccm/sec-4.html#s-4.2.5

> , or are we opening popups when we don't have focus?
> 
> Popups can be opened in background windows, tooltips for instance.

Ah yes, tooltips are a good example of popups without focus, but I don't think
we capture rollup events for tooltips.  Tooltips should be hidden on mouse
LeaveNotify I think (though apparently that's not always happening - bug
440652).

Grabbing the pointer (as we do for capturing rollup events) from unfocused
windows would be quite nasty.  If we are doing that then I think that should
be fixed (in a different way in a different bug).

> > I'm not clear why rollup should also happen on move/resize but I think that
> > should be fine.
> 
> Either that or the visible popups would need to be moved relative to the new
> position of the window. Both Windows and Mac close popups when a popup is moved
> or resized.

OK, thanks.  Consistency with Windows/Mac sounds good/simplest here.

> > OnConfigureEvent is currently not detecting resizes when the top-left stays
> > the same, though.
> 
> Ah, I should move the rollup check before the bounds check, or some other event
> is needed here?

AFAIK moving the rollup check to before the bounds check should be fine here.
OnConfigureEvent is called "when the size, position or stacking of the
widget's window has changed".  I don't actually see any events on stacking
(z-)order change on my system here, possibly because it's actually the window
manager's parent frame window that is changing order.  Stacking order change
sounds a good time to rollup if these events are ever received, and if the
order of stacking and click events is suitable, so I don't know whether or not
to recommend checking for changes in bounds (size or position).

Actually the bounds check looks wrong for toplevel windows anyway, as the
aEvent.x has coords relative to the parent window but mBounds.x is root window
(screen) coords.

I'm OK if you just move rollup check to before the bounds check and we can see
how that goes.

Comment 15

[Neil Deakin]

Attachment: patch, v3

Comment 16

[Neil Deakin]

I'm continuing to investigate an odd test failure in test_largemenu.xul caused by this patch.

Comment 17

[Neil Deakin]

Attachment: just adding a delay to the test does the trick

The problem with the test is caused because GTK or the window manager updates its position/size data directly when a window/popup is moved, but doesn't fire the configure notification events until a bit later. Thus, when trying to get the window position, we get the new coordinates before the event has occured, and think the window has moved correctly. When those events do arrive, we've already moved on to opening a popup, and it is then closed due to this fix.

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4097291a632c

Comment 19

[u88484]

Verified fixed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre