Last Comment Bug 752294 - Firefox fails over to maximized when trying to open a window in Lion fullscreen mode
: Firefox fails over to maximized when trying to open a window in Lion fullscre...
Status: NEW
[leave open]
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: 14 Branch
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-06 01:48 PDT by Johan Larsson
Modified: 2012-06-27 10:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
Handle failure to enter fullscreen mode (partial fix) (1.40 KB, patch)
2012-06-19 13:21 PDT, Steven Michaud [:smichaud] (Retired)
bgirard: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Johan Larsson 2012-05-06 01:48:02 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/15.0 Firefox/15.0a1
Build ID: 20120505040218

Steps to reproduce:

I quit Firefox when in Lion Fullscreen mode.


Actual results:

When I restarted Firefox, it opened in windowed mode.


Expected results:

Firefox should start in Fullscreen mode if it was in Fullscreen mode when the user exited (or restarted) Firefox.
Comment 1 Steven Michaud [:smichaud] (Retired) 2012-06-18 11:58:28 PDT
I can confirm this.  In fact Firefox can't open a window in Lion fullscreen mode.

You see this when you quit Firefox in fullscreen mode and restart it.  You also see it when you close a fullscreen window and then open another window.  In both cases the window is maximized but not fullscreen.

Furthermore Firefox still *thinks* it's fullscreen.  So you need to issue the fullscreen "command" twice to make the window fullscreen again.

This is true only of the Lion fullscreen mode -- not of the fullscreen mode used on OS X 10.6 and 10.5.
Comment 2 Steven Michaud [:smichaud] (Retired) 2012-06-18 11:59:51 PDT
This is a pretty bad bug, but I'm not sure it should block the first appearance of the Lion-specific fullscreen mode (currently scheduled for FF 14).

What do you think, Alex?
Comment 3 Alex Keybl [:akeybl] 2012-06-18 12:06:05 PDT
Tracking for FF14 and FF15 - we'll uplift depending on when a low risk patch is available.
Comment 4 Alex Keybl [:akeybl] 2012-06-18 12:07:25 PDT
(if a low risk fix isn't found, this wouldn't block FF14's release, or cause the new feature to be backed out)
Comment 5 Steven Michaud [:smichaud] (Retired) 2012-06-18 16:09:30 PDT
I've figured out the underlying cause of this bug:  When opening a new fullscreen window, cross-platform code (nsGlobalWindow::SetFullScreen()) tries to make the window fullscreen before it's been made visible (before nsCocoaWindow::Show() has been called on it).

Fixing this isn't going to be easy, or low risk.

There's a hacky workaround (calling the undocumented [mWindow setVisible:] in nsCocoaWindow::MakeFullScreen() if mWindow isn't yet visible).  But this triggers annoying glitches in the animated transition to fullscreen mode.

So I'm afraid FF 14 is going to have to go out with this bug still in it.
Comment 6 Steven Michaud [:smichaud] (Retired) 2012-06-18 16:12:27 PDT
Forgot to mention that the following error shows up in the Console when FF fails to open a window in fullscreen mode:

*** Assertion failure in -[_NSWindowFullScreenTransition makeAndSetupOverlayWindow], /SourceCache/AppKit/AppKit-1138.32/AppKit.subproj/NSWindowFullScreenTransition.m:671

This is what happens when you try to use [NSWindow toggleFullScreen:] to put an invisible window in fullscreen mode.
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-06-18 16:29:43 PDT
Another thing:

There's no way to tell that [mWindow toggleFullScreen:] has failed (the OS never calls [WindowDelegate windowDidFailToEnterFullScreen:]).  So we don't have any way to reset mFullScreen appropriately when the failure happens.
Comment 8 Steven Michaud [:smichaud] (Retired) 2012-06-18 16:52:32 PDT
> So we don't have any way to reset mFullScreen appropriately when the failure
> happens.

Actually I suppose we could just fail in nsCocoaWindow::MakeFullScreen() when mWindow is invisible.  That would fix part of this bug.  I'll submit a patch tomorrow.
Comment 9 Steven Michaud [:smichaud] (Retired) 2012-06-19 13:21:52 PDT
Created attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

This patch follows my suggestion from comment #8.

It doesn't fix this bug.  But it does get rid of the confusion that would otherwise result when Firefox fails to enter fullscreen mode:  With this patch, you only need one "enter fullscreen mode" command to recover from the bug.

The patch is very simple and should be low-risk.
Comment 10 Benoit Girard (:BenWa) 2012-06-19 20:53:56 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

LGTM
Comment 11 Steven Michaud [:smichaud] (Retired) 2012-06-20 08:55:12 PDT
Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a7d3fce7893

This is only a partial fix, so I've put [leave open] in the whiteboard.
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-06-20 22:27:11 PDT
I know I'm a bit behind here, but the work I started to do in the last patch in bug 714911 may help too. I was tracking calls to MakeFullScreen that came in too early, and then calling MakeFullScreen when the window was ready.
Comment 13 Ed Morley [:emorley] 2012-06-21 04:07:33 PDT
https://hg.mozilla.org/mozilla-central/rev/5a7d3fce7893
Comment 14 Steven Michaud [:smichaud] (Retired) 2012-06-21 09:34:30 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MakeFullScreen called prematurely from cross-platform code
User impact if declined: Users will have to click twice to re-enter fullscreen mode
Testing completed (on m-c, etc.): By myself and a little on m-c
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: none

As noted above (in comment #9) this patch doesn't fix the bug, but it does mitigate it.  With this patch, Firefox will no longer be confused about what state the window is in.

It's not the end of the world if the window state confusion isn't fixed in Firefox 14.  But I think it makes fullscreen mode look flaky -- even more so than the original bug.  So I think we should try to get this into FF 14.

The patch hasn't yet baked on mozilla-central.  But it'll get much more exposure to user testing on aurora and beta.  So if there are going to be problems, we'll be more likely to find them out before the FF 14 release.
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:29:07 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

Approving for Aurora then, let's get more user coverage here and then see if this makes sense to take on Beta.
Comment 16 Steven Michaud [:smichaud] (Retired) 2012-06-22 16:58:11 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

Landed on aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/6d28ccd8dde5

Because this bug isn't yet fully fixed, I'm not changing the aurora status.
Comment 17 Alex Keybl [:akeybl] 2012-06-24 12:15:22 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

[Triage Comment]
Let's also land for our next Beta, to make sure it makes it in.
Comment 18 Steven Michaud [:smichaud] (Retired) 2012-06-25 08:49:07 PDT
Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

Landed on mozilla-beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/324e72553ab4
Comment 19 Steven Michaud [:smichaud] (Retired) 2012-06-27 10:27:58 PDT
Lukas:  The original bug hasn't (yet) been fixed.  My patch from comment #9 (attachment 634562 [details] [diff] [review]) just ameliorates its effects.

That's why I've been holding off setting status-firefox14 and status-firefox15 to "fixed".  Don't know if that messes up your tracking system, though.

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