Closed Bug 1188322 Opened 5 years ago Closed 5 years ago

The menubar still exists when the document enters fullscreen on a secondary monitor

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox41 --- verified
firefox42 --- verified

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Steps to reproduce:
1. Open a window and move it to a secondary monitor
2. Open https://developer.mozilla.org/samples/domref/fullscreen.html
3. Click the fullscreen button

Expected result:
The window takes the fullscreen in the secondary monitor, menubar and dock should be hidden

Actual result:
The menubar is still there


This could be a regression from bug 1105939 or bug 1172664. Both are on Firefox 41, anyway.
There is a comment in nsCocoaUtils::HideOSChromeOnScreen() says:
>   // Only hide the menu bar if the window is on the same screen.
>   // The menu bar is always on the first screen in the screen list.

This seems no longer be true. We probably should remove the condition which checks whether we are on the first screen. But I'm not quite sure whether it would cause problem in older versions.
I have a patch fixes this behavior. smichaud, could you help testing this on different version of OS X with multiple monitors, and see if it causes any issue?

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c614f64fbcd8
binary: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/quanxunzhen@gmail.com-c614f64fbcd8/try-macosx64/
Flags: needinfo?(smichaud)
I can't reproduce this bug on OS X 10.7.5, so I didn't bother to test there.

I *can* reproduce it on OS X 10.9.5 and 10.10.4.  I did minimal tests with your tryserver build there, and found that your patch fixes the bug and doesn't appear to cause trouble.

Later I'll test on OS X 10.11, but it's taking me a while to upgrade to the latest beta.
Flags: needinfo?(smichaud)
I got the same results on the latest build of OS X 10.11 (15A244d):  I can reproduce the bug in today's m-c nightly, but don't see it in your test build.  I also didn't see any problems (in my minimal testing).
(In reply to Steven Michaud [:smichaud] from comment #3)
> I can't reproduce this bug on OS X 10.7.5, so I didn't bother to test there.

I really wonder the code I removed was for those systems which don't have this bug. Could you test the new try build version there with the old versions? It seems to me if we completely remove that condition, we could simplify the code there a lot.
That code was added in bug 370857 (the initial fullscreen support for OS X). It seems to me that patch just tries to as much as possible avoid hiding things. But I think this bug is somehow more annoying than unnecessary hiding things, because people are likely to use fullscreen to play video in the second screen.
I just tested with your test build on OS X 10.7.5.  I went into and out of "regular" and DOM fullscreen mode, on the primary and secondary monitors.  DOM fullscreen mode worked just fine, but my tests were vitiated by very serious weirdness with the "regular" fullscreen mode on the secondary monitor.  This isn't caused by your patch -- it also happens in today's m-c nightly.

So this is a new, unrelated bug -- presumably a very recent regression.  I'm too tired now to do decent work.  I'll open a bug tomorrow on the issue I found.
It could be considered as a regression from bug 1105939 since before we switch from using native fullscreen mode for DOM fullscreen, we don't have this issue.
Blocks: 1105939
Attached patch patchSplinter Review
mstange, it seems this part of code was initially implemented by you in bug 370857, I assume you know this code better and could notice something important I didn't notice.
Assignee: nobody → quanxunzhen
Attachment #8644709 - Flags: review?(mstange)
(Following up comment #7)

No problems testing on OS X 10.6.8 (in either your test build or today's m-c nightly).  But that's not too surprising, as 10.6.8 doesn't have the Lion-style "native" fullscreen mode, and both our "regular" and your DOM fullscreen modes are implemented entirely in Gecko.
Comment on attachment 8644709 [details] [diff] [review]
patch

Review of attachment 8644709 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I think I developed the original patch on 10.4, and I'm not sure if I ever tested it with multiple monitors.
Attachment #8644709 - Flags: review?(mstange) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e3431a4745626d5c7e6cbe2054f10500fe2232f1
changeset:  e3431a4745626d5c7e6cbe2054f10500fe2232f1
user:       Xidorn Quan <quanxunzhen@gmail.com>
date:       Fri Aug 07 13:49:12 2015 +1000
description:
Bug 1188322 - Always hide menubar as well as dock for fullscreen on OS X whatever the screen is. r=mstange
Comment on attachment 8644709 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1105939 which starts using this code path for Fullscreen API on OS X 10.7+
[User impact if declined]: when using fullscreen page (e.g. watching video) in non-primary screens, the menubar would not be hidden, which is annoying
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low risk, as it merely removes some code whose comment is no longer true for new systems. In addition, :smichaud tested some equivalent code on all versions we are still supporting, and didn't notice any new issue introduced by that.
[String/UUID change made/needed]: n/a
Attachment #8644709 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e3431a474562
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Xidorn, can you please verify the fix works as expected in the latest Nightly build? I can then approve this for uplift to Beta41.
Flags: needinfo?(quanxunzhen)
(In reply to Ritu Kothari (:ritu) from comment #15)
> Xidorn, can you please verify the fix works as expected in the latest
> Nightly build? I can then approve this for uplift to Beta41.

Yes, I've just verified that it works as expected in the Nightly.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8644709 [details] [diff] [review]
patch

see comment 13
Attachment #8644709 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8644709 [details] [diff] [review]
patch

Approved for uplift to Beta as the fix was verified on Nightly.
Attachment #8644709 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I connected a macbook pro retina with 10.9.5 to an external monitor and could reproduce the issue where the menubar is visible in fullscreen mode.
I can confirm that using Firefox 41 beta 8 and latest Dev Edition 42.0a2 2015-09-08, the menubar hides in fullscreen.

I noticed that on the retina screen, the dock and the menubar are also hidden while the video is in fullscreen. Is this expected? Thank you!
Flags: needinfo?(quanxunzhen)
Do you mean, menubar and dock are hidden on all monitors when we have only one fullscreen window?

It is a known issue, see bug 1188331.
Flags: needinfo?(quanxunzhen)
Thanks! Marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.