1406032 - Hamburger menu doesn't work in second window in fullscreen mode on Mac OS High Sierra

Status: RESOLVED DUPLICATE of bug 1405577

(Core :: Widget: Cocoa, defect, P1)

Description

[alexander]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170908115024

Steps to reproduce:

Mac OS, High Sierra. Using latest Nightly Firefox (57.0b5)

1. Start Firefox
2. Switch to fullscreen mode (green system button in top-left corner)
3. Go to "hamburger" menu, click on it, open "New window"
4. In new window try to click on "hamburger" menu.


Actual results:

Menu doesn't open. You need to switch new window in fullscreen mode to make this menu appear


Expected results:

Menu must appear

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Bogdan, are you able to run mozregression on this?

Comment 2

[Bogdan Maris, Desktop QA]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Bogdan, are you able to run mozregression on this?

I found that the first nightly that works on my machine with 10.13 is from 2016-07-12 and that build is also affected by this behavior (using mozregression). The build from 2016-07-11 did not throw any errors it simply did not start (using mozregression).

I also manually installed both builds and I can confirm the above. Dropping regressionwindow-wanted and I don't think we can call this a regression either so dropping that as well.

Here is the pushlog for those builds:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=214884d507ee369c1cf14edb26527c4f9a97bf48&tochange=aac8ff1024c553d9c92b85b8b6ba90f65de2ed08

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Markus, could you look in to this? Maybe this has to do with the popup level of the Firefox menu panel?

Comment 4

[Marcia Knous [:marcia]]

I am not able to reproduce this on my macbook pro using the latest beta (6) running 10.13.1. In my case if I follow the STR it works fine.

Comment 5

[Stephen A Pohl [:spohl]]

I'm able to reproduce with latest nightly.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for confirming Stephen, would you be able to pick this up? I'm mostly worried about this showing a larger issue that we're only seeing one symptom of with these STR.

Comment 7

[Stephen A Pohl [:spohl]]

I'm currently busy with bug 1405151, but I'll keep the n-i set. Are there any errors printed in the browser or system console?

Comment 8

[Stephen A Pohl [:spohl]]

I had another look at this today. It appears that the menu actually does work. However, it is displayed "behind" the browser window. If the new window is moved to the left a bit, clicking the hamburger menu will make a small sliver of the menu appear on the right side of the browser window. It is hard to click on the menu items because only a few pixels are showing, but I've been able to confirm that the menu items do indeed work. I'm not sure how we're currently trying to layer the menu on top of the browser window. :jaws or :mstange, do you know off-hand how this is currently done?

Comment 9

[Markus Stange [:mstange]]

I think it's done by declaring the panel a child window of the main window: http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/widget/cocoa/nsCocoaWindow.mm#889-890
(not sure if this code is really hit for this case, but I would expect it to be.)

The reason I think there's a parent->child link here is that, if I have a browser window in non-fullscreen mode open, open the panel, and then move the browser window by dragging its toolbar, you can briefly see that the panel moves together with it. The panel fades out once you start dragging, but the fact that it is affected by the drag of the browser window is still visible.

I don't know what the XUL side of this looks likes, but my guess would be that you get this parent->child association if you set a window level that's not floating.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thank you! This put me on the right track. This fixes things on macOS 10.13 and I will send to try shortly to confirm.

Comment 11

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8968ada6b2fd7af3827479e312e93a32944464

Comment 12

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7b6f90f1ddb13c817d8d5da3cad1eefa8629e5
Bug 1406032: Ensure that popup windows (such as the hamburger menu window) are shown on top of the browser window on macOS. r=mstange

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing toolkit/content/tests/chrome/test_largemenu.xul in c3 test suite on OS X (this also failed in the Try push):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d86b238b5db553056162c336aac62a213c255329

Push with ran failing test: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7d3d173274366da44e05cd144b20b60370271040&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=138321803&repo=mozilla-inbound

15:20:56     INFO - TEST-PASS | toolkit/content/tests/chrome/test_largemenu.xul | menu movement move after set left and top x to -1 
15:20:56     INFO - TEST-PASS | toolkit/content/tests/chrome/test_largemenu.xul | menu movement move after set left and top y to -1 
15:20:56     INFO - TEST-PASS | toolkit/content/tests/chrome/test_largemenu.xul | menu movement left is not set after moving to -1 
15:20:56     INFO - TEST-PASS | toolkit/content/tests/chrome/test_largemenu.xul | menu movement top is not set after moving to -1 
15:20:56     INFO - TEST-PASS | toolkit/content/tests/chrome/test_largemenu.xul | panel movement (1, 1) x 
15:20:56     INFO - Buffered messages finished
15:20:56     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_largemenu.xul | panel movement (1, 1) y - got 1, expected 23
15:20:56     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
15:20:56     INFO - is@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/window_largemenu.xul:164:24
15:20:56     INFO - testPopupMovement@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/window_largemenu.xul:331:3
15:20:56     INFO - onpopupshown@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/window_largemenu.xul:1:1

