1710459 - Use native menus for toolbar button dropdowns ("anchored" menupopups)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement, P3)

Description

[Markus Stange [:mstange]]

As of bug 34572 we now use native menus for context menus on macOS. However, "anchored" menus are still non-native. These can be seen in the following places:

• When long-pressing the back button / forward button
• In the library window, when pressing any of the three dropdown toolbar buttons.

We should switch these menus to use native menus as well.

However, the following menus should remain non-native:

• <select> popups
• XUL menulist popups
• bookmark folder dropdowns in the bookmarks toolbar

Comment 1

[Markus Stange [:mstange]]

I expect there to be three parts to fixing this bug:

1. Finding the right API to open an anchored menupopup, in such a way that the menu doesn't overlap the button even if the button is positioned close to the screen's edges.
2. Making sure that this change doesn't affect bookmark folder dropdowns (or any of the other exclusions listed in the previous comment).
3. Getting tests to pass.

Comment 4

[Markus Stange [:mstange]]

It is possible to open an NSMenu relative to an anchor rect by following what this chromium code does:

• Create an NSPopUpButtonCell
• Create an empty NSView which covers the anchor rect
• Add this anchor NSView to the window
• call -[NSPopUpButtonCell attachPopUpWithFrame:inView:] and -[NSPopUpButtonCell performClickWithFrame:inView:]
• once the menu is closed, remove the NSView again

Comment 5

[Bernard Igiri]

I marked the blocking ticket, Bug 1729770, as fixed. I'm leaving it at the results that I have already collected, so that I can refocus on higher priorities. Hopefully whoever takes this ticket up will find those results helpful.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Probably not a S2
Reminder, S2 means: (Serious) Major functionality/product severely impaired or a high impact issue and a satisfactory workaround does not exist

And it seems to be part of the backlog now

Comment 10

[Sam Johnson]

I have a working patch that addresses this bug, however, I have a couple questions before I submit anything:

1. I ran locally the tests mentioned in bug 1729770 comment 3, however they all passed with expected results without any changes to the tests. Is this plausible? It seems too good to be true :)
2. The chromium code sample linked in comment 4 sets pullsDown:NO on the NSPopUpButtonCell. We don't want that, because this causes the menu to overlap the button. Using pullsDown:YES prevents the overlap, but the NSPopUpButtonCell eats the first item of the NSMenu for use as the placeholder item. I was able to work around this by adding a disabled "dummy" item to the top of the menu in nsMenuX for anchored menus. Everything seems to work OK, but I am unsure if there are any subtle interactions here that might be unhappy about this (outside of OnHighlightedItemChanged and some other functions which work on the index, but is easily handled). Is this OK?
3. To exclude the bookmarks toolbar menus, I am looking at the container attribute that is already present on them. I don't think that is right. Is it more appropriate add native="false" or some other attribute as a method to exclude these?
4. What is the motivation behind excluding <select> and XUL menulists, given these use a native-like appearance these days--is it due to the tests that will break?

Thanks!!

Comment 11

[Sam Johnson]

I've mostly figured out the above, so I am going to just submit the patch for feedback :)

Comment 12

[Sam Johnson]

Attachment: Bug 1710459 - Implement native anchored menus on macOS. r?#mac-reviewers

This patch utilizes the existing native menu infrastructure to add support for displaying native anchored menus on macOS, controllable via prefs:

• widget.macos.native-anchored-menus enables native anchored menus on macOS, except for <menulist> and <select>. This is the only pref that needs to be enabled to achieve native menus for the items specified in the bug description.
• widget.macos.native-anchored-menulists enables native menus for XUL menulists, when the above pref is also enabled. This is disabled per the bug description, but is fully functional if enabled.
• widget.macos.native-anchored-select enables native menus for <select> popups, and therefore is disabled per the bug description, but is fully functional if enabled.

