752294 - Firefox fails over to maximized when trying to open a window in Lion fullscreen mode

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect)

Description

[Johan Larsson]

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)]

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)]

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]]

Tracking for FF14 and FF15 - we'll uplift depending on when a low risk patch is available.

Comment 4

[Alex Keybl [:akeybl]]

(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)]

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)]

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)]

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)]

> 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)]

Attachment: 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)]

Comment on attachment 634562 [details] [diff] [review]
Handle failure to enter fullscreen mode (partial fix)

LGTM

Comment 11

[Steven Michaud [:smichaud] (Retired)]

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 bugmail, email directly)]

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]]

https://hg.mozilla.org/mozilla-central/rev/5a7d3fce7893

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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]

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)]

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]]

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)]

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)]

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.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

This is left over from when I was a Mozilla employee. I'll never get back to it.

Comment 21

[Stephen A Pohl [:spohl]]

We're working on switching to native fullscreen in bug 1631735.