The default bug view has changed. See this FAQ.

fullScreen property operates inconsistently for frames

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Michael Ventnor)

Tracking

Trunk
x86
Windows 95
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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

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

Comment 2

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

Comment 3

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

Comment 4

15 years ago
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

Comment 5

15 years ago
Created attachment 92368 [details] [diff] [review]
checks (aFullScreen == mFullScreen) after we were the root window

Updated

15 years ago
Keywords: review
(Reporter)

Comment 6

15 years ago
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+

Comment 7

15 years ago
Created attachment 92506 [details] [diff] [review]
changed GetFullScreen as well (per Neil's suggestion)
Attachment #92368 - Attachment is obsolete: true

Comment 8

15 years ago
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

Comment 9

15 years ago
=> default owner
Assignee: kyle.yuan → jaggernaut
Keywords: review
(Reporter)

Comment 10

14 years ago
*** Bug 227553 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 11

14 years ago
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
(Assignee)

Comment 12

10 years ago
Created attachment 264942 [details] [diff] [review]
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 ;) )
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
(Assignee)

Comment 16

10 years ago
(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]
(Assignee)

Comment 17

10 years ago
Created attachment 266033 [details] [diff] [review]
Mochitest
Flags: in-testsuite?
(Assignee)

Comment 18

10 years ago
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.

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
(Assignee)

Comment 20

10 years ago
Comment on attachment 266033 [details] [diff] [review]
Mochitest

Thanks!
Attachment #266033 - Flags: review?(martijn.martijn)
Created attachment 267220 [details] [diff] [review]
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
Attachment #266033 - Attachment is obsolete: true
Attachment #267220 - Flags: review?(jst)
Attachment #266033 - Flags: review?(martijn.martijn)

Updated

10 years ago
Attachment #267220 - Attachment is patch: true
Attachment #267220 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 22

10 years ago
(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.

Updated

10 years ago
Attachment #267220 - Flags: review?(jst) → review+
You need to log in before you can comment on or make changes to this bug.