Closed Bug 127013 Opened 19 years ago Closed 13 years ago

fullScreen property operates inconsistently for frames

Categories

(Core :: XUL, defect)

x86
Windows 95
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: ventnor.bugzilla)

References

Details

Attachments

(2 files, 3 obsolete files)

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
*** Bug 127440 has been marked as a duplicate of this bug. ***
Neil: since bug 127405 was fixed, we can't do fullScreen = true/false outside 
mozilla now. should we close this bug?
Not necessarily. At least, if you browse to about:config and then
javascript:fullScreen=true then the window will go full screen.
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1884

We shouldn't do that before we are the root window.
Assignee: hewitt → kyle.yuan
Keywords: review
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?
Attachment #92368 - Flags: needs-work+
Attachment #92368 - Attachment is obsolete: true
Comment on attachment 92506 [details] [diff] [review]
changed GetFullScreen as well (per Neil's suggestion)

this patch doesn't solve the problem.
Attachment #92506 - Attachment is obsolete: true
=> default owner
Assignee: kyle.yuan → jaggernaut
Keywords: review
*** Bug 227553 has been marked as a duplicate of this bug. ***
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?
Summary: fullScreen = false doesn't turn full screen mode off → fullScreen property operates inconsistently for frames
Attached patch New patchSplinter Review
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 ;) )
Assignee: jag → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #264942 - Flags: superreview?(jst)
Attachment #264942 - Flags: review?(jst)
Comment on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst
Attachment #264942 - Flags: superreview?(jst)
Attachment #264942 - Flags: superreview+
Attachment #264942 - Flags: review?(jst)
Attachment #264942 - Flags: review+
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 on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst
(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.
Whiteboard: [checkin needed]
Attached patch Mochitest (obsolete) — Splinter Review
Flags: in-testsuite?
Now that the tree has reopened, can someone with CVS rights please check-in this patch?
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Comment on attachment 266033 [details] [diff] [review]
Mochitest

Thanks!
Attachment #266033 - Flags: review?(martijn.martijn)
Attached patch Mochitest v2Splinter Review
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
Attachment #266033 - Attachment is obsolete: true
Attachment #267220 - Flags: review?(jst)
Attachment #266033 - Flags: review?(martijn.martijn)
Attachment #267220 - Attachment is patch: true
Attachment #267220 - Attachment mime type: application/octet-stream → text/plain
(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.
Attachment #267220 - Flags: review?(jst) → review+
You need to log in before you can comment on or make changes to this bug.