I didn't notice this when I was working on the feature, but it's there... When opening a new window from a fullscreen window, there's a black bar where the menubar would have been, along with a couple grey spots in the corner (window corners?). Sometimes you can notice the window's titlebar fading out. Initially I thought it had to do with the backout of bug 714911, but it doesn't appear so. It could have to do with the initial window size though, since the previous window size gets used when opening windows, it seems to fill the area where the menubar would be.

Not surprisingly, the same thing happens when a tab is detached.

Tracking for FF14 since this is a fairly visible bug for a new feature, but we probably wouldn't need to back out bug 639705 if this were left unfixed. The un-FS button still works even when this issue is reproduced.

So I think I tracked this down, but I don't really understand why. It sounds like it could be similar to what's described in CreateNativeWindow: https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.mm#400 (the comment goes back to CVS & bug 325336)

Paul, I hope to get to this later this week.

Paul, have you looked at what happens when you open a new window from a fullscreen window in FF 12 (on OS X 10.7, 10.6.8 or 10.5.8) and then press Command-Shift-F to make *that* window fullscreen? In all cases you get a white, outlined box the same size and shape as your "black bar". I wonder if that's related. (I've started to do tests for this bug, and should be able to give you feedback on your patch tomorrow.)

(In reply to Steven Michaud from comment #6) > Paul, have you looked at what happens when you open a new window from a > fullscreen window in FF 12 (on OS X 10.7, 10.6.8 or 10.5.8) and then press > Command-Shift-F to make *that* window fullscreen? In all cases you get a > white, outlined box the same size and shape as your "black bar". > > I wonder if that's related. > > (I've started to do tests for this bug, and should be able to give you > feedback on your patch tomorrow.) Interesting... Definitely seems like it could be related. There's the same issue of the window being opened with a size that fills the screen. Also, looking in nsCocoaUtils, I just saw MenuBarScreenHeight and tried that, but that fails hard. It's returning 1200 (height of screen) when there's no menubar (which makes sense, it's looking for the first object when there's no menubar. And then turning lion FS off, it still returns screen height (menubar is gone by then I think) so we get all sorts of broken. So we can't use that...

For this and bug 742178 I'm going to do a test build with logging on all the fullscreen activity. I'll play with this and see what I turn up. It's time I learned this code anyway :-)

[Triage Comment] Re-assigning this to Steven as per comment 8, since it appears to be something he's taking on.

(In reply to Steven Michaud from comment #8) > For this and bug 742178 I'm going to do a test build with logging on all the > fullscreen activity. I'll play with this and see what I turn up. > > It's time I learned this code anyway :-) This is an uncommon enough use case that I wouldn't block on it. But have we made any progress in resolving? We would consider uplifting a fix, if found.

I've been working on this (and bug 742178 and bug 757618). But I've had many distractions, and so haven't been able to finish my work. Bug 757618 is the most serious of the three. And fortunately I'm also closest to a fix for that bug.

Not critical for the FS feature, but we'll evaluate feedback and prioritize for FF15 if necessary.

Is there any progress on this bug. 16.0.2 is still a problem and also the beta. Very annoying bug if you ask me. Thanks in advance.

The impact of this bug is aggravated by per-window private browsing: - Users are more likely to open new (private) windows - PWPBM now uses drawintitlebar, which will cause the black rect to appear over the tab strip, hiding it, instead of just pushing the whole window incl. tab strip down. The tab strip isn't readable for Private windows! I also dug into why this occurs in the first place. 1. nsCocoaWindow::mFullScreen (or ::mInFullScreenTransition) is not always correct. Hence the SizeMode gets not always updated to nsSizeMode_Fullscreen. E.g. when opening a new window from another window, nsCocoaWindow::MakeFullScreen will bail early because the [window isVisible] === false. However AppKit itself will put this thing in fullscreen, leaving mFullScreen and sizemode in an incorrect state until finally a windowDidEnterFullScreen notification is received. E.g.2. Seems there is a windowDidResize notification before windowDidEnterFullScreen, which causes an UpdateBounds(). 2. nsCocoaWindow::UpdateBounds() will update the bounds to the full screen size aka. [screen visibleFrame]. (UpdateBounds() does not seem to happen when the window is restored from full screen mode, btw). 3. nsXULWindow will subsequently persist these insane bounds from 2., which will not only later cause the black rect, but kills the previously stored dimensions as well. The dims are persisted in the first place because SizeMode is incorrectly _Normal (see 1.) at the point of a nsXULWindow::SavePersistentAttributes() call. STR (OK): - Open Firefox. Resize the resulting window to only covering part of the screen - Open a new window. -> Correct. New window will have the same dims, only covering part of the screen. STR (trigger bug): - Open Firefox. Resize the resulting window to only covering part of the screen - Fullscreen the window. - Deactivate Fullscreen again -> Correct. Window restored to covering only part of the screen. - Open a new window -> Incorrect. New window covering the whole screen, basically having been maximized. Should have dims of the first window instead. 4. When opening a new window, it will be opened covering the whole screen frame (3.). The AppKit native fullscreen implementation will then mess with the dimensions of this already full-screen (but not tracked as such, see 1.) window, resulting in the black rect. zpao's WIP patch 0.1 does work around this by forcefully reducing the insane bounds to somewhat sane ones, however it does not address any of the underlying causes and the buggy behavior described in 2. 5. The black rect isn't actually always there, due to 3. STR (OK) - Open Firefox - Open a second window and but into full screen - Switch back to regular first window and resize it a bit. - Open a new third window from the second (fullscreen) window -> No black rect. This is because resizing the first window caused another round of nsXULWindow::SavePersistentAttributes(), this time with the sane dims of the regular window, overriding the previous insane dims of the second window. You can actually see the new (third) window starting out with the sane dims from the first window in the fullscreen animation. As I have no experience with Obj-C, let alone Cocoa, and the massive and strongly coupled nsXULWindow, nsCocoaWindow and friends make my head hurt when staring to long at them, I cannot really provide a patch here. However, the solution IMO would be to make sure UpdateBounds never actually updates mBounds when there is reason to believe that the window is in native full screen mode (mFullScreen, mInFullScreenTransition, [screen visibleFrame] == [window frame], etc.), or even better fix the code to set mFullScreen early and correctly always.

Just in case anybody is wondering what the black rect looks like in general, and in PWPBM in particular with the tab strip overlap, here is a cropped version of the black rect, and how stuff is supposed to look like below that.

This is bad, I agree, but we won't block shipping per-window PB because of this.

Hey Steven, or anybody else from the OSX Widget folks, I polished zpao's patch a bit. It does not fix the underlying causes, but makes sure the black-rect does not occur. Since with per-window private browsing this is no longer merely an UX glitch any longer, but actually a functional issue, I figure: Better just fix the worst symptom for now and make stuff functional again than nothing at all. The mMenubarHeight stuff is admittedly ugly, however as far as google tells me [[NSApp mainMenu] menuBarHeight] is the only supported way to get that height, but it will return 0 in full-screen mode, hence the need to cache a valid value once it appears for later use. Any additional thoughts?

I'll try to get to this review next week ... but I won't cry if someone else beats me to it. I agree that even a hacky (and possibly temporary) fix for this bug is now more acceptable, since fixing it has now become more urgent.

(In reply to Steven Michaud from comment #19) > I'll try to get to this review next week ... but I won't cry if someone else > beats me to it. > > I agree that even a hacky (and possibly temporary) fix for this bug is now > more acceptable, since fixing it has now become more urgent. Hi guys. Appreciate the effort. When would this be available and in which build? It is really annoying.

Turns out, I forgot the check for native fullscreen + screenBounds.y == 0, which had undesirable behaviour for some non-fullscreen windows covering the whole screen. That also made widget/tests/test_bug413277.html fail. Green try: https://tbpl.mozilla.org/?tree=Try&rev=f0ee1871ad37

Comment on attachment 703434 [details] [diff] [review] Partial fix for the black-rect of doom Turns out I'm not going to be able to get to this this week. I hope to by the middle of next week, though.

Steven, do you have time to do the review? Or are you too busy? Should I ask somebody else?

I'm working on it right now. I should have some sort of response today or tomorrow. (I dug up the logging patch I mentioned in comment #8, and will also play with that.)

I'm going to make one last stab at actually fixing this bug (instead of just working around it). One odd thing I noticed, which seems to indicate a bug in cross-platform (aka Gecko) code: When you create a new window while an existing window is in fullscreen mode, nsIWidget::SetSizeMode() is called on it twice (instead of just once), and both times with the wrong value -- first nsSizeMode_Normal then nsSizeMode_maximized. I'll try to get to the bottom of this. Did any of you notice this in your own tests? Did you try to remedy it?

(Following up comment #25) Actually it sounds like nmaier discussed this in comment #15. I'll still try to get to the bottom of it tomorrow, though.

For the record, here's a trace of the two wrong calls to nsCocoaWidget::SetSizeMode() I mentioned above, plus one correct one (which happened after a pause of several seconds).

I found what seems to be a real fix for this bug (not just a workaround) -- a surprisingly simple one. But it may be risky, as it removes part of Paul O'Shannnessy's Lion-style fullscreen support patch (bug 639705). So I'd like people here to do some testing, and report their results back, before I ask for a review. I've started a tryserver build, which should be available in a few hours. As noted in comment #25 (and in my trace from comment #27), the second of the two "wrong" calls from Gecko code to nsCocoaWindow::SetSizeMode() sets the size mode to nsSizeMode_Maximized. This happens during the transition to fullscreen mode, and seems to confuse Gecko/widget code. One of the symptoms is this bug. Getting rid of that call fixes this bug. I find I can stop that call from happening by sending a nsSizeMode_Fullscreen size mode event to Gecko at the start of the transition to Lion's fullscreen mode (which takes several seconds). Previously we only sent this event at the end of the transition. But that allows Gecko to think, for several seconds, that we're maxmizing the window (not making it fullscreen). And that seems to be the source of the confusion that causes this bug. The first "wrong" call to nsCocoaWindow::SetSizeMode(), which sets it to nsSizeMode_Normal, doesn't seem to cause trouble. And judging by the code comment where the call is made, it's there for a good reason. http://hg.mozilla.org/mozilla-central/annotate/19f630648c80/xpfe/appshell/src/nsXULWindow.cpp#l560

Comment on attachment 706590 [details] [diff] [review] Possible "real" fix Hurray, this patch prevents the underlying issue as stated, namely the window persisting bogus window dimensions. However, it does not help if your existing profile has persisted bogus dimensions before. STR: - Run an unpatch build - Full screen window - Create a new window (black rect) - Quit browser - Run the patched build - Full screen window - Create new window -> Black rect still present

Thanks, Nils, for your tests! I see the same problem you do. But it doesn't happen if I resize the window (in the patched build) before going into full screen mode. That appears to clear out the bogus stored window dimensions.

There appear to be some test failures that I'll need to deal with, either by changing the patch or the tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=19143729&tree=Try&full=1 I'll do that next week.

Here's the tryserver build made from my patch from comment #28: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-21f510e000a2/try-macosx64/firefox-21.0a1.en-US.mac.dmg

I'm still working on this, but each new revision has new test failures (ones that happen consistently through multiple tryserver runs, and which don't happen with other tryserver builds done around the same time). For my own reference, here are the ones that happen with my current patch: https://tbpl.mozilla.org/php/getParsedLog.php?id=19294205&tree=Try&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=19294458&tree=Try&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=19293828&tree=Try&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=19288385&tree=Try&full=1 It's all very puzzling, as most of these have no obvious relation to my patch. I'll keep trying to figure out the problems. But if I can't by the end of this week I'll fall back on one of the workarounds.

Here's a tryserver build made with my patch from comment #33: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-d53ad81966a5/try-macosx64/firefox-21.0a1.en-US.mac.dmg

(Following up comment #33) > I'll keep trying to figure out the problems. But if I can't by the > end of this week I'll fall back on one of the workarounds. I think I'm close to figuring this out, but I'm not quite there yet. So I'll postpone my deadline until Monday of next week.

I give up. Friday's hunch didn't pan out, and I seem as far now as I ever was from figuring out how to truly fix this bug. I still think Gecko gets confused by the delay in entering fullscreen mode, and my previous "real fix" patches do make it behave better. But all of them bumped against what seems to be an insurmountable problem -- if we don't delay sending an nsSizeMode_Fullscreen size mode event to Gecko, Gecko sometimes tries to change out of fullscreen mode before the original transition to fullscreen mode has completed. I thought I had a fix for this, but it turns out I don't. I've kept this patch as simple, as narrowly focused and as shameless as possible. For example it just shifts the window down by 22 pixels. This number is the nominal height of the menu bar. But it's also the nominal height of a window's title bar, and we don't know which of these two is relevant to this bug. At some point I (or someone else) may learn enough to truly fix this bug -- probably by accident, while researching some other, unrelated bug. But until then this patch fixes an annoying problem, without (apparently) causing any other problems. I've started a tryserver build. The results should be available tomorrow morning.

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround This is essentially the same thing that zpao did and I then used in my partial fix... Can you clarify by reviewing my thing or canceling the corresponding review?

> This is essentially the same thing that zpao did and I then used in > my partial fix. True enough. > Can you clarify by reviewing my thing or canceling the corresponding > review? To be honest, I didn't fully like either of the patches as it stood. So I took bits and pieces from each. I prefer that Paul O’Shannessy's patch just uses the '22 pixels' constant. As I said in comment #36, we're not entirely sure where this comes from. It might be the (conventional) height of the menu bar, or it might be the height of the titlebar ... or it might (conceivably) be something else altotegher. I prefer that your patch narrows the scope of the patch's application -- for example it only does anything if the window being checked has mUsesNativeFullScreen set to 'true'. But I also think it should be still more narrowly focused -- that it should furthermore only do anything if aRect.y == 0 and aRect.height == screenBounds.height. Side note: Your patch's mMenuBarHeight and menuBarHeight should be CGFloats, not floats. As far as I know, a CGFloat is a float in 32-bit mode and a double in 64-bit mode.

Here's a tryserver build made from my patch from comment #37: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-4a35fc99d2ed/try-macosx64/firefox-21.0a1.en-US.mac.dmg There were no test failures at all (even spurious ones). Thanks to both Paul O'Shannessy and Nils Maier for your patches! Even though I didn't end up using either of them, your workaround wasn't obvious, and I might not have found it on my own.

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround Review of attachment 709989 [details] [diff] [review]: ----------------------------------------------------------------- reluctant r+

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b00fee0a69b4

The issue is also present in Firefox 18. Will this only be fixed in Firefox 21 ?

We should definitely look at uplifting it to Firefox 20, since per-window private browsing makes it much more visible.

(In reply to Josh Matthews [:jdm] from comment #44) > We should definitely look at uplifting it to Firefox 20, since per-window > private browsing makes it much more visible. Note: This black bar is not only to see in the private Mode Window. Also in normal fullscreen windows. It would be really nice to have it also in FF19, too. This black bar is really annoying. Thanks.

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround Let's try getting this onto the 20 branch first. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 639705 User impact if declined: Annoying visual glitch, more common in private browsing mode. Testing completed (on m-c, etc.): Some testing by myself, plus a few days testing on m-c. Risk to taking this patch (and alternatives if risky): low to moderate String or UUID changes made by this patch: none This patch is very simple and narrowly defined. But it's only a workaround (we don't fully know why the bug happens). So the more testing it gets the better. It will definitely get more testing on aurora and mozilla-central than it will on mozilla-central alone.

Comment on attachment 621809 [details] [diff] [review] Patch v0.1 (WIP) Discussed in comment #38.

Comment on attachment 703434 [details] [diff] [review] Partial fix for the black-rect of doom Discussed in comment #38.

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround In the interest of having the highest quality launch of per-window PB we'll take this low-enough risk uplift.

Comment on attachment 709989 [details] [diff] [review] Simplified, hackish workaround Landed on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/dd1a4d28655c

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0 Verified with Firefox 20 beta 6. Original issue fixed. Navigated through full screen and normal mode - all operations were successful - no issues spotted.

It seems to me that this tricky hack is no longer necessary. I cannot reproduce this bug even if I comment out the related code in the patch. I intend to backout this patch because when I'm working on bug 1105939 (for bug 1160014), this change causes some weird behavior that the fullscreen window is not attched to the topmost, but have a 22px gap. I could workaround on that, but it seems silly to have a workaround because of another workaround which has probably been useless. Steven, could you confirm that, we won't meet this problem again even if we backout this patch? I've pushed a try build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/quanxunzhen@gmail.com-39b57d0c5ca9/try-macosx64/firefox-40.0a1.en-US.mac.dmg

I tested your tryserver build (using the STR from comment #0) on each of OS X 10.7.5, 10.8.5, 10.9.5 and 10.10.3. I didn't see this bug with any of them. Thanks for catching this! Whatever fixes you come up with, do make sure to also test them on each of these OS X versions -- that is if you're going to be using the OS X native fullscreen mode, instead of the Gecko cross-platform one. As you've probably already seen, the native fullscreen mode has lots of hidden (undocumented) gotchas, and has changed from major version (of OS X) to major version.

(In reply to Steven Michaud from comment #53) > I tested your tryserver build (using the STR from comment #0) on each of OS > X 10.7.5, 10.8.5, 10.9.5 and 10.10.3. I didn't see this bug with any of > them. Thanks for catching this! Thanks for testing! > Whatever fixes you come up with, do make sure to also test them on each of > these OS X versions -- that is if you're going to be using the OS X native > fullscreen mode, instead of the Gecko cross-platform one. As you've > probably already seen, the native fullscreen mode has lots of hidden > (undocumented) gotchas, and has changed from major version (of OS X) to > major version. I'm going to make DOM fullscreen use the previous fullscreen method instead of the native fullscreen mode, and unify a different transition across platforms. But since we have not been using that code path in new systems for a while, I guess it is probably also dangerous to switch. Hence test on those versions could be helpful. Do we currently have any infrastructure where I can test on those versions? Or is there any cloud service provides that? Or I have to install all of them myself to test?

> Do we currently have any infrastructure where I can test on those > versions? Or is there any cloud service provides that? Or I have to > install all of them myself to test? Ages ago, QA had a lab (in the Mountain View office of that time) with many different kinds of Macs, where people could go to test things. But (of course) you had to physically be in the lab. I believe the lab disappeared years ago, and that since then Mozilla has had no systemmatic way to deal with this problem. I've grown used to keeping all my old machines, and setting them up with multiple bootable partitions (40 GB should be enough room for one). You can also run (some) older versions of OS X in virtual machines (I use VMWare Fusion). But for something like this (fullscreen mode, especially native fullscreen mode) you really do need hardware. > But since we have not been using that code path in new systems for a > while But we *have* been, on OS X 10.6 (which doesn't have the Lion-and-up fullscreen mode). And something like 20% of our users are still on this version of OS X. So if there were any serious problems, I think we would have heard about it. > the previous fullscreen method I assume this is what I've called the Gecko cross-platform fullscreen mode -- which we still use on OS X 10.6, and also (as far as I know) on Windows and Linux. I don't believe it has ever had any platform-specific features or limitations. So if it works on OS X 10.6, it should also work on other versions of OS X.