410337 - Windows opened with centerscreen appearing on wrong screen

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Sylvain Pasche]

Attachment: set the parent when using centerscreen

I can see the issue on Mac or Linux (didn't try Windows). If you have multiple monitors, try moving a Firefox window on each screen and open the preferences from there.
You will notice that the preference window is always opened on the same screen. The more logical behavior would be to open it on the same screen as where the opener Firefox window is located.

I could see that the kind of windows which are wrongly positioned this way are the one opened with the "centerscreen" feature given to window.open/openDialog.

After some debugging, I could see that windows opened this way are positioned in nsXULWindow::Center (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/appshell/src/nsXULWindow.cpp&rev=1.178&mark=627#627). This method looks at the parent window to find the screen where it should be centered. However, I saw that the parent window was null in such a situation (so it always uses the same default screen instead).

In fact, the parent is null unless the opened window has a "dependent" feature. The attached patch sets the parent also when the window has a "centerscreen" feature.

I tried to look at the side effects of having a parent set in nsXULWindow. I don't have the impression it can be too bad in our situation: http://mxr.mozilla.org/mozilla/search?string=mParentWindow&find=nsXULWindow.cpp&findi=&filter=&tree=mozilla

Comment 2

[Neil Deakin]

No that can't be right. That would cause the centerscreen window to behave as a dependant window (which will close when the parent does), which we do not want.

Comment 3

[Sylvain Pasche]

Comment on attachment 294967 [details] [diff] [review]
set the parent when using centerscreen

Apparently there's a platform difference between Linux and Windows. I tried opening a dependent window on Linux, and I can still close the parent window without the opened window being closed (is this a bug?).

On Windows, closing the parent of a dependent windows closes the child as expected, so does the centerscreen window with this patch applied as you noticed.

Do you have an idea how this can be dealt with?

Another approach I see would be to have a method like screenUnderPointer() on nsIScreenManager which would return the screen containing the mouse pointer instead of using nsIScreenManager::primaryScreen which will always return the same screen.

Comment 4

[Sylvain Pasche]

This bug is more visible on Linux because of this specific behavior:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/appshell/src/nsXULWindow.cpp&rev=1.178&mark=968-974#968

On that platform, windows opened without parent (such as the Page Info, preferences, ...) won't have their position restored from the localstore. This means that if you open preferences and move it to the other screen, its position won't be remembered the next time you open it (its position is remembered on Mac and Windows).

I'm wondering if this platform divergence is still useful with recent window managers.

Comment 5

[Sylvain Pasche]

Attachment: use XUL persisted position for centerscreen windows

This patch brings Linux to the same behavior as Windows/Mac for windows opened with "centerscreen". This means that if you open preferences, move it to another screen, close it and reopen it, it will appear at the position where you closed it.

For non-dependent centerscreen windows, a better fix would be to save the screen on which the parent is located, and use that when centering the window instead of the default screen. nsIScreenManager can't deal with screen indexes, so we could save the nsRect of the parent window and use nsIScreenManager::screenForRect in nsXULWindow::Center with that saved rectangle.

On Windows/Mac (and Linux to some extent) I'm not too happy with the way windows are positioned: If you open pageInfo on screen A, close it and then open again pageInfo from a browser window on screen B, it will appear back on screen A because its position is persisted. For some types of windows (Error Console), this behavior makes more sense (it's nice to have the console always use the same position on one of your screen).
This is a more general issue than this specific bug that could be discussed on a more visible place. One idea would be to have a class of windows with per-screen persisted position (page info, view source, preferences) and another class with global persisted position like we have now (except on Linux).

Comment 6

[Sylvain Pasche]

Note: see also bug 394768 which is somewhat related.

Comment 8

[Jaroslav Zaruba]

The same happens with alert/confirm/prompt dialog-boxes.

Comment 9

[David Mitchell]

This bug is driving me bonkers. I ask for prompts when accepting new cookies, so I hit this all the time. Is there some config knob I can tweak to fix this, or is the only fix to build firefox from scratch with the above patch? Pretty much any of the proposed solutions would be better. Attach the dialog to the parent window. Remember it's position. Let my WM do it's job. Anything would be preferable to the current code.

Comment 10

[Sylvain Pasche]

Are you sure that window is opened with centerscreen?  Are you using Firefox, I didn't see an option to have a prompt when getting new cookies (or maybe that's an extension?)

Comment 11

[David Mitchell]

Preferences / Privacy / Cookies
 Accept cookies from sites: checked
 Accept third-party cookies: checked
 Keep until: ask me every time.

The dialog which asks has a check to remember your choice, which is then viewable via the 'Exceptions...' button. The windows always open centered on my rightmost screen. Same for the 'Preferences' dialog, 'About Mozilla Firefox', etc. They all open exactly centered on the right screen no matter where my pointer is or where my browser window is. This is all with Firefox 3.0.6 under Ubuntu 8.10.

Comment 12

[Sylvain Pasche]

ok, thanks for the detail.  Replying to my comment 10, it is opened with centerscreen: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsCookiePromptService.cpp#112

Note that specifically for the cookie window, it is now opened with a parent so it won't appear on the center of the main screen but rather centered above your browser window (bug 470356). David, looks like it's time for you to try Firefox 3.1 (where it is fixed).

Comment 13

[David Mitchell]

Firefox 3.1 does seem better with respect to the cookie windows. Other dialogs still open on the right screen irrespective of the location of the browser window. That annoying, but since I don't deal with those on a regular basis it isn't a huge problem.

It does seem odd that Firefox is neither allowing my WM to place the windows nor remembering where I move the windows to. Oh well. Thanks for the help!

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: proposed patch

This patch passes the parent window further down (but still doesn't use it as the widget parent window unless the dependent flag was passed) and saves its screen rect in the new child window so that the child window can use that to determine the screen of what I've called the 'opener' and center on that screen.

Not totally sure about reviewer here.

Comment 16

[Neil Deakin]

Comment on attachment 398335 [details] [diff] [review]
proposed patch

>+  nsCOMPtr<nsIBaseWindow> base(do_QueryInterface(aOpener, &rv));
>+  if (NS_SUCCEEDED(rv) && base) {

No real need to check for failure here. Usually, we just check if base is set.

> {
>+  mOpenerScreenRect.x = mOpenerScreenRect.y = 0;
>+  mOpenerScreenRect.width = mOpenerScreenRect.height = 0;
> }

More readable to just say mOpenerScreenRect = nsRect()

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to comment #16)
> (From update of attachment 398335 [details] [diff] [review])
> >+  nsCOMPtr<nsIBaseWindow> base(do_QueryInterface(aOpener, &rv));
> >+  if (NS_SUCCEEDED(rv) && base) {
> 
> No real need to check for failure here. Usually, we just check if base is set.

Ok.

> > {
> >+  mOpenerScreenRect.x = mOpenerScreenRect.y = 0;
> >+  mOpenerScreenRect.width = mOpenerScreenRect.height = 0;
> > }
> 
> More readable to just say mOpenerScreenRect = nsRect()

The default constructor for nsRect initializes everything to 0, so I just removed these lines.

Comment 18

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/1deff8db4352

Comment 19

[Timothy Nikkel (:tnikkel)]

Windows opened with centerscreen and null parent will still be opened on the primary screen. I filed bug 519404 to pass a non-null parent when we can.