Last Comment Bug 539601 - window.sizeMode incorrect when using full screen mode
: window.sizeMode incorrect when using full screen mode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
Mentors:
: 484833 (view as bug list)
Depends on:
Blocks: 539597 409095 639705 670026
  Show dependency treegraph
 
Reported: 2010-01-13 16:58 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-03-21 09:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (WIP) (1.32 KB, patch)
2010-01-28 13:54 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.2 (WIP) (3.03 KB, patch)
2010-02-06 11:49 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.3 (4.97 KB, patch)
2012-01-03 18:06 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
patch on top of bug 715867 (4.50 KB, patch)
2012-01-27 12:49 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
Patch v0.4 (4.36 KB, patch)
2012-02-03 16:07 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
mstange: review+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-01-13 16:58:47 PST
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 reading much bugmail, email directly) 2010-01-28 13:54:14 PST
Created attachment 424107 [details] [diff] [review]
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] 2010-01-29 02:33:26 PST
Is this bug only present on OS X?
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-01-29 10:33:42 PST
(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 reading much bugmail, email directly) 2010-01-30 02:36:48 PST
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 5 Markus Stange [:mstange] 2010-02-05 07:05:34 PST
*** Bug 484833 has been marked as a duplicate of this bug. ***
Comment 6 Markus Stange [:mstange] 2010-02-05 07:07:42 PST
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 reading much bugmail, email directly) 2010-02-05 11:42:09 PST
(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 reading much bugmail, email directly) 2010-02-06 11:49:31 PST
Created attachment 425660 [details] [diff] [review]
Patch v0.2 (WIP)

Updated to handle SetSizeMode(). Still not sure about what to do in Show()
Comment 9 Markus Stange [:mstange] 2010-02-07 13:01:08 PST
Doug, can you help out?
Comment 10 Doug Turner (:dougt) 2010-02-22 14:53:19 PST
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] 2010-02-23 03:59:06 PST
(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 reading much bugmail, email directly) 2012-01-03 18:06:27 PST
Created attachment 585620 [details] [diff] [review]
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] 2012-01-27 12:48:56 PST
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] 2012-01-27 12:49:57 PST
Created attachment 592222 [details] [diff] [review]
patch on top of bug 715867
Comment 15 Markus Stange [:mstange] 2012-01-27 12:55:38 PST
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 reading much bugmail, email directly) 2012-02-03 16:05:00 PST
(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 reading much bugmail, email directly) 2012-02-03 16:07:54 PST
Created attachment 594348 [details] [diff] [review]
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 reading much bugmail, email directly) 2012-02-08 12:05:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f033ecdf2aa
Comment 19 Ed Morley [:emorley] 2012-02-09 10:29:22 PST
https://hg.mozilla.org/mozilla-central/rev/5f033ecdf2aa

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