1656301 - [macOS 11 Beta] Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts

Status: VERIFIED FIXED

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

Description

[Haik Aftandilian [:haik]]

Attachment: Screenshot showing Nightly and Safari Popup Menus on Big Sur Beta 3 (20A5323l)

On Nightly 81 on Big Sur Beta 3 (20A5323l), the right click popup menus

• have some small visual artifacts at the edges (see screenshot)
• don't match the look and feel of native popups with respect to
  • border spacing between text and the edges
  • triangle arrows for the sub-menus

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Stephen A Pohl [:spohl]]

This is most likely related to our forcing aqua appearance on (see bug 1623684), or the fact that aqua appearance is no longer supported in macOS 11.

Comment 4

[Haik Aftandilian [:haik]]

[Tracking Requested - why for this release]: Look and feel bug affecting macOS 11.

Comment 5

[Haik Aftandilian [:haik]]

(In reply to Stephen A Pohl [:spohl] from comment #3)

This is most likely related to our forcing aqua appearance on (see bug 1623684), or the fact that aqua appearance is no longer supported in macOS 11.

I tested with the NSRequiresAquaSystemAppearance key removed from our Info.plist, and also with building on macOS 11, but it didn't change the appearance of the menu.

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Haik Aftandilian [:haik] from comment #5)

I tested with the NSRequiresAquaSystemAppearance key removed from our Info.plist, and also with building on macOS 11, but it didn't change the appearance of the menu.

What SDK were you using when building on macOS 11? And did you test with the NSRequiresAquaSystemAppearance key removed when building on macOS 11?

Comment 7

[Haik Aftandilian [:haik]]

(In reply to Stephen A Pohl [:spohl] from comment #3)

This is most likely related to our forcing aqua appearance on (see bug 1623684), or the fact that aqua appearance is no longer supported in macOS 11.

With macOS 11, I did have NSRequiresAquaSystemAppearance removed from the plist. And my build was without any --with-macos-sdk setting in my mozconfig. I also tried RenderWithCoreUI() in nsNativeThemeCocoa.mm to not use the Aqua appearance.

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Affect of removing NSRequiresAquaSystemAppearance

I noticed a difference in how right-click context menus are displayed with the NSRequiresAquaSystemAppearance key removed. For right-click menus shown when right-clicking on a link, the vertical spacing of the menu items is more condensed.

Comment 9

[lolrepeatlol]

(In reply to Haik Aftandilian [:haik] from comment #8)

Created attachment 9173680 [details]
Affect of removing NSRequiresAquaSystemAppearance

I noticed a difference in how right-click context menus are displayed with the NSRequiresAquaSystemAppearance key removed. For right-click menus shown when right-clicking on a link, the vertical spacing of the menu items is more condensed.

I could just be stupid but that might be the fake context menu trying to imitate Yosemite as the font is Helvetica and not San Francisco.

Comment 10

[Markus Stange [:mstange]]

Yes, Firefox's context menus are fake. The menu separators are drawn using HITheme, in DrawMenuSeparator.
It is possible that there's another way to draw these separators (maybe with CoreUI) that matches the native appearance.

(In reply to Alexei Solonari from comment #9)

I could just be stupid but that might be the fake context menu trying to imitate Yosemite as the font is Helvetica and not San Francisco.

Indeed, it looks like the difference in spacing is just down to the font difference.

Comment 11

[lolrepeatlol]

(In reply to Markus Stange [:mstange] from comment #10)

Yes, Firefox's context menus are fake. The menu separators are drawn using HITheme, in DrawMenuSeparator.
It is possible that there's another way to draw these separators (maybe with CoreUI) that matches the native appearance.

(In reply to Alexei Solonari from comment #9)

I could just be stupid but that might be the fake context menu trying to imitate Yosemite as the font is Helvetica and not San Francisco.

Indeed, it looks like the difference in spacing is just down to the font difference.

Why not just use the native context menus, that way this doesn't have to be worried about in the future? Is it very hard, because of Firefox being cross-platform?

I'm just curious. I hope you don't mind me asking. Also, if you'd like, I'd be happy to (try) and research CoreUI if it helps. :)

Comment 12

[Haik Aftandilian [:haik]]

(In reply to Alexei Solonari from comment #11)

Why not just use the native context menus, that way this doesn't have to be worried about in the future? Is it very hard, because of Firefox being cross-platform?

I talked to the Mac widget experts and the reason we use fake context menus is inertia--i.e., we did that a long time ago and it has been easier to adjust that implementation compared to rewriting natively--and also due to the custom back/forward/reload/bookmark menu items requiring it (at least at some point in time.)

I'm just curious. I hope you don't mind me asking. Also, if you'd like, I'd be happy to (try) and research CoreUI if it helps. :)

Any recommendations or ideas are welcome.

Comment 13

[Haik Aftandilian [:haik]]

The root cause of this problem appears to be a regression in HIThemeDrawMenuSeparator() on the macOS 11 Betas on Intel, but HIThemeDrawMenuSeparator() has been deprecated for years and Apple indicated this is not likely to be fixed.

I tested a basic change in toolkit/themes/osx/global/menu.css adding a top border to menuseparator and changed nsNativeThemeCocoa::DrawMenuSeparator() to not draw anything. This would need some tweaks to match the Big Sur look and feel. However, I'm not knowledgeable about the right way to apply such a style change only on macOS 11+ and don't have a lot of experience with front end styling. We also need a front-end fix to change the submenu triangles to arrows to match the Big Sur look. @dao, would you be able to help with this or recommend someone to take the bug?

I tested using HIThemeDrawSeparator() instead as a workaround. That draws a darker line the full width of the popup by default which matches the style of macOS Catalina, but not Big Sur (ignoring the darker line).

I also tested using HIThemeDrawMenuSeparator() and making the line longer so it is wider than the popup. The result is no visible artifacts, but the style matches macOS Catalina, not Big Sur, because the separator width extends across the menu completely.

I haven't found a more modern way to draw the separator respecting the OS look and feel.

Comment 14

[lolrepeatlol]

(In reply to Haik Aftandilian [:haik] from comment #12)

(In reply to Alexei Solonari from comment #11)

Why not just use the native context menus, that way this doesn't have to be worried about in the future? Is it very hard, because of Firefox being cross-platform?

I talked to the Mac widget experts and the reason we use fake context menus is inertia--i.e., we did that a long time ago and it has been easier to adjust that implementation compared to rewriting natively--and also due to the custom back/forward/reload/bookmark menu items requiring it (at least at some point in time.)

I'm just curious. I hope you don't mind me asking. Also, if you'd like, I'd be happy to (try) and research CoreUI if it helps. :)

Any recommendations or ideas are welcome.

That's understandable, but doing so causes issues like this that need to be fixed every time macOS gains a new style, which is not trivial. After all, it has almost been two years after the release of Mojave, yet dark mode has not yet been implemented into the fake context menu. I'm not sure if you are aware, but spohl has suggested to remove HITheme entirely and render the context menu (and possibly tooltips) natively, which would overcome this problem, dark mode problems, future macOS redesign problems, and help with the progress on using the latest macOS SDK instead of the 10.11 SDK. I will request info from spohl to see what they think about fixing this issue with the continued use of HITheme. :)

Another idea would be to not mimic the native style of the context menu and instead use some sort of non-native theme, preferably Photon style, meaning there would be no cat-and-mouse game of constantly trying to match the new macOS style. I believe a full rewrite would not be required. For an example of an app that has this behavior, see Microsoft Edge's macOS context menu: https://imgur.com/a/9iliLSR

Comment 15

[Stephen A Pohl [:spohl]]

Yes, Haik and I have discussed this bug. Unfortunately, at this time, there isn't much to add beyond what Haik said in comment 12 and comment 13.

Comment 16

[Haik Aftandilian [:haik]]

(In reply to Alexei Solonari from comment #14)

Any recommendations or ideas are welcome.

That's understandable, but doing so causes issues like this that need to be fixed every time macOS gains a new style, which is not trivial.

To add a bit more explanation, the macOS 11 release date is approaching and we will have to uplift this fix to Firefox ESR and Firefox Release so the right fix for now is something low risk. But how we fix this bug is not necessarily indicative of what we should do long term.

I was going to suggest you file a new bug, but we have a very old bug (21 years) to do this. Please add comments to bug 34572. We are definitely not against using native menus, but it might not be high priority enough to work on now. And we need to prototype a native menu with the back/forward/reload/bookmark menu items like what we have today.

Comment 17

[Michal Jakubowski]

Another thing against this "fake" context menu - it still completely ignores dark mode being enabled, at least on custom themes (which was added back in Mojave)

Comment 18

[Haik Aftandilian [:haik]]

Attachment: WIP - implement separator in CSS

WIP patch that implements the menu separators in CSS on macOS, matching the Big Sur look and avoiding the white line artifacts.

Comment 19

[Haik Aftandilian [:haik]]

Attachment: WIP - screenshot

Comment 20

[Haik Aftandilian [:haik]]

@ntim, are you able to help with this. See comment 13 for more details. Thanks.

Comment 21

[Tim Nguyen :ntim]

If you would like to target this CSS specifically to Big Sur, this needs a new media query value, see:

https://searchfox.org/mozilla-central/rev/3d53187b90605ccef321c9cadbba762ad06a6381/widget/cocoa/nsNativeThemeCocoa.mm#319-321
https://searchfox.org/mozilla-central/rev/3d53187b90605ccef321c9cadbba762ad06a6381/widget/cocoa/nsNativeThemeCocoa.mm#355-366

Then:

@media (-moz-mac-big-sur-theme) {

}

But it might be a bit more elegant to unify these media queries with -moz-os-version: https://searchfox.org/mozilla-central/search?q=moz-os-version&path=&case=false&regexp=false , though I don't think there's an immediate need for this.. -moz-os-version works a bit differently nvm (has values for each version, not just a few).

Comment 22

[Tim Nguyen :ntim]

This might be helpful: https://searchfox.org/mozilla-central/search?q=-moz-mac-yosemite-theme&path=&case=false&regexp=false

Comment 23

[Haik Aftandilian [:haik]]

Attachment: WIP - implement separator in CSS

Comment 24

[Tim Nguyen :ntim]

Looks reasonable at first sight.

Comment 25

[Itiel]

In the @media (-moz-mac-bigsur-theme) part you can simplify the margin to margin: 5px 21px;.

Comment 26

[Markus Stange [:mstange]]

I think it would be better to keep using -moz-appearance for this. That's where we try to keep the OS-version dependent differences, if we can.
Since this is really just a line, we should be able to draw it with CGContextFillRect with some hardcoded color.

The "-moz-mac-bigsur-theme" media will definitely be useful regardless, so let's add that in a separate patch (and ideally a separate bug) anyway.

Comment 27

[Tim Nguyen :ntim]

I totally agree with Markus if that's easily possible, since -moz-default-appearance is unlikely to be deprecated at this point. He's also much more familiar with the platform bits than I am.

My only concern is how easy it is to consider the menu icon spacing as well now on the platform side, since Big Sur doesn't seem to have some by default. That might still require some media query code, but maybe it doesn't matter too much for UX.

Comment 28

[Haik Aftandilian [:haik]]

Attachment: CGContextFillRect() Screenshot

Here's a screenshot using the CGContextFillRect() workaround using the following code.

  // Workaround for visual artifacts issues with                                       
  // HIThemeDrawMenuSeparator on macOS Big Sur.                                        
  if (nsCocoaFeatures::OnBigSurOrLater()) {                                            
    CGRect separatorRect = inBoxRect;                                                  
    separatorRect.size.width -= 42;                                                    
    separatorRect.size.height = 1;                                                     
    separatorRect.origin.x += 21;                                                      
    // Use a gray color similar to the native separator                                
    DeviceColor color = ToDeviceColor(mozilla::gfx::sRGBColor::FromU8(193, 208, 208, 255)); 
    CGContextSetRGBFillColor(cgContext, color.r, color.g, color.b, color.a);     
    CGContextFillRect(cgContext, separatorRect);                                       
    return;                                                                            
  }

Comment 29

[Haik Aftandilian [:haik]]

(In reply to Markus Stange [:mstange] from comment #26)

I think it would be better to keep using -moz-appearance for this. That's where we try to keep the OS-version dependent differences, if we can.
Since this is really just a line, we should be able to draw it with CGContextFillRect with some hardcoded color.

The "-moz-mac-bigsur-theme" media will definitely be useful regardless, so let's add that in a separate patch (and ideally a separate bug) anyway.

I've changed nsNativeThemeCocoa::DrawMenuSeparator() to use CGContextFillRect() on Big Sur and I think the results are acceptable. I noticed the margins and vertical spacing don't match Big Sur and those are being controlled by menu.css styling. So I've used the @media (-moz-mac-bigsur-theme to set those for Big Sur. Markus, is this consistent with what you meant or should this be set somewhere else. Code is on phabricator now.

I still need to change the a) submenu triangle to be an arrow and b) the submenu placement offset to match Big Sur native. Fixing the arrow is important, but the offset could be addressed later. I can't tell where we get the current arrow image from though.

I will also post a screenshot showing the new popup and alignment.

Comment 30

[Haik Aftandilian [:haik]]

Attachment: Bug 1656301 - Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts r?mstange!

Add a Big Sur-specific @media query.

Avoid the menu separator artifacts caused by HIThemeDrawMenuSeparator() on macOS Big Sur by drawing the the separator with CGContextFillRect() instead.

Adjust the popup menu margins to more closely mimic Big Sur native menus.

Needs additional changes for the submenu arrow and submenu placement offset.

Comment 31

[Haik Aftandilian [:haik]]

Attachment: WIP - Screenshot with native comparison

Comment 32

[Markus Stange [:mstange]]

Nice work!

(In reply to Haik Aftandilian [:haik] from comment #29)

(In reply to Markus Stange [:mstange] from comment #26)

I think it would be better to keep using -moz-appearance for this. That's where we try to keep the OS-version dependent differences, if we can.
Since this is really just a line, we should be able to draw it with CGContextFillRect with some hardcoded color.

The "-moz-mac-bigsur-theme" media will definitely be useful regardless, so let's add that in a separate patch (and ideally a separate bug) anyway.

I've changed nsNativeThemeCocoa::DrawMenuSeparator() to use CGContextFillRect() on Big Sur and I think the results are acceptable. I noticed the margins and vertical spacing don't match Big Sur and those are being controlled by menu.css styling.

Interesting. A better solution would be to move the padding out of the CSS into GetWidgetPadding or GetWidgetBorder.

I can't tell, are we using the correct font now? If not, we should worry about that first and then fix the spacing afterwards because the font also affects the spacing.

So I've used the @media (-moz-mac-bigsur-theme to set those for Big Sur. Markus, is this consistent with what you meant or should this be set somewhere else. Code is on phabricator now.

I think we should try to move as much as we can outside of the CSS code. As for the spacing on the sides, I preferred your solution from comment 28 which was adjusting the rectangle bounds in nsNativeWidgetCocoa.

I still need to change the a) submenu triangle to be an arrow and b) the submenu placement offset to match Big Sur native. Fixing the arrow is important, but the offset could be addressed later. I can't tell where we get the current arrow image from though.

The arrow is styled with -moz-default-appearance: menuarrow (it's the <hbox class="menu-right"> element). So it's also drawn with HITheme at the moment.
It's possible that there is a named NSImage for the new arrow. If you create a new project in Xcode and go to the window interface builder pane, you'll be able to add an NSImage to the window and it'll show you a list of available images.

Thanks for the screenshots, they're very useful!

Comment 33

[Markus Stange [:mstange]]

Here's how I usually find out how to call CoreUI drawing.

1. In a mostly-empty "App" project in Xcode, add the widget you're interested in to your window. In our case, we're interested in menu drawing, so I just add a text field to the window, because text fields have context menus which contain all the interesting widgets.
2. I run the project. This displays my window.
3. In Xcode's debugging pane, I pause the debugger and put the following into its lldb command line thingy:

(lldb) b CUIDraw
Breakpoint 1: where = CoreUI`CUIDraw, address = 0x00007fff47d090d6
(lldb) br com add 1
Enter your debugger command(s).  Type 'DONE' to end.
> po $rdx
> c
> DONE

This prints the value of the drawing NSDictionary on every invocation to CUIDraw. The argument happens to be passed in the rdx register.

4. I continue execution.
5. I right-click the text field.
6. Now the lldb console contains a whole bunch of json-like dictionaries.

One of them happens to be

{
    widget = kCUIWidgetMenuItemSeparator;
}

However, in this case, I think manually drawing the separator line is preferable because it doesn't rely on undocumented internals and is less likely to break during macOS updates. In fact, we ran into just this problem with the menu arrows on 10.11, where the internal image names changed: https://hg.mozilla.org/mozilla-central/rev/f274aac3256b

Comment 34

[Haik Aftandilian [:haik]]

(In reply to Markus Stange [:mstange] from comment #32)

Nice work!

(In reply to Haik Aftandilian [:haik] from comment #29)

(In reply to Markus Stange [:mstange] from comment #26)

I think it would be better to keep using -moz-appearance for this. That's where we try to keep the OS-version dependent differences, if we can.
Since this is really just a line, we should be able to draw it with CGContextFillRect with some hardcoded color.

The "-moz-mac-bigsur-theme" media will definitely be useful regardless, so let's add that in a separate patch (and ideally a separate bug) anyway.

I've changed nsNativeThemeCocoa::DrawMenuSeparator() to use CGContextFillRect() on Big Sur and I think the results are acceptable. I noticed the margins and vertical spacing don't match Big Sur and those are being controlled by menu.css styling.

Interesting. A better solution would be to move the padding out of the CSS into GetWidgetPadding or GetWidgetBorder.

I'll give this a shot.

I can't tell, are we using the correct font now? If not, we should worry about that first and then fix the spacing afterwards because the font also affects the spacing.

The font is correct, but some characters have slight anti aliasing differences when I compare my screenshots. The color is not an exact match, but I don't know if that is caused by HIThemeDrawMenuBackground() being slightly off on Big Sur or some transparency effects.

So I've used the @media (-moz-mac-bigsur-theme to set those for Big Sur. Markus, is this consistent with what you meant or should this be set somewhere else. Code is on phabricator now.

I think we should try to move as much as we can outside of the CSS code. As for the spacing on the sides, I preferred your solution from comment 28 which was adjusting the rectangle bounds in nsNativeWidgetCocoa.

OK.

I still need to change the a) submenu triangle to be an arrow and b) the submenu placement offset to match Big Sur native. Fixing the arrow is important, but the offset could be addressed later. I can't tell where we get the current arrow image from though.

The arrow is styled with -moz-default-appearance: menuarrow (it's the <hbox class="menu-right"> element). So it's also drawn with HITheme at the moment.
It's possible that there is a named NSImage for the new arrow. If you create a new project in Xcode and go to the window interface builder pane, you'll be able to add an NSImage to the window and it'll show you a list of available images.

Ah, thanks for the explanations.

Comment 35

[Haik Aftandilian [:haik]]

Using GetWidgetPadding() and GetWidgetBorder(), I've been able get similar results. There are some problems with the test cases :ntim pointed out on phabricator where there is a checkmark in the menu. In that case the alignment is not correct. Will post some screenshots. Is it possible to address this from platform code?

Comment 36

[Haik Aftandilian [:haik]]

Attachment: Checkbox misalignment with platform code fix

Comment 37

[Haik Aftandilian [:haik]]

The latest version of the fix only attempts to 1) workaround the separator artifacts 2) adjust the width of the separator to resemble Big Sur and 3) move the submenu triangle in a bit to be aligned with the separator.

I will file another bug to address the other differences with Big Sur popups because it is going to require front end changes and lots more tweaking OR possibly motivate the switch to native menus. And we are not sure how much time we have before Big Sur releases.

The changes are limited to when we are running on Big Sur.

Comment 38

[Haik Aftandilian [:haik]]

Attachment: Screenshot showing the fixed versions of the popup currently under review.

Comment 39

[Haik Aftandilian [:haik]]

Comment on attachment 9177797 [details]
Bug 1656301 - Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts r?mstange!

Beta/Release Uplift Approval Request

• User impact if declined: On macOS 11, right-click popups will display white-line artifacts.

(We don't know when macOS 11 will ship, but it is expected within the next month.)

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open a right-click popup.
• List of other uplifts needed: 1668493
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are limited to macOS 11.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Right-click popups display white-line artifacts on macOS 11 resulting in a buggy appearance.

(We don't know when macOS 11 will ship, but it is expected within the next month.)

• User impact if declined: On macOS 11, right-click popups will display white-line artifacts.
• Fix Landed on Version: 83
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are limited to macOS 11.
• String or UUID changes made by this patch: None

Edited to include bug 1668493 on October 5th, 2020.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6550edff7eb2
Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts r=mstange

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9177797 [details]
Bug 1656301 - Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts r?mstange!

Let's let this ride with 82 in a couple weeks.

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/6550edff7eb2

Comment 43

[Julien Cristau [:jcristau]]

Comment on attachment 9177797 [details]
Bug 1656301 - Popup Menus Don't Match Big Sur Look and Feel, Have White Line Artifacts r?mstange!

approved for 82.0b6 and 78.4.0esr

Comment 44

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52e8c78edd72

Comment 45

[Sören Hentzschel]

Attachment: screenshot

On my MacBook Pro with macOS Big Sur Beta (20A5374i) the color of the separators is very different compared to Safari. Is this expected?

Comment 46

[Peter Magyari (Desktop QA)]

Reproduced the issue on Firefox 81.0 (20200917005511)
Verified fixed on Firefox Beta 82.0b6 (20200930122753) and Firefox Nightly 83.0a1 (20200930214529)

Comment 47

[Cristian Fogel [:cfogel]]

Noticed the same issue as in comment 45.

@Sören
for tracking purposes, I've taken the liberty and filed 1668493.
Clearing needinfo off Haik for this bug, since we will continue the discussion on that thread/bug.

Comment 48

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: blue-ish tint.png

Stumbled upon this, and verified too with the build Julien posted in Comment 44. Seems that the bars between have a blue-ish tint on them. I couldn't see that on the retina display, but it was quite noticeable on my second display. I don't know if this is intended or not, but I wanted to let this note here.

Comment 49

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: close-up.png

And here's a close-up on one of the bars.

Comment 50

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I just found Bug 1668493, so I'll just repost these there.

Comment 51

[Haik Aftandilian [:haik]]

Moved my comment to bug 1668493 comment 3.

Comment 52

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/fad7cbf79ff6

Comment 53

[Haik Aftandilian [:haik]]

@jcristau, this fix requires bug 1668493 being uplifted as well. Do I need to file uplift requests on bug 1668493? Thanks.

Comment 54

[Julien Cristau [:jcristau]]

Yes please :)

Comment 55

[Peter Magyari (Desktop QA)]

Verified on Firefox 78.4.0esr (20201002092524) using MacOS 11.0 (20A5384c)