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

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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/[e-mail]-c614f64fbcd8/try-macosx64/

Comment 3

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

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).

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(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 11

[Markus Stange [:mstange]]

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.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e3431a4745626d5c7e6cbe2054f10500fe2232f1
changeset:  e3431a4745626d5c7e6cbe2054f10500fe2232f1
user:       Xidorn Quan <[e-mail]>
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 13

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3431a474562

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Xidorn, can you please verify the fix works as expected in the latest Nightly build? I can then approve this for uplift to Beta41.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8644709 [details] [diff] [review]
patch

see comment 13

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8644709 [details] [diff] [review]
patch

Approved for uplift to Beta as the fix was verified on Nightly.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4aa0864df9f

Comment 20

[Petruta Horea [:phorea]]

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!

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 22

[Petruta Horea [:phorea]]

Thanks! Marking as verified.