To allow selective usage of non-native menus where desired, thenative="false" attribute can be set on the menu. This is currently used for the bookmarks dropdowns in the browser toolbar, which allow drag-drop reordering and right-clicking, which native menus do not support. It is also used to disable native menus for <select> popups when widget.macos.native-anchored-select is disabled, or when dom.forms.select.customstyling or dom.forms.selectSearch are enabled.

Native anchored menus are achieved on macOS by creating a NSPopUpButtonCell and attaching a NSMenu and the desired anchor rect to it. This is used instead of [NSMenu popUpMenuPositioningItem:atLocation:inView:] so that the OS can decide popup placement without us needing to perform brittle calculations. The pullsDown property is used to manage whether or not the menu should overlap the anchor. Because XUL popups support many more anchor alignment configurations than Cocoa, they are mapped to the closest value.

When a NSPopUpButtonCell has the pullsDown property set to true, Cocoa uses the first NSMenuItem as the placeholder item for the button and hides it from the menu. To handle this, a disabled placeholder item is added to the top of NSMenu in this condition.

Native menus now also invoke the menu button's PopupOpened and PopupClosed handlers so that the open attribute is properly set on the button.

Cocoa native menu support has also been taught several new tricks in order to allow their usage as much as possible:

• <label> and <menucaption> elements now generate a disabled menu item. <menucaption>s are used for HTML <optgroup>s, and an example usage of <label> is in the searchmode switcher (I am not sure why the searchmode switcher is using <label> here instead of something else).
• tooltiptext is now supported for showing a tooltip on NSMenuItems.
• Menu items within an HTML <optgroup> display indented. Unfortunately <menuitem> currently has no attribute to indicate this, so we need to look at the class.
• Font sizes for menu items from the style system are now applied to native menu items of anchored popup menus. While Chrome and Safari use the font size of the <select> element rather than the font size of the individual menu items, doing this per-item instead provides consistency with non-native menus and existing user expectations.
• Menu items using the selected attribute instead of the checked attribute now correctly appear checked, and changes to these properties while the menu is open are now properly reflected.
• The placeholder item for empty native menus has been shrunk a bit so that when an already-open menu has all of its items removed and repopulated, it does not grow undesirably. An example scenario is at the completion of a CSS transform on a <select>.
• There were additional existing issues with inserting NSMenuItems within currently-open menus on recent versions of macOS that became more apparent when enabling the prefs from this patch. On recent versions of macOS (at least 15 and 26, but not 10.15 or 11), NSMenuItem insertions can clobber menu items at or below the insertion point. A workaround for this was implemented that recreates in-place the NSMenuItems that exist after the insertion point.

Tests have not been touched yet in this patch. I expect a large number of tests to break if all 3 prefs are enabled, but hopefully a much smaller number with only widget.macos.native-anchored-menus enabled.

Comment 13

[Sam Johnson]

Attachment: Bug 1710459 - Change search mode switcher description label to a menucaption. r?emilio

The search mode switcher menu includes a <label> element, which generally should not be present in menus. This patch changes it to a disabled <menuitem>, which is similar to other instances of descriptive text in menus, such as the extensions panel menus.

This patch is required for the text to be shown in native menus.

Comment 14

[Sam Johnson]

Attachment: Bug 1710459 - Use indent attribute for content select <optgroup> items. r?emilio

This allows native menus to cleanly indent menu items within <optgroup>s as opposed to checking a class name. Note that this is a boolean rather than a "nesting level" because <optgroup>s cannot currently be nested.

Comment 15

[Sam Johnson]

Attachment: Bug 1710459 - Support custom font sizes in native anchored popups on macOS. r?emilio

This patch teaches Cocoa native menu support to apply custom font sizes from the style system for menu items within anchored popups only. These are used for anchored popups only because anchored pulldowns and unanchored popups should look as native as possible. This patch gives <select> and <menulist> items the appropriate font sizes within native menus on macOS.

While Chrome and Safari use the font size of the <select> element rather than the font size of the individual menu items, doing this per-item instead provides consistency with non-native menus and existing user expectations.

Comment 16

[Sam Johnson]

Attachment: Bug 1710459 - Convert sidebar history sort menu heading to a menucaption. r?#sidebar-reviewers

