539601 - window.sizeMode incorrect when using full screen mode

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This is similar to bug 535215, but not quite the same, so I filed a new one, feel free to dupe it.

STR:
1. Open Minefield - windowState == STATE_NORMAL
2. Enter full screen mode - windowState == STATE_NORMAL (should be STATE_FULLSCREEN)
3. Exit full screen mode - windowState == STATE_MAXIMIZED (should be STATE_NORMAL)

To get the window back into normal mode at the end, I need to resize the window.

You can use the extension from bug 535215 to check these values, in combination with a dump somewhere (I'm working on bug 539597, so I was just watching changes to sessionstore state & dumping some extra stuff to stdout)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

An idea...

This both ensures that we dispatch the size mode event when we enter/leave full screen, but also ensures we will use nsSizeMode_Fullscreen properly (I think).

Comment 2

[Markus Stange [:mstange]]

Is this bug only present on OS X?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Is this bug only present on OS X?

Not sure. I've only had a chance to check on OSX. I'll fire up some VMs later today and check.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Doesn't look like it's a problem on other platforms. A quick test on Linux looks like it works, didn't have a chance to rebuild on Windows.

And looking at the code, nsWindow::MakeFullScreen in gtk2 & windows both set mSizeMode. nsCocoaWindow::MakeFullScreen does not. It looks like mSizeMode is never set except by DispatchSizeModeEvent and having it handled in some circular way (nsWebShellWindow::HandleEvent sets mSizeMode so I assume there's some sort of inheritance chain there, which then calls SetSizeMode).

Comment 6

[Markus Stange [:mstange]]

So actually this is just bug 484833 - we haven't implemented sizemode=fullscreen on Mac yet.
Looking at the patch in bug 510546, you probably need to add the equivalents of the first two hunks, too.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #6)
> Looking at the patch in bug 510546, you probably need to add the equivalents of
> the first two hunks, too.

It would appear so. The 2nd one for sure. The first hunk actually got taken out in bug 527615, so I'm not completely sure about that one. Though it looks like a bunch of irregular things went into the gtk2 widget to support fennec & starting in fullscreen mode, so perhaps it's still needed. The test (test_fullscreen.xul) passes without it, and the window is fullscreen (though it also works without this patch). That test doesn't actually test widget stuff so do we need some tests here or just trust it works?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2 (WIP)

Updated to handle SetSizeMode(). Still not sure about what to do in Show()

Comment 9

[Markus Stange [:mstange]]

Doug, can you help out?

Comment 10

[Doug Turner (:dougt)]

GTK remembered if the top level windows was maximized or not.  On other platforms, it might be different.  For example, on windows, we had to call MakeFullScreen:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1114

Does OSX remember top level window state?  If it doesn't, you may need to also make a similar call.

Comment 11

[Markus Stange [:mstange]]

(In reply to comment #10)
> Does OSX remember top level window state?  If it doesn't, you may need to also
> make a similar call.

No, it doesn't. It relies on a resize following after going into / out of full screen mode. (The size is what determines whether a window is zoomed.)

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

I cleaned this up & pulled out changes from bug 639705 -- it seems to be working correctly on its own.

The side effect here is that without bug 714911, we're going to reopen in fullscreen in some cases. We're already doing that on Windows/Linux though so this will just make us more consistent...

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 585620 [details] [diff] [review]
Patch v0.3

>diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm
>--- a/widget/src/cocoa/nsCocoaWindow.mm
>+++ b/widget/src/cocoa/nsCocoaWindow.mm
>@@ -1085,16 +1085,22 @@ NS_METHOD nsCocoaWindow::SetSizeMode(PRI
>       [mWindow miniaturize:nil];
>   }
>   else if (aMode == nsSizeMode_Maximized) {
>     if ([mWindow isMiniaturized])
>       [mWindow deminiaturize:nil];
>     if (![mWindow isZoomed])
>       [mWindow zoom:nil];
>   }
>+  else if (aMode == nsSizeMode_Fullscreen) {
>+    // We will reach here on a call to MakeFullScreen(true), so we need to
>+    // check that we're not already in full screen mode before calling.

The other reason is that MakeFullScreen would assert. But I'm not sure we need that comment here at all; all the other branches above also check that we don't try to enter a state that we're already in.

>+    if (!mFullScreen)
>+      MakeFullScreen(true);
>+  }
> 
>   return NS_OK;
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> }

What about exiting full screen mode through SetSizeMode? Do other platforms handle that case?

> NS_METHOD nsCocoaWindow::MakeFullScreen(bool aFullScreen)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> 
>   NS_ASSERTION(mFullScreen != aFullScreen, "Unnecessary MakeFullScreen call");
> 
>   NSDisableScreenUpdates();
>+
>   // The order here matters. When we exit full screen mode, we need to show the
>   // Dock first, otherwise the newly-created window won't have its minimize
>   // button enabled. See bug 526282.
>   nsCocoaUtils::HideOSChromeOnScreen(aFullScreen, [mWindow screen]);
>+
>+  // Set mFullScreen here otherwise calls into nsBaseWidget::SetFullScreen()
>+  // get confused about current state. We end up with too many size mode events.
>+  mFullScreen = aFullScreen;

This seems a little fragile. I've just uploaded a patch to bug 715867 which avoids duplicate size mode events in general; with that, we can keep the line that sets mFullScreen at its natural place and send the event afterwards. I'll upload a patch that does that.

The rest looks good.

Comment 14

[Markus Stange [:mstange]]

Attachment: patch on top of bug 715867

Comment 15

[Markus Stange [:mstange]]

By the way, I've tested sizemode events with this in the error console:

(function () { var aConsoleService = Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService); var win = top.opener; win.addEventListener("sizemodechange", function (e) { aConsoleService.logStringMessage("sizemode changed to " + win.windowState); }, false) })()

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Markus Stange from comment #13)
> Comment on attachment 585620 [details] [diff] [review]
> Patch v0.3
> >+  else if (aMode == nsSizeMode_Fullscreen) {
> >+    // We will reach here on a call to MakeFullScreen(true), so we need to
> >+    // check that we're not already in full screen mode before calling.
> 
> The other reason is that MakeFullScreen would assert. But I'm not sure we
> need that comment here at all; all the other branches above also check that
> we don't try to enter a state that we're already in.

Fair enough. I'll take that out. And I'm planning on getting rid of the assert for Lion full screen (though maybe that won't be necessary without duplicate sizemode events...)

> >+    if (!mFullScreen)
> >+      MakeFullScreen(true);
> >+  }
> > 
> >   return NS_OK;
> > 
> >   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> > }
> 
> What about exiting full screen mode through SetSizeMode? Do other platforms
> handle that case?

Looking at Linux, it looks like we don't really allow exiting full screen mode. I played a bit, we can call window.minimize() and it "works", but window.maximize() & window.restore() don't have any effect. OS X won't even minimize, So it seems like we're ok there (fair given platform conventions I'd say).

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4

Markus, I know you're busy but you've been responsive :) If you can't do the review, then I'll poke Josh again.

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f033ecdf2aa

Comment 19

[Ed Morley [:emorley]]

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