The "minimize/maximize/close" buttons are displayed over the tab after "Forget!" the data from Full Screen

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: cbadau, Assigned: Gijs)

Tracking

34 Branch
Firefox 36
x86
macOS
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Posted image issue.png
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.
Assignee

Comment 1

5 years ago
(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?
Flags: needinfo?(camelia.badau)
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.
Flags: needinfo?(camelia.badau)
Assignee

Comment 3

5 years ago
(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.
Flags: needinfo?(camelia.badau)
Assignee

Comment 4

5 years ago
(if so, this is related to bug 1069658)
I tested on Mac OS X 10.9.5 and I was able to reproduce.
Assignee

Updated

5 years ago
Flags: needinfo?(gijskruitbosch+bugs)
I reproduced the bug on Mac OSX 10.8.5 and Mac OSX 10.9.5.
Flags: needinfo?(camelia.badau)
Assignee

Updated

5 years ago
Assignee

Updated

5 years ago
See Also: → 1069658
Assignee

Comment 7

5 years ago
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?)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 8

5 years ago
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.
Assignee

Comment 9

5 years ago
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).
Assignee

Comment 10

5 years ago
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...
Iteration: --- → 36.1
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Assignee

Comment 11

5 years ago
Err, actually taking per comment #10
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Assignee

Comment 12

5 years ago
In the meantime, Markus, is there a sensible way to make this approach work for restoring the initial window?
Attachment #8508414 - Flags: feedback?(mstange)
Added to IT 36.1
Flags: needinfo?(mmucci)
Assignee

Comment 14

5 years ago
(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...
Assignee

Comment 15

5 years ago
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.
Attachment #8508429 - Flags: review?(dolske)
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.
Attachment #8508414 - Flags: feedback?(mstange) → feedback+
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...
Attachment #8508429 - Flags: review?(dolske) → review+
Assignee

Comment 18

5 years ago
(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?
Flags: needinfo?(mstange)
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.
Attachment #8508429 - Flags: approval-mozilla-beta+
Attachment #8508429 - Flags: approval-mozilla-aurora+
Assignee

Comment 20

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/21d6238c711f
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 21

5 years ago
(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?
Assignee

Updated

5 years ago
Attachment #8508429 - Attachment is obsolete: true
Assignee

Comment 22

5 years ago
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)
Attachment #8508414 - Flags: review?(mstange)
Attachment #8508414 - Flags: review?(bzbarsky)
Assignee

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 23

5 years ago
This fixes the issue identified in comment #21
Attachment #8512335 - Flags: review?(dolske)
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
Attachment #8508414 - Flags: review?(bzbarsky) → review+
Assignee

Comment 26

5 years ago
d'oh. Now with ifdef.
Attachment #8512601 - Flags: review?(dolske)
Assignee

Updated

5 years ago
Attachment #8512335 - Attachment is obsolete: true
Attachment #8512335 - Flags: review?(dolske)
Iteration: 36.1 → 36.2
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.
Attachment #8512601 - Flags: review?(dolske) → review+
Assignee

Comment 28

5 years ago
Landed on alder... waiting on fx-team to reopen to land there...

remote:   https://hg.mozilla.org/projects/alder/rev/914bed88291e
Whiteboard: [fixed-alder]
Assignee

Comment 29

5 years ago
Comment on attachment 8512601 [details] [diff] [review]
deny fullscreen from the forget button,

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

[Triage Comment]

Needed for 33.1.
Attachment #8512601 - Flags: approval-mozilla-beta+
Attachment #8512601 - Flags: approval-mozilla-aurora+
QA Contact: bogdan.maris
https://hg.mozilla.org/mozilla-central/rev/7c582981a0b0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Verified that this is fixed on Mac OS X 10.8.5 using Firefox 33.1, latest Aurora and latest Nightly. Marking flags accordingly.
Assignee

Updated

5 years ago
Blocks: 1091927
Assignee

Comment 35

5 years ago
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.
Attachment #8508414 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8508414 - Flags: review?(mstange)
Also verified on Mac OS X 10.9.5 using Firefox 34 beta 6.
You need to log in before you can comment on or make changes to this bug.