Steps to reproduce: 1. Open Aurora. 2. Move the Panic/Forget button to Toolbar. 3. Click the 'Forget' icon. 4. Click on the "Forget!" option. Actual result: The "minimize/maximize/close" buttons are displayed over the tab after forgetting the data from Full Screen. Please see attached screenshot ("issue.png"). Expected results: The "minimize/maximize/close" buttons are correctly displayed, without overlapping the tab. Notes: 1. This is Mac OSX only. 2. Also reproducible in latest Nightly 35.0a1.

(In reply to Camelia Badau, QA [:cbadau] from comment #0) > Created attachment 8501017 [details] > issue.png > > Steps to reproduce: > 1. Open Aurora. > 2. Move the Panic/Forget button to Toolbar. > 3. Click the 'Forget' icon. So per the summary, do you need to be in full screen to reproduce this issue?

Yes. Sorry, my mistake, I forgot to mention that. Updated steps to reproduce: 1. Open Aurora. 2. Move the Panic/Forget button to Toolbar. 3. Enter Fullscreen. 4. Click the 'Forget' icon. 5. Click on the "Forget!" option.

(In reply to Camelia Badau, QA [:cbadau] from comment #2) > Yes. Sorry, my mistake, I forgot to mention that. > > Updated steps to reproduce: > 1. Open Aurora. > 2. Move the Panic/Forget button to Toolbar. > 3. Enter Fullscreen. > 4. Click the 'Forget' icon. > 5. Click on the "Forget!" option. Are you using yosemite (10.10) ? I can't reproduce on 10.9.

(if so, this is related to bug 1069658)

I tested on Mac OS X 10.9.5 and I was able to reproduce.

I reproduced the bug on Mac OSX 10.8.5 and Mac OSX 10.9.5.

So we open this new window, and then there's a fullscreen animation on OS X, and in the meantime, we're closing all these other windows. OS X then (for whatever reasons of its own, I guess?) kicks the newly created window back to normal mode, and core doesn't give us either a fullscreen or a sizemodechange event - but the sizemode attribute changes; the inFullscreen attribute doesn't, however... investigating further. (because of the backporting, we may just need to work around this on 33 if we think it's important to fix; on Mac we can probably open the new window after closing all the others? Maybe?)

So the fun bit is that browser-fullScreen, when init'ing, listens to the browser.fullScreen property. That gets set from here: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#1170 because sizemode is persisted. Whatever trickle-down semantics are meant to tell Cocoa about this, that never happens (in time), and so our cocoa widget code thinks the window is still not in fullscreen, after which it going back to normal mode means it doesn't even fire a sizemodechange event because it thinks the window is already in normal mode to begin with. Note that this is racy (sometimes it works) and I'm not sure how to make it less so.

It looks like MakeFullScreen bails out ( http://hg.mozilla.org/mozilla-central/annotate/50b689feab5f/widget/cocoa/nsCocoaWindow.mm#l1288 ) and in the normal case of new windows being shown, OS X puts them in fullscreen under this stack: * frame #0: 0x000000010253b9a0 XUL`-[WindowDelegate windowWillEnterFullScreen:](self=0x0000000110df7c20, _cmd=0x00007fff8ee336d0, notification=0x00000001279aa340) at nsCocoaWindow.mm:2259 frame #1: 0x00007fff89ac6e0c CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12 frame #2: 0x00007fff899ba82d CoreFoundation`_CFXNotificationPost + 2893 frame #3: 0x00007fff90928e4a Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 68 frame #4: 0x00007fff8e6cd09d AppKit`-[_NSFullScreenTransition enterFullScreenTransitionWithOptions:animated:activatingIt:] + 994 frame #5: 0x00007fff8e6cc48e AppKit`-[NSWindow _enterFullScreenMode:animating:activating:] + 291 frame #6: 0x00007fff8e5324de AppKit`-[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 3548 frame #7: 0x00007fff8eb59877 AppKit`__71-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:]_block_invoke2684 + 92 frame #8: 0x00007fff8e50b1e8 AppKit`NSPerformWithScreenUpdatesDisabled + 65 frame #9: 0x00007fff8e531518 AppKit`-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 970 frame #10: 0x00007fff8e5310e0 AppKit`-[NSWindow orderWindow:relativeTo:] + 162 frame #11: 0x00007fff8e522686 AppKit`-[NSWindow makeKeyAndOrderFront:] + 51 frame #12: 0x000000010253811a XUL`nsCocoaWindow::Show(this=0x00000001221c7500, bState=<unavailable>) + 1482 at nsCocoaWindow.mm:809 frame #13: 0x0000000102e3a8b0 XUL`nsXULWindow::SetVisibility(this=0x000000010b6c6250, aVisibility=<unavailable>) + 128 at nsXULWindow.cpp:811 frame #14: 0x0000000102e37494 XUL`nsXULWindow::OnChromeLoaded(this=0x000000010b6c6250) + 484 at nsXULWindow.cpp:1015 frame #15: 0x0000000102e3725c XUL`nsWebShellWindow::OnStateChange(this=0x000000010b6c6250, aProgress=<unavailable>, aRequest=<unavailable>, aStateFlags=<unavailable>, aStatus=<unavailable>) + 652 at nsWebShellWindow.cpp:558 frame #16: 0x0000000102e378bd XUL`non-virtual thunk to nsWebShellWindow::OnStateChange(this=<unavailable>, aProgress=<unavailable>, aRequest=<unavailable>, aStateFlags=<unavailable>, aStatus=<unavailable>) + 13 at nsWebShellWindow.cpp:562 frame #17: 0x00000001016fecf2 XUL`nsDocLoader::DoFireOnStateChange(this=0x0000000113514000, aProgress=0x0000000113514028, aRequest=0x0000000123eed950, aStateFlags=0x00007fff5fbfcfdc, aStatus=2153578529) + 306 at nsDocLoader.cpp:1269 frame #18: 0x00000001016fe8db XUL`nsDocLoader::doStopDocumentLoad(this=<unavailable>, request=0x0000000123eed950, aStatus=2153578529) + 315 at nsDocLoader.cpp:861 frame #19: 0x00000001016fdc20 XUL`nsDocLoader::DocLoaderIsEmpty(this=0x0000000113514000, aFlushLayout=<unavailable>) + 608 at nsDocLoader.cpp:740 <snip> I'll look at if the JS-side workaround is easy enough that we should consider uplifting for 33; otherwise I think we should try to see if we can fix this properly in cocoa's fullscreen handling - but I doubt I'd be happy to take that on 33 (perhaps not even on 34).

Dolske and I talked this over last week and I think we agreed we should try to fix/workaround this, so taking. However, the hack I was planning to use doesn't work because when the window is opened, the sizemode is set to fullscreen, and so no event fires. The sizemodechange event doesn't fire here because the window gets set to normal mode, and the window's internal structure initializes as such. So then I tried to make nsGlobalWindow actually care about the result of MakeFullscreen. That works beautifully, but it breaks quitting in fullscreen and then reopening in fullscreen. My next idea is to force nsCocoaWindow to actually fire a sizemodechange...

Err, actually taking per comment #10

In the meantime, Markus, is there a sensible way to make this approach work for restoring the initial window?

Added to IT 36.1

(In reply to :Gijs Kruitbosch from comment #10) > My next idea is to force nsCocoaWindow to actually fire a sizemodechange... This doesn't work because at that point the fullscreen property hasn't changed yet...

This gross hack works. It's not pretty. But at least it works. I still think we should fix platform, but this is also upliftable, which probably can't be said for the platform patch.

Comment on attachment 8508414 [details] [diff] [review] don't allow setting fullscreen when widget says no, f?mstange Seems reasonable enough? I don't think it's too much of a hack - if stuff fails, it fails.

Comment on attachment 8508429 [details] [diff] [review] deny fullscreen from the forget button, Review of attachment 8508429 [details] [diff] [review]: ----------------------------------------------------------------- To be clear, I understand the intent is to land this as a short-term, front-end bandaid in 33.1, and only use the widget fix for future releases? I suppose I could update my patch in bug 1088137 to use browser-delayed-startup-finished too...

(In reply to Justin Dolske [:Dolske] from comment #17) > Comment on attachment 8508429 [details] [diff] [review] > deny fullscreen from the forget button, > > Review of attachment 8508429 [details] [diff] [review]: > ----------------------------------------------------------------- > > To be clear, I understand the intent is to land this as a short-term, > front-end bandaid in 33.1, and only use the widget fix for future releases? > Yes, ideally... see below. > I suppose I could update my patch in bug 1088137 to use > browser-delayed-startup-finished too... That would be great. (In reply to Markus Stange [:mstange] from comment #16) > Comment on attachment 8508414 [details] [diff] [review] > don't allow setting fullscreen when widget says no, f?mstange > > Seems reasonable enough? I don't think it's too much of a hack - if stuff > fails, it fails. Hmm, but as noted, this breaks quitting the browser from fullscreen and then reopening it (doesn't open in fullscreen). I don't know how to fix that. Do you have ideas?

Comment on attachment 8508429 [details] [diff] [review] deny fullscreen from the forget button, [Triage Comment] Please land this on alder today, and nightly/aurora/beta asap. (Will need to update patch with bug 1088137's changes) And please file a new bug for the longer-term platform changes.

(In reply to :Gijs Kruitbosch from comment #20) > https://hg.mozilla.org/integration/fx-team/rev/21d6238c711f Backed out because of course, 15 minutes after re-testing and landing this I tried fixing another bug and realized that *after* restarting the profile, the titlebar state still gets messed up, even if it doesn't immediately... remote: https://hg.mozilla.org/integration/fx-team/rev/37d5c621ad52 (In reply to :Gijs Kruitbosch from comment #18) > (In reply to Markus Stange [:mstange] from comment #16) > > Comment on attachment 8508414 [details] [diff] [review] > > don't allow setting fullscreen when widget says no, f?mstange > > > > Seems reasonable enough? I don't think it's too much of a hack - if stuff > > fails, it fails. > > Hmm, but as noted, this breaks quitting the browser from fullscreen and then > reopening it (doesn't open in fullscreen). I don't know how to fix that. Do > you have ideas? Oddly, I just tried and can't reproduce the quitting+restarting with fullscreen working on OS X at all, even without the patch. So maybe this is fine?

Comment on attachment 8508414 [details] [diff] [review] don't allow setting fullscreen when widget says no, f?mstange So does this just work? Markus, can you confirm restoring to fullscreen already doesn't work? (Boris: I don't believe there are any other widget implementations that return non-NS_OK in the real world... yet)

This fixes the issue identified in comment #21

Comment on attachment 8512335 [details] [diff] [review] deny fullscreen from the forget button, Review of attachment 8512335 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, will test it out in a bit. ::: browser/base/content/sanitize.js @@ +550,5 @@ > if (subject != newWindow) > return; > > Services.obs.removeObserver(onWindowOpened, "browser-delayed-startup-finished"); > + newWindow.removeEventListener("fullscreen", onFullScreen); Hmm, I think you'll be wanting an #ifdef here, since onFullScreen is undefined?

Comment on attachment 8508414 [details] [diff] [review] don't allow setting fullscreen when widget says no, f?mstange r=me

d'oh. Now with ifdef.

Comment on attachment 8512601 [details] [diff] [review] deny fullscreen from the forget button, Review of attachment 8512601 [details] [diff] [review]: ----------------------------------------------------------------- Hmm. I can't reproduce this anymore on 10.10, but I can on 10.9. Weird. Did check that 10.10 still works fine with this patch, and that it fixes the problem on 10.9.

Landed on alder... waiting on fx-team to reopen to land there... remote: https://hg.mozilla.org/projects/alder/rev/914bed88291e

Comment on attachment 8512601 [details] [diff] [review] deny fullscreen from the forget button, https://hg.mozilla.org/integration/fx-team/rev/7c582981a0b0

Comment on attachment 8512601 [details] [diff] [review] deny fullscreen from the forget button, [Triage Comment] Needed for 33.1.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/1490073aeac6 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/776f967418e1

Verified that this is fixed on Mac OS X 10.8.5 using Firefox 33.1, latest Aurora and latest Nightly. Marking flags accordingly.

Comment on attachment 8508414 [details] [diff] [review] don't allow setting fullscreen when widget says no, f?mstange I'm fairly sure this will suffer from the same problem my initial frontend patch had, namely a broken sizemode attribute. Let's take further iteration on the core stuff to a separate bug. Filed bug 1091927.

Also verified on Mac OS X 10.9.5 using Firefox 34 beta 6.