Comment 14

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f8573c3b28ba22f6cdc39cbe44ae338e99bb11

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Fix test

Sorry, I must have been looking at a green try run from another bug when I landed this.

Well, with a bit of a detour, I think we've figured out more detail about bug 1324892 comment 54: The reason why [mWindow frame].origin.y in nsCocoaWindow::UpdateBounds()[1] returned different values depending on the SDK used was because the new SDK expected us to set the window's level to NSPopUpMenuWindowLevel. Now that we're starting to do so with attachment 8919982 [details] [diff] [review], we can restore the test to its original form.

Try run in comment 14 to confirm.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8920451 [details] [diff] [review]
Fix test

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

Everything makes more sense now!

Comment 17

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e50716eaebeeb3b6bef731370605bf511f8b094
Bug 1406032: Ensure that popup windows (such as the hamburger menu window) are shown on top of the browser window on macOS. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/9adb0bc561c3c5d16f8c31640a4ff2570feb2cf3
Bug 1406032: Backout 096247c952ea (bug 1324892) to fix popup tests because popups now work correctly again on macOS. r=mstange

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9e50716eaebe

Comment 19

[Stephen A Pohl [:spohl]]

Comment on attachment 8919982 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324892
[User impact if declined]: Popup windows (such as the hamburger menu popup) may not display correctly (or at all) on macOS 10.13.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0 for steps.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very narrow change that restores the behavior that we used to have with the 10.7 SDK before switching to the 10.11 SDK.
[String changes made/needed]: none

Comment 20

[Stephen A Pohl [:spohl]]

Comment on attachment 8920451 [details] [diff] [review]
Fix test

Approval Request Comment
Test changes to cover the code in the patch.

Comment 21

[Stephen A Pohl [:spohl]]

I am not sure that this affects Firefox 56 and ESR52, as these were built with the 10.7 SDK. Will need to verify.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8919982 [details] [diff] [review]
Patch

macos menu fix, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/337db7b083a7
https://hg.mozilla.org/releases/mozilla-beta/rev/e35c13737463

Comment 24

[Bogdan Maris, Desktop QA]

Verified that this issue is no longer reproducible using Firefox 57 beta 11 and latest Nightly 58.0a1 on macOS 10.13.

FYI Fx 56.0.1 and 52.4.1esr are also affected, are we going to fix them as well?

Comment 25

[Stephen A Pohl [:spohl]]

Thanks! So this is a regression in 10.13 and not caused by the SDK switch. Since this isn't security related, I'm not sure we will uplift this to ESR.

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

Might be worth it for esr, risk seems low enough.

Comment 27

[Stephen A Pohl [:spohl]]

Comment on attachment 8919982 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Usability issue
User impact if declined: Popup windows (such as the hamburger menu) may not appear correctly.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky):  Very low risk since this only applies changes to popup windows, which aren't going to work correctly without this patch on 10.13 anyway.
String or UUID changes made by this patch: none

Note: The test patch does not need to be uplifted since the test is already correct on ESR52. The test was changed in 57 to accommodate the SDK switch from 10.7 to 10.11 (bug 132489), but that switch was not uplifted to ESR52.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8919982 [details] [diff] [review]
Patch

I don't think we should try to fix Mac 10.13 regressions this late in the ESR 52 cycle, unless they have a severe impact on usability.  Let's let this wait for ESR 59 unless we see a lot of user complaints.

Comment 29

[Stephen A Pohl [:spohl]]

Ritu, this caused a regression and the fix for it in bug 1412130 is causing more regressions. We should back the patches here out of m-c and 57 as well as the patch in bug 1412130 out of m-c. Could you sign off on these backouts? RyanVM is already in the loop about this.

A better fix for these issues will land in bug 1405577 for 58.

Comment 30

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

I trust you, please go ahead. Thanks for raising the flag for this one.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out from m-c. Beta backout coming shortly.

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0363254a0268

Comment 33

[Stephen A Pohl [:spohl]]

Marking this as a duplicate of bug 1405577 as I can now confirm that it will be fixed by the patch there.