The sidebar history sort by menu includes a <h1> element, which generally should not be present in menus. This patch changes it to a disabled <menuitem>, which is similar to other instances of descriptive text in menus, such as the extensions panel menus. Styles have been applied to maintain the existing appearance.

This patch is required for the "Sort by" text to be shown in native menus.

Comment 17

[Markus Stange [:mstange]]

Sorry it took me so long to get to this! And thank you for working on it!

(In reply to Sam Johnson from comment #10)

1. I ran locally the tests mentioned in bug 1729770 comment 3, however they all passed with expected results without any changes to the tests. Is this plausible? It seems too good to be true :)

Huh, I'm not sure. Maybe you found an answer to this in the meantime?

2. The chromium code sample linked in comment 4 sets pullsDown:NO on the NSPopUpButtonCell. We don't want that, because this causes the menu to overlap the button. Using pullsDown:YES prevents the overlap, but the NSPopUpButtonCell eats the first item of the NSMenu for use as the placeholder item. I was able to work around this by adding a disabled "dummy" item to the top of the menu in nsMenuX for anchored menus. Everything seems to work OK, but I am unsure if there are any subtle interactions here that might be unhappy about this (outside of OnHighlightedItemChanged and some other functions which work on the index, but is easily handled). Is this OK?

Weird that it eats the first item. I'll take a look at the patch and see if I notice any spots where we get confused about indexes.

3. To exclude the bookmarks toolbar menus, I am looking at the container attribute that is already present on them. I don't think that is right. Is it more appropriate add native="false" or some other attribute as a method to exclude these?

Not sure.

4. What is the motivation behind excluding <select> and XUL menulists, given these use a native-like appearance these days--is it due to the tests that will break?

I think the idea was just to limit the scope so that it's easier to land in one go. For XUL menulists I think it makes total sense to use native popups always. For <select> I can imagine people preferring the non-native versions because the non-native version is probably better at handling cases where the popup contains a huge number of entries, because (I believe) it gives you a scrollbar rather than just scroll arrows.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/03e4687e006e
https://hg.mozilla.org/integration/autoland/rev/c70f8e50a978
Implement native anchored menus on macOS. r=places-reviewers,layout-reviewers,emilio,mstange,credential-management-reviewers,search-reviewers,urlbar-reviewers,dimi,Standard8,mak
https://github.com/mozilla-firefox/firefox/commit/2b67330d50ba
https://hg.mozilla.org/integration/autoland/rev/eef050630469
Change search mode switcher description label to a menucaption. r=emilio,fluent-reviewers,desktop-theme-reviewers,urlbar-reviewers,dao,bolsson
https://github.com/mozilla-firefox/firefox/commit/b679bbd6ba22
https://hg.mozilla.org/integration/autoland/rev/c22cb3034a8a
Use indent attribute for content select <optgroup> items. r=emilio,desktop-theme-reviewers
https://github.com/mozilla-firefox/firefox/commit/8f67288b12cd
https://hg.mozilla.org/integration/autoland/rev/5c3136eae69b
Support custom font sizes in native anchored popups on macOS. r=emilio,mstange
https://github.com/mozilla-firefox/firefox/commit/d7ce7c62d876
https://hg.mozilla.org/integration/autoland/rev/82dc65d15df5
Convert sidebar history sort menu heading to a menucaption. r=fluent-reviewers,desktop-theme-reviewers,sclements,bolsson,dao

Comment 19

[pstanciu]

Backed out for causing bp-nu bustages @nsMenuUtilsX.mm

• https://hg.mozilla.org/integration/autoland/rev/03f5e43868b667513962f49accfb16f432a6b3ed
• Push with failures Bp-nu
• Failure Log Bp-nu
• Push with failures Marinette
• Failure Log Marinette

Comment 20

[Sam Johnson]

