403706 - popup positioning clamps to old screen size, ignoring dynamic changes

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that the symptoms are worse when going from large to small -- then the popups end up off-screen.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 4

[Mike Schroepfer]

David any thoughts on priority here?

Comment 5

[Damon Sicore (:damons)]

+'ing with P4.  David please adjust priority as needed.

Comment 6

[Sylvain Pasche]

Attachment: take dynamic path in case there's only one screen

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: convert from nsISupportsArray to nsCOMArray

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).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: allow the screen manager array to be larger than mNumScreens

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: don't leak the result of PR_LoadLibrary for the xinerama library

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: move reinitialization from the screen to the screen manager

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...

Comment 12

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

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 13

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

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...

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 15

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

If we only update the screens array from the event loop (i.e. not while JS is running synchronously), I think that's fine.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: remove mNumScreens

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: don't leak the result of PR_LoadLibrary for the xinerama library

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: move reinitialization from the screen to the screen manager

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/ef86a928ec61
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8e1ffe9e3b63
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8be867e8b341
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4f8c69ab3e26
http://hg.mozilla.org/mozilla-central/index.cgi/rev/3cea658b6377

All 5 patches landed.  (Thanks for yours, Sylvain.)