127013 - fullScreen property operates inconsistently for frames

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Using Build ID: 2002022003
Steps to reproduce problem:
1. Open the JavaScript console
2. Evaluate "fullScreen = true"
3. Evaluate "fullScreen = false"
4. Evaluate "top.fullScreen = false"

Expected results: step 3 restores the window

Actual results: step 4 restores the window

Comment 1

[Olav Vitters]

*** Bug 127440 has been marked as a duplicate of this bug. ***

Comment 2

[Kyle Yuan]

Neil: since bug 127405 was fixed, we can't do fullScreen = true/false outside 
mozilla now. should we close this bug?

Comment 3

[[e-mail]]

Not necessarily. At least, if you browse to about:config and then
javascript:fullScreen=true then the window will go full screen.

Comment 4

[Kyle Yuan]

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1884

We shouldn't do that before we are the root window.

Comment 5

[Kyle Yuan]

Attachment: checks (aFullScreen == mFullScreen) after we were the root window

Comment 6

[[e-mail]]

Comment on attachment 92368 [details] [diff] [review]
checks (aFullScreen == mFullScreen) after we were the root window

Nice work but can you fix GetFullScreen as well?

Comment 7

[Kyle Yuan]

Attachment: changed GetFullScreen as well (per Neil's suggestion)

Comment 8

[Kyle Yuan]

Comment on attachment 92506 [details] [diff] [review]
changed GetFullScreen as well (per Neil's suggestion)

this patch doesn't solve the problem.

Comment 9

[Kyle Yuan]

=> default owner

Comment 10

[[e-mail]]

*** Bug 227553 has been marked as a duplicate of this bug. ***

Comment 11

[[e-mail]]

So just to be clear here, there are three code paths:
1. setting full screen forwards to top
2. unsetting full screen early returns when it shouldn't (except for top)
3. getting full screen always reads the local member (wrong except for top)

Would it be better to store the full screen state somewhere else?

Comment 12

[Michael Ventnor]

Attachment: New patch

Even better: we read the full screen flag using the same approach as setting it, and then just re-use it in setFullScreen.

Not only does this fix Neil's problem, but this fixes the broken readonly window.fullScreen for content (which I'm sure the many upcoming web-based Powerpoint killers can use ;) )

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst

Comment 14

[Martijn Wargers (dead)]

Michael, you need someone to check the patch in for you?
Also, I guess this needs some Mochikit tests, eventually: http://developer.mozilla.org/en/docs/Mochitest

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst

Comment 16

[Michael Ventnor]

(In reply to comment #14)
> Michael, you need someone to check the patch in for you?

Yes please.

> Also, I guess this needs some Mochikit tests, eventually:
> http://developer.mozilla.org/en/docs/Mochitest

I'll see what I can do.

Comment 17

[Michael Ventnor]

Attachment: Mochitest

Comment 18

[Michael Ventnor]

Now that the tree has reopened, can someone with CVS rights please check-in this patch?

Comment 19

[Martijn Wargers (dead)]

Checking in nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.936; previous revision: 1.935
done

Checked into trunk.

Thanks for the patch Michael! Also thank you for the Mochitest! I'll review that one.

Comment 20

[Michael Ventnor]

Comment on attachment 266033 [details] [diff] [review]
Mochitest

Thanks!

Comment 21

[Martijn Wargers (dead)]

Attachment: Mochitest v2

I noticed you didn't reset the window back after the test, so I added it. I also added a test whether the resize event is fired. I hope you don't mind I did this.

I added some very limited documentation here:
http://developer.mozilla.org/en/docs/DOM:window.fullScreen

Comment 22

[Michael Ventnor]

(In reply to comment #21)
> Created an attachment (id=267220) [details]
> Mochitest v2
> 
> I noticed you didn't reset the window back after the test, so I added it. I
> also added a test whether the resize event is fired. I hope you don't mind I
> did this.

That's fine. In fact I thought about that just after I hit submit, but never got around to fixing it.