Closed Bug 403706 Opened 14 years ago Closed 14 years ago

popup positioning clamps to old screen size, ignoring dynamic changes

Categories

(Core :: Widget: Gtk, defect, P4)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: dbaron, Unassigned)

References

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Dynamic changes in screen size have stopped being reflected in the position of popup menus.  (Popup menus are always positioned within the screen.  However, this algorithm seems to be using the screen size at startup.)

Steps to reproduce:
 1. resize screen smaller
 2. start firefox
 3. resize screen larger
 4. move firefox window to or past the right edge of the screen, so the "Help" on the menu bar is near the right edge
 5. click on the "Help" in the menu bar

Expected results:  Help menu opens near right edge of screen.

Actual results: Help menu opens a good bit to the left, near the right edge of the small screen size that we were running at when we started firefox.

This worked correctly in Linux nightly 2007-05-02-04-trunk and regressed in Linux nightly 2007-05-03-04-trunk.

The checkin window is:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-02+02%3A00&maxdate=2007-05-03+05%3A00&cvsroot=%2Fcvsroot
Flags: blocking1.9?
Note that the symptoms are worse when going from large to small -- then the popups end up off-screen.
Blocks: 318331
Presumably a regression from turning on Xinerama.  I'm not surprised -- I recall warning people (bug 376658 comment 1) about a whole bunch of things that worked in the non-Xinerama codepath that didn't work in the Xinerama-ifdef in one of the bugs about turning on Xinerama.
(For what it's worth, I only noticed this regression in the past week or two -- so probably something changed in an update to Fedora 7, or when I finally updated to the X server that shipped with Fedora 7, such that this broke for me.  But on my new configuration, the regression date is as-above.)
Component: XP Toolkit/Widgets: Menus → Widget: Gtk
QA Contact: xptoolkit.menus → gtk
David any thoughts on priority here?
+'ing with P4.  David please adjust priority as needed.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
We should definitely use the same dynamic approach in the Xinerama version, and deal with the _NET_WM_STRUT_PARTIAL stuff.

This patch is a kind of hack to take previous dynamic codepath in case there is only one screen and you have Xinerama enabled.

I think that you see the issue when upgrading to Fedora 7 because the Xinerama extension is now loaded by default, even if you have one screen.
Flags: tracking1.9+ → blocking1.9-
Flags: wanted-next+
Comment on attachment 288744 [details] [diff] [review]
take dynamic path in case there's only one screen

Yeah, I think we should do this, although I think we should also move some of the dynamic change handling from nsScreenGtk to nsScreenManagerGtk so that we pick up changes better (including, in the future, changes in number of screens).  I have some patches in my tree.

But r=dbaron on this.  (I was expecting screenInfo to be leaked, but I checked the source outside the context of the diff, and I see that this doesn't make it leak.)
Attachment #288744 - Flags: review+
Attachment #288744 - Flags: superreview?(roc)
Attachment #288744 - Flags: superreview?(roc) → superreview+
This series of 4 patches I'm attaching also fixes this bug; I think we should take both sets of patches, since we ought to handle dynamic changes in the Xinerama case.  These 4 patches don't get us all the way there, since there are probably some Xinerama notifications we should be listening for as well, but they move the notification handling into the screen manager and make us listen to the current notifications in all cases rather than just the non-Xinerama case.

(As I said above, I think we should still take both sets of patches.)

This patch in particular makes other cleanup easier, if my memory is correct (I wrote this a while ago).
Attachment #325336 - Flags: superreview?(roc)
Attachment #325336 - Flags: review?(roc)
This is progress towards allowing the number of screens to vary dynamically.  If the number of screens goes down, we'll let mNumScreens fall, but keep the cached screen objects around in case it goes back up again.  This seems like the nice thing to do (particularly if it goes down and up again in some sort of reconfiguration blip, which wouldn't surprise me).

It doesn't actually get us into that situation yet, though.
Attachment #325337 - Flags: superreview?(roc)
Attachment #325337 - Flags: review?(roc)
Attachment #325338 - Flags: superreview?(roc)
Attachment #325338 - Flags: review?(roc)
This move reinitialization from the screen to the screen manager so that we can handle reinitialization in the case of having multiple screens.

I could really use some help testing all of this for the multiple screens case...
Attachment #325340 - Flags: superreview?(roc)
Attachment #325340 - Flags: review?(roc)
Attachment #325336 - Flags: superreview?(roc)
Attachment #325336 - Flags: superreview+
Attachment #325336 - Flags: review?(roc)
Attachment #325336 - Flags: review+
Why do we want to be able to reuse the screen objects if the number of screens falls and then increases again? It's not a performance issue, I assume.
Comment on attachment 325338 [details] [diff] [review]
patch 4: don't leak the result of PR_LoadLibrary for the xinerama library

I'm not sure why I said there's no other way to do this...
Attachment #325338 - Flags: superreview?(roc)
Attachment #325338 - Flags: superreview+
Attachment #325338 - Flags: review?(roc)
Attachment #325338 - Flags: review+
(In reply to comment #12)
> Why do we want to be able to reuse the screen objects if the number of screens
> falls and then increases again? It's not a performance issue, I assume.

At the time it seemed like it might be less likely to confuse callers, but I could just go get rid of mNumScreens and remove the extra screens from the array.
If we only update the screens array from the event loop (i.e. not while JS is running synchronously), I think that's fine.
Attachment #325337 - Attachment is obsolete: true
Attachment #329329 - Flags: superreview?(roc)
Attachment #329329 - Flags: review?(roc)
Attachment #325337 - Flags: superreview?(roc)
Attachment #325337 - Flags: review?(roc)
Attachment #325340 - Attachment is obsolete: true
Attachment #329332 - Flags: superreview?(roc)
Attachment #329332 - Flags: review?(roc)
Attachment #325340 - Flags: superreview?(roc)
Attachment #325340 - Flags: review?(roc)
Attachment #329329 - Flags: superreview?(roc)
Attachment #329329 - Flags: superreview+
Attachment #329329 - Flags: review?(roc)
Attachment #329329 - Flags: review+
Attachment #329332 - Flags: superreview?(roc)
Attachment #329332 - Flags: superreview+
Attachment #329332 - Flags: review?(roc)
Attachment #329332 - Flags: review+
Depends on: 448076
Duplicate of this bug: 414844
Duplicate of this bug: 376764
Depends on: 451985
You need to log in before you can comment on or make changes to this bug.