Closed Bug 522956 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: elmar.ludwig, Assigned: enndeakin)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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...
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?
blocking2.0: --- → ?
Duplicate of this bug: 538265
(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
Duplicate of this bug: 545988
Duplicate of this bug: 550513
Neil, is this a problem with panels on Linux?
Yes, similar to bug 545429.
Duplicate of this bug: 540096
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
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.
Assignee: nobody → enndeakin
Blocks: 545429
blocking2.0: ? → betaN+
Attached patch something like so (obsolete) — Splinter Review
This should roll up open menus/popups upon move, resize, activate, deactivate, close, minimize and maximize on a toplevel window or dialog.
Attachment #464484 - Flags: review?(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.
(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?
(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.
Attached patch patch, v3 (obsolete) — Splinter Review
Attachment #464484 - Attachment is obsolete: true
Attachment #465653 - Flags: review?(karlt)
Attachment #464484 - Flags: review?(karlt)
Attachment #465653 - Flags: review?(karlt) → review+
I'm continuing to investigate an odd test failure in test_largemenu.xul caused by this patch.
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.
Attachment #465653 - Attachment is obsolete: true
Attachment #475626 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4097291a632c
Status: NEW → RESOLVED
Closed: 12 years ago
Component: General → Widget: Gtk
Keywords: checkin-needed
Product: Firefox → Core
QA Contact: general → gtk
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Depends on: 597029
Verified fixed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
Depends on: 551540
Depends on: 618248
Depends on: 631518
You need to log in before you can comment on or make changes to this bug.