509828 - Specifying left and top features in openWindow doesn't work when screenX and screenY attributes are set on the <window> element and are set to be persisted

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Nochum Sossonko [:Natch]]

See bug 465186 comment 6 and bug 465186 comment 8.

Comment 1

[Nochum Sossonko [:Natch]]

Update: I'm finding that no matter what, i.e. even without having screenX and screenY defined on the <window> it still won't use the left and top features for some reason...

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is only an issue if the window has its screen position persisted, right?

Comment 3

[Nochum Sossonko [:Natch]]

This doesn't work on any level afaict, I have a console "extension" that doesn't specify screenX/screenY on the window (and doesn't persist it), and I'm using this js to open it:

javascript:window.openDialog("chrome://console/content/console.xul",null,"chrome,dialog=no,all,screenX=250,screenY=250",null);void%200;

It doesn't work and won't open it at that location, it opens at the default location every time. I've also tried using window watcher's openWindow and got the same results.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Odd.  Are you ending up under nsChromeTreeOwner::SetPosition with those values?  What happens after that?

Comment 5

[Nochum Sossonko [:Natch]]

(In reply to comment #4)
> Odd.  Are you ending up under nsChromeTreeOwner::SetPosition with those values?
>  What happens after that?

Yes, that is called. However, x is 800 and y is 600 so the problem must be before that.

Comment 6

[Nochum Sossonko [:Natch]]

Sorry, big mistake on my part. This is all correct, as this printf spew shows:

In OpenWindow, |features|, centerscreen,chrome,modal,resizable=no
In OpenWindow, |chromeFlags|, 4160750598
In OpenWindow, |CalcSizeSpec|, sizeSpec.mLeft: 1827320, sizeSpec.mTop: 58468384
In SizeOpenedDocShellItem |treeOwnerAsWin->GetPositionAndSize|, left=0, top=0
In SizeOpenedDocShellItem |NSToIntRound|, left=0, top=0
In OpenWindow, |features|, chrome,dialog=no,all,screenX=400,screenY=200,width=
In OpenWindow, |chromeFlags|, 2147487742
In OpenWindow, |CalcSizeSpec|, sizeSpec.mLeft: 400, sizeSpec.mTop: 200
In SizeOpenedDocShellItem |treeOwnerAsWin->GetPositionAndSize|, left=0, top=0
In SizeOpenedDocShellItem |NSToIntRound|, left=400, top=200
In SizeOpenedDocShellItem |screen->GetAvailRect|, screenLeft=0, screenTop=0
In SizeOpenedDocShellItem after switch block, left=400, top=200, postionSpecified=1
In SetPosition, x=400, y=200

JS used: javascript:window.openDialog("chrome://console/content/console.xul",null,"chrome,dialog=no,all,screenX=400,screenY=200,width=",null);void 0;

So it has to be that it isn't _actually_ being set, probably a widget issue or something.

Comment 7

[Nochum Sossonko [:Natch]]

Ok, it seems the culprit is nsXULWindow->SetPosition, see the following spew:

In OpenWindow, |features|, chrome,dialog=no,all,screenX=400,screenY=200,width=
In OpenWindow, |chromeFlags|, 2147487742
In OpenWindow, |CalcSizeSpec|, sizeSpec.mLeft: 400, sizeSpec.mTop: 200
In SizeOpenedDocShellItem |treeOwnerAsWin->GetPositionAndSize|, left=0, top=0
In SizeOpenedDocShellItem |NSToIntRound|, left=400, top=200
In SizeOpenedDocShellItem |screen->GetAvailRect|, screenLeft=0, screenTop=0
In SizeOpenedDocShellItem after switch block, left=400, top=200, postionSpecified=1
In SetPosition, x=400, y=200
In nsXULWindow::SetPosition, aX=400, aY=200
In nsXULWindow::SetPosition, error return code=0
Appending Initial Items
In nsXULWindow::SetPosition, aX=235, aY=159
In nsXULWindow::SetPosition, error return code=0
In OpenWindow, |features|, centerscreen,chrome,modal,resizable=no
In OpenWindow, |chromeFlags|, 4160750598
In OpenWindow, |CalcSizeSpec|, sizeSpec.mLeft: 1235832, sizeSpec.mTop: 66070592
In SizeOpenedDocShellItem |treeOwnerAsWin->GetPositionAndSize|, left=0, top=0
In SizeOpenedDocShellItem |NSToIntRound|, left=0, top=0
In nsXULWindow::SetPosition, aX=536, aY=308
In nsXULWindow::SetPosition, error return code=0
In OpenWindow, |features|, chrome,dialog=no,all,screenX=700,screenY=800,width=
In OpenWindow, |chromeFlags|, 2147487742
In OpenWindow, |CalcSizeSpec|, sizeSpec.mLeft: 700, sizeSpec.mTop: 800
In SizeOpenedDocShellItem |treeOwnerAsWin->GetPositionAndSize|, left=0, top=0
In SizeOpenedDocShellItem |NSToIntRound|, left=700, top=800
In SizeOpenedDocShellItem |screen->GetAvailRect|, screenLeft=0, screenTop=0
In SizeOpenedDocShellItem after switch block, left=700, top=800, postionSpecified=1
In SetPosition, x=700, y=800
In nsXULWindow::SetPosition, aX=700, aY=800
In nsXULWindow::SetPosition, error return code=0
Appending Initial Items
In nsXULWindow::SetPosition, aX=235, aY=159
In nsXULWindow::SetPosition, error return code=0

So basically, nsXULWindow gets the correct left and top the first time, but then gets called again for some reason and _always_ sets it to 235/159.

Comment 8

[Nochum Sossonko [:Natch]]

Err, ok. The issue is only when there are or _ever_were_ persistent values. I originally had the extension persist the screenX and screenY, then I removed them, but it seems they still persisted. nsXULWindow calls PersistentAttributesDirty and SavePersistentAttributes one of which resets the screenX and screenY.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So...  I _think_ this is the designed behavior of persistence.  But who knows?

Comment 10

[[e-mail]]

Well, maybe nobody cared what happened when you mixed persistence and features before, despite the -width and -height parameters of Mozilla 1.x ... presumably they only ever affected the first ever launch or something.

Comment 11

[Nochum Sossonko [:Natch]]

Attachment: patch

So, basically what happens is that as soon as the chrome is loaded (onChromeLoaded in nsXULWindow) screenX, screenY, height and width are updated based on the attributes. That's what overrides the window open at that position, it doesn't really have to do with persistence (afaict) just if there's a screenX/screenY attribute on the window element it will adjust the position to the specified value. Of course, if the values are persisted it will adjust it to the persisted values.

This patch adds a method to nsIBaseWindow that instructs the window not to adjust to the screenX/screenY or left/top attribute values.

Comment 12

[Boris Zbarsky [:bzbarsky]]

nsIBaseWindow is effectively frozen .  You can't change it.

Comment 13

[Nochum Sossonko [:Natch]]

ok, I'll move the method to nsIDocShellTreeOwner, and have that call through to a function on nsXULWindow through nsChromeTreeOwner, is that ok?

Also, should I add "@status frozen" to nsIBaseWindow (or at least some sort of annotation)?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Can the xul window just keep track of whether its position has been set explicitly and not override it if it has?

See bug 269323.

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2

Yeah, this seems to work fine in my tests. I opted on not persisting the one-time values that are set from someone else (i.e. not from the xul attribute positions).

What do you think about this?

Note: the SetPositionAndSize changes aren't absolutely necessary for this bug, however I threw them in for consistency.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406594 [details] [diff] [review]
patch ver. 2

I like it.

Comment 17

[Nochum Sossonko [:Natch]]

Thanks!

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a847cf5b4669

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: mochitest

Here's a test for this functionality. I'm throwing this to the tryserver now, tinderboxen don't always like it when you open windows during the test.

One note: When you specify a size in the openDialog call, the window actually ends up a bit bigger. Is that a bug (which I should todo on) or is that just inevitable?

Also, I'm gonna add the dev-doc-needed keyword so that devs get the line on this change.

dev-doc-needed: in 1.9.2 window.[open|openDialog] calls that set window size or position override the attributes on the xul <window> element. These positions aren't persisted (i.e. the attributes aren't updated) unless the window is moved after opening.

Comment 20

[Nochum Sossonko [:Natch]]

Tryserver results:

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1255897419.1255908045.31520.gz (Success modulo unrelated errors)

MacOSX: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1255897419.1255907624.27147.gz (Success

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1255897419.1255904696.28013.gz (Failed)

The linux failure is an obvious fluke, for some reason the window.screenX/screenY values aren't reporting correctly: "The window should be placed now at x=100px - got 0, expected 100"

Not sure what to do about that failure, perhaps just add a setTimeout?

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5cfcaf8c4b10

Comment 22

[Florian Quèze [:florian]]

Has the patch here fixed the very old bug 35703?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Test looks ok for the most part, except I bet the Linux failure is because on Linux behavior for the persisted stuff is in fact different, no?

Comment 24

[Nochum Sossonko [:Natch]]

I was thinking the same, but that wouldn't explain the result of the test on Linux, it was reporting [height=250,width=250,screenX=0,screenY=0]. So screenX and screenY were incorrect, while height and width were correct. Also, the values reported for screenX/screenY were pretty random it seems (0,0), why would that happen? That seems very odd to me. Can someone with a linux box perhaps test the result of this patch, either with the test, or by simply typing in the error console:

window.openDialog("chrome://global/content/console.xul",null,"chrome,dialog=no,all,screenX=200,screenY=200,height=600,width=800");

When the error console opens, where does it open?

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1255897419.1255904696.28013.gz#err0

(In reply to comment #22)
I don't think this fixed bug 509828, but I'll give it a shot sometime this week and update that bug.

Comment 25

[Nochum Sossonko [:Natch]]

(In reply to comment #24)
Err, bug 35703...

Comment 26

[Boris Zbarsky [:bzbarsky]]

> So screenX and screenY were incorrect, while height and width were correct.

Right; the thing that tends to get ignored on Linux last I checked is the positioning, not the sizing.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #24)
> window.openDialog("chrome://global/content/console.xul",null,"chrome,dialog=no,all,screenX=200,screenY=200,height=600,width=800");
> 
> When the error console opens, where does it open?

It opens at the specified position.

Comment 28

[Nochum Sossonko [:Natch]]

Just tried this on Linux (got me a Linux box), and its working perfectly, screenX and screenY are correct. What I'm using is the url I provided in comment 24, then I typed this into the error console to get the response:

(function(){return top.screenX + " " + top.screenY})();

I realized, however, that Linux doesn't allow you to place the window off-screen, when I tried using too-high or too-low numbers it wouldn't report correctly (although I didn't get 0,0). I'm going to look a little more in to this, it may just be that I need to wait for a later event ("DOMContentLoaded"?), I'll try to play around with this.

Comment 29

[Nochum Sossonko [:Natch]]

Using the following code seems to work fine for me:

javascript:var focused = false, loaded = false;var win = window.openDialog("chrome://global/content/console.xul",null,"chrome,dialog=no,all,screenX=80,screenY=80,height=600,width=800");win.addEventListener("load",function(){if(focused)alert(win.screenX); else loaded = true;},false);win.addEventListener("focus",function(){if(loaded)alert(win.screenX);else focused = true;},false);void 0;

I am at a loss as to why the tryserver doesn't like the test. I'll fix it up a bit and repost to try, see if it was a fluke perhaps, or maybe something else...

Comment 30

[Boris Zbarsky [:bzbarsky]]

What window manager is the tryserver running?  Is it identical to the one you're running?

Comment 31

[Nochum Sossonko [:Natch]]

Where can I find out which window manager tryserver is running? As for me, I'm using Ubuntu Hardy Heron (8.04) which comes with GNOME. I haven't really done anything to it, so it should match whatever the default package comes with...

Comment 32

[Boris Zbarsky [:bzbarsky]]

Alice, any idea on comment 31?

Comment 33

[Nochum Sossonko [:Natch]]

I'm dumb :(

The error was _not_ in the part of the test that specified the position of the window during opening, the error was in the part where the xul attribute is supposed to decide the position of the window. *That* is clearly intentional and part of this, http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp?mark=1015-1021#1013 , ifdef. That's probably what bz was trying to point out... ##headdesk##

What now, exclude that part of the test from running on Linux?

Comment 34

[Boris Zbarsky [:bzbarsky]]

That seems like the simplest approach, yeah...

Can it be rolled out into a separate test?  Then you can exclude easily on the makefile level...

Comment 35

[Nochum Sossonko [:Natch]]

Attachment: mochitest

Here's the updated mochitest. This uses nsIXULRuntime's OS string as the test for Linux.

(In reply to comment #34)
> Can it be rolled out into a separate test?  Then you can exclude easily on the
> makefile level...

It's only two of the checks, and I'd have to add another test just for that, this seemed easier. If you would rather have me put those two in another file anyhow, that's fine by me as well.

Some observations on this patch in general (possible follow-up bugs):

1) If we set mIgnoreXULPosition, should we consider the window intrinsically sized and not SizeToContent in OnChromeLoaded[1]?

2) When the window is blurred and refocused (in other words, on every refocus of the window) mPersistentAttributesDirty is "re-dirtied"[2]. So, essentially, the idea of not persisting the attributes is useless in that case, is that a problem? Why should we be behaving like that anyhow?

1: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1004
2: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWebShellWindow.cpp#432

If any of these issues warrant another bug, please let me know.

Comment 36

[Nochum Sossonko [:Natch]]

(In reply to comment #35)
> 1) If we set mIgnoreXULPosition, should we consider the window intrinsically
> sized and not SizeToContent in OnChromeLoaded[1]?

that should read: "If we set mIgnoreXULSize..."

Also, please ignore the extraneous private browsing makefile change, it belongs in another bug...

Comment 37

[[e-mail]]

(In reply to comment #35)
> 1) If we set mIgnoreXULPosition, should we consider the window intrinsically
> sized and not SizeToContent in OnChromeLoaded[1]?
Yes, it looks like there's a fair bit of overlap between mIntrinsicallySized and mIgnoreXULSize, and indeed you could write
if (!mIgnoreXULSize && !LoadSizeFromXUL()) {
  ...
    markupViewer->SizeToContent();
}
thus "eliminating" mIntrinsicallySized.

Comment 38

[[e-mail]]

I say "eliminating" in quotes because nsAppShellService sets mIntrinsicallySized because most of its callers tell it to. There is one caller that doesn't but it doesn't get used in Firefox (it predates tabbed browsing).

Comment 39

[Nochum Sossonko [:Natch]]

Filed bug 523909 and bug 523951 on the two issues in comment 35.

Comment 40

[Nochum Sossonko [:Natch]]

Attachment: mochitest

Refreshed the test to take out the pb stuff.

Comment 41

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b6e85128da39
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9adb4e79c859

Comment 42

[Eric Shepherd [:sheppy]]

Am I correct in reading the comments and the patch that this makes it so that overriding the default left and top using features in openWindow() calls works, and doesn't overwrite the persisted position, so that when the window is opened next time, it uses its previously persisted position?

Comment 43

[Eric Shepherd [:sheppy]]

For doc purposes, this is where work will need to be done:

https://developer.mozilla.org/en/XUL_Tutorial/Features_of_a_Window

Comment 44

[Nochum Sossonko [:Natch]]

(In reply to comment #42)
Yes, see comment 19. Although it may be worthwhile to note bug 523951, which kind of renders the non-persisting thing useless since it _will_ persist on every window activation.

Comment 45

[Eric Shepherd [:sheppy]]

I've updated the documentation here:

https://developer.mozilla.org/en/DOM/window.open#Position_and_size_features
https://developer.mozilla.org/en/XUL_Tutorial/Features_of_a_Window

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: trivial followup: reorder constructor initializer-list to fix warning

This patch introduced the following build warning:

> /mozilla/xpfe/appshell/src/nsXULWindow.h: In constructor 'nsXULWindow::nsXULWindow(PRUint32)':
> /mozilla/xpfe/appshell/src/nsXULWindow.h:179: warning: 'nsXULWindow::mAppPerDev' will be initialized after
> /mozilla/xpfe/appshell/src/nsXULWindow.h:172: warning:   'PRPackedBool nsXULWindow::mIgnoreXULSize'
> /mozilla/xpfe/appshell/src/nsXULWindow.cpp:133: warning:   when initialized here

Attached followup patch fixes this.

Comment 47

[Daniel Holbert [:dholbert]]

Followup patch landed:
 http://hg.mozilla.org/mozilla-central/rev/a07e94220d47
 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8ab9a5daf9ec