Sorry about that! I've updated the patch to fix the issues.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/30312694b765
https://hg.mozilla.org/integration/autoland/rev/f2aba2f62c61
Implement native anchored menus on macOS. r=places-reviewers,layout-reviewers,emilio,mstange,credential-management-reviewers,search-reviewers,urlbar-reviewers,dimi,Standard8,mak,spohl
https://github.com/mozilla-firefox/firefox/commit/f42303af58ca
https://hg.mozilla.org/integration/autoland/rev/9677a501d2ee
Change search mode switcher description label to a menucaption. r=emilio,fluent-reviewers,desktop-theme-reviewers,urlbar-reviewers,dao,bolsson
https://github.com/mozilla-firefox/firefox/commit/0b6ea7581f8a
https://hg.mozilla.org/integration/autoland/rev/9cfa183f2f01
Use indent attribute for content select <optgroup> items. r=emilio,desktop-theme-reviewers
https://github.com/mozilla-firefox/firefox/commit/7a4471ba46e9
https://hg.mozilla.org/integration/autoland/rev/19a515a8fdb0
Support custom font sizes in native anchored popups on macOS. r=emilio,mstange
https://github.com/mozilla-firefox/firefox/commit/f4e5ae52151c
https://hg.mozilla.org/integration/autoland/rev/8a5e52ca891d
Convert sidebar history sort menu heading to a menucaption. r=fluent-reviewers,desktop-theme-reviewers,sclements,bolsson,dao

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2e1507af9214
https://hg.mozilla.org/integration/autoland/rev/3a90b09a023c
Revert "Bug 1710459 - Convert sidebar history sort menu heading to a menucaption. r=fluent-reviewers,desktop-theme-reviewers,sclements,bolsson,dao" for causing bc failures on browser_history_sidebar.js

Comment 23

[Silaghi Andreea]

Backed out for causing multiple mochitests failures in browser_history_sidebar.js

Backout Link
Push with failures
Failure Log
Failure line TEST-UNEXPECTED-FAIL | browser/components/sidebar/tests/browser/browser_history_sidebar.js | Shutdown - leaked 1 window(s) until shutdown [url = chrome://browser/content/sidebar/sidebar-history.html]

Comment 24

[Sam Johnson]

I'm so sorry for all these backouts :( I thought that was a known intermittent failure, maybe some timing change from the patch caused it to become a perma fail? I'll see what I can do.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/48fc0bae0f22
https://hg.mozilla.org/integration/autoland/rev/eaaa92340bd3
Implement native anchored menus on macOS. r=places-reviewers,layout-reviewers,emilio,mstange,credential-management-reviewers,search-reviewers,urlbar-reviewers,dimi,Standard8,mak,spohl
https://github.com/mozilla-firefox/firefox/commit/39763474e1a7
https://hg.mozilla.org/integration/autoland/rev/35550b8a56cc
Change search mode switcher description label to a menucaption. r=emilio,fluent-reviewers,desktop-theme-reviewers,urlbar-reviewers,dao,bolsson
https://github.com/mozilla-firefox/firefox/commit/92eed91736cf
https://hg.mozilla.org/integration/autoland/rev/f4db86131ee2
Use indent attribute for content select <optgroup> items. r=emilio,desktop-theme-reviewers
https://github.com/mozilla-firefox/firefox/commit/561565764e77
https://hg.mozilla.org/integration/autoland/rev/dbafd35ead32
Support custom font sizes in native anchored popups on macOS. r=emilio,mstange
https://github.com/mozilla-firefox/firefox/commit/bf4ea752c244
https://hg.mozilla.org/integration/autoland/rev/26479b2e022a
Convert sidebar history sort menu heading to a menucaption. r=fluent-reviewers,desktop-theme-reviewers,sclements,bolsson,dao

Comment 26

[amarc]

https://hg.mozilla.org/mozilla-central/rev/eaaa92340bd3
https://hg.mozilla.org/mozilla-central/rev/35550b8a56cc
https://hg.mozilla.org/mozilla-central/rev/f4db86131ee2
https://hg.mozilla.org/mozilla-central/rev/dbafd35ead32
https://hg.mozilla.org/mozilla-central/rev/26479b2e022a