Last Comment Bug 127013 - fullScreen property operates inconsistently for frames
: fullScreen property operates inconsistently for frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows 95
: -- normal (vote)
: ---
Assigned To: Michael Ventnor
: John Morrison
Mentors:
: 127440 227553 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-02-21 07:53 PST by neil@parkwaycc.co.uk
Modified: 2007-06-13 18:08 PDT (History)
7 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
checks (aFullScreen == mFullScreen) after we were the root window (1.02 KB, patch)
2002-07-23 02:38 PDT, Kyle Yuan
no flags Details | Diff | Review
changed GetFullScreen as well (per Neil's suggestion) (1.70 KB, patch)
2002-07-23 19:20 PDT, Kyle Yuan
no flags Details | Diff | Review
New patch (2.05 KB, patch)
2007-05-15 18:17 PDT, Michael Ventnor
jst: review+
jst: superreview+
Details | Diff | Review
Mochitest (1.98 KB, patch)
2007-05-25 00:01 PDT, Michael Ventnor
no flags Details | Diff | Review
Mochitest v2 (2.38 KB, patch)
2007-06-04 17:39 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
jst: review+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2002-02-21 07:53:50 PST
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 2002-02-23 10:51:58 PST
*** Bug 127440 has been marked as a duplicate of this bug. ***
Comment 2 Kyle Yuan 2002-07-20 00:26:06 PDT
Neil: since bug 127405 was fixed, we can't do fullScreen = true/false outside 
mozilla now. should we close this bug?
Comment 3 neil@parkwaycc.co.uk 2002-07-22 16:27:27 PDT
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 2002-07-23 02:36:33 PDT
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 2002-07-23 02:38:14 PDT
Created attachment 92368 [details] [diff] [review]
checks (aFullScreen == mFullScreen) after we were the root window
Comment 6 neil@parkwaycc.co.uk 2002-07-23 04:51:21 PDT
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 2002-07-23 19:20:30 PDT
Created attachment 92506 [details] [diff] [review]
changed GetFullScreen as well (per Neil's suggestion)
Comment 8 Kyle Yuan 2002-12-11 04:53:38 PST
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 2002-12-11 04:54:26 PST
=> default owner
Comment 10 neil@parkwaycc.co.uk 2003-12-05 16:31:47 PST
*** Bug 227553 has been marked as a duplicate of this bug. ***
Comment 11 neil@parkwaycc.co.uk 2003-12-05 16:34:49 PST
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 2007-05-15 18:17:51 PDT
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 ;) )
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2007-05-22 15:21:39 PDT
Comment on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst
Comment 14 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-05-22 15:36:39 PDT
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, jst@mozilla.com) 2007-05-22 17:10:33 PDT
Comment on attachment 264942 [details] [diff] [review]
New patch

Looks good. r+sr=jst
Comment 16 Michael Ventnor 2007-05-22 17:11:57 PDT
(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 2007-05-25 00:01:26 PDT
Created attachment 266033 [details] [diff] [review]
Mochitest
Comment 18 Michael Ventnor 2007-06-04 16:05:31 PDT
Now that the tree has reopened, can someone with CVS rights please check-in this patch?
Comment 19 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-06-04 16:16:42 PDT
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 2007-06-04 16:33:58 PDT
Comment on attachment 266033 [details] [diff] [review]
Mochitest

Thanks!
Comment 21 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-06-04 17:39:29 PDT
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
Comment 22 Michael Ventnor 2007-06-04 17:42:39 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.