1698997 - Initial support for native context menus

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

The goal of this bug is to add basic support for native context menus, behind a pref.

This first incarnation will have bugs.

I'm planning to put it behind two prefs, actually: widget.macos.native-context-menus and browser.proton.contextmenus.enabled. The former will stay false until the biggest bugs have been ironed out and native context menus are usable on Nightly. The latter is used for MR1 pref experiments and helps evaluate the overall impact of MR1; native context menus are considered part MR1. Native context menus are only enabled if both prefs are set to true.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Ignore cancellations of popuphiding. r=harry

We fire it when the OS notifies us that the menu is already closed, so by that
time, there's nothing we can do to stop it.

Depends on D108717

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Inline OnClose into MenuClosed. r=harry

Depends on D108718

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Simplify nsMenuX state management. r=harry

We track two pieces of state: mNeedsRebuild and mIsOpen.
mNeedsRebuild is set to true on DOM mutations that require a rebuild, and set no
false when RebuildMenu() is done.
mIsOpen is set to true in MenuOpened() and set to false in MenuClosed().

Depends on D108719

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Allow OnOpen() to be called before MenuOpened(), and track the state correctly. r=harry

This also clarifies our inability to interfere with menu opening, which clashes
a little bit with the cross-platform expectations around popupshowing events, but
should hopefully not make a difference in practice.

Depends on D108720

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - When an item in a menu is clicked, make sure that the command event is fired before the popuphiding event. r=harry

We have a lot of front-end code that expects this order of events. For example,
gContextMenu is set to null in contentAreaContextMenu's popuphiding handler, and
when a command in the context menu is executed, that code operates on gContextMenu
(i.e. it assumes gContextMenu is still non-null).
Using a runnable for MenuClosedAsync() delays it just enough that menuItemHit can
run first.

Depends on D108721

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - WIP: Open context menus as native menus.

Depends on D108722

Comment 7

[Markus Stange [:mstange]]

Attachment: screenshot of content context menu with patch

Comment 8

[Markus Stange [:mstange]]

Attachment: screenshot of tab context menu with patch

Comment 9

[Markus Stange [:mstange]]

Try build (Intel)

Bugs that I'm aware of:

• Some toolbar context menus have blank items on first open (bug 1691553).
• Sometimes hover effects break after dismissing a menu.
• Sometimes clicks are ignored after dismissing a menu.
• Inside bookmarks toolbar folder dropdowns, right-clicking does not open a context menu.
• After right clicking a tab, the tab tooltip appears in front of the context menu .

Comment 10

[Phabricator Automation]

Comment on attachment 9209609 [details]
Bug 1698997 - Ignore cancellations of popuphiding. r=harry

Revision D108718 was moved to bug 1699551. Setting attachment 9209609 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9209610 [details]
Bug 1698997 - Inline OnClose into MenuClosed. r=harry

Revision D108719 was moved to bug 1699551. Setting attachment 9209610 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9209612 [details]
Bug 1698997 - Simplify nsMenuX state management. r=harry

Revision D108720 was moved to bug 1699551. Setting attachment 9209612 [details] to obsolete.

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Make it possible to listen for nsMenuX opening and closing. r=harry

Depends on D108722

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Make it possible to listen for NativeMenu opening/closing. r=harry

This will be used for native context menus. I'm planning for nsMenuPopupFrame to
be the owner of the NativeMenu, and it only wants to keep the object around
while the menu is open. So it needs to be notified when it closes.

Depends on D109177

Comment 15

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Disable the Services item in context menus for now. r=harry

Bug 660452 tracks turning this on for the appropriate menus.

Depends on D108721

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Add nsCocoaUtils::GeckoPointToCocoaPoint. r=harry

Depends on D109179

Comment 17

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Add NativeMenu::ShowAsContextMenu. r=harry

Depends on D109180

Comment 18

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Add NativeMenuSupport::CreateNativeContextMenu. r=harry

Depends on D109181

Comment 19

[Markus Stange [:mstange]]

Attachment: Bug 1698997 - Make nsXULPopupManager::ShowPopupAtScreen open a native context menu, if preffed on. r=harry,tnikkel!,NeilDeakin

This is macOS only and behind the prefs widget.macos.native-context-menus and
browser.proton.contextmenus.enabled .

The big design question here is: Where do we put the fork in the road? How much
should the existing non-native popup management machinery know about the state
of the native menu? Which parts of the non-native state should a) reflect the
true native state, b) enter a special "handled natively" state, or c) lie?

This patch picks the following approach:

• The nsMenuPopupFrame of the root menupopup knows about the native menu; it
  keeps it alive in its new mNativeMenu field.
• If the context menu has submenus, i.e. nested <menupopup> elements, the
  nsMenuPopupFrames for those nested menupopups do not know anything about the
  native menu.
• The mPopupState of natively-handled nsMenuPopupFrames is ePopupClosed.
• XULPopupElement::GetState, which queries the frame's mPopupState, also
  returns "closed". This might cause problems in the future.
• The XUL popup manager's mPopups "menu chain" does not know about any open
  native menus.
• The rollup widget also does not know about the native popup.

I've chosen to use ePopupClosed for native menus for the following reasons:

1. While mirroring / updating the state for the root menu would be doable
   without too much trouble, it would be much more annoying to do the same for
   nested menupopups of submenus. So if submenus will be ePopupClosed, it's
   consistent for the root menu to also be ePopupClosed.
2. nsXULPopupManager has assertions (for example in MayShowPopup) that make
   sure that the menu popup frame's mPopupState is consistent with the XUL
   popup manager's mPopups menu chain. Keeping the two both "closed" is the
   easiest way to achieve consistency.

Unless there are grave concerns with this approach, I suggest we go with it for
now and see what trouble arises.

Depends on D109182

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3783483587da
When an item in a menu is clicked, make sure that the command event is fired before the popuphiding event. r=harry
https://hg.mozilla.org/integration/autoland/rev/72928f201429
Make it possible to listen for nsMenuX opening and closing. r=harry
https://hg.mozilla.org/integration/autoland/rev/39473c31bf74
Make it possible to listen for NativeMenu opening/closing. r=harry
https://hg.mozilla.org/integration/autoland/rev/8ed5205576b3
Allow OnOpen() to be called before MenuOpened(), and track the state correctly. r=harry
https://hg.mozilla.org/integration/autoland/rev/fa01e8ccb917
Disable the Services item in context menus for now. r=harry
https://hg.mozilla.org/integration/autoland/rev/2ba76e86d89f
Add nsCocoaUtils::GeckoPointToCocoaPoint. r=harry
https://hg.mozilla.org/integration/autoland/rev/8dcaad49a297
Add NativeMenu::ShowAsContextMenu. r=harry
https://hg.mozilla.org/integration/autoland/rev/96dfc588c55b
Add NativeMenuSupport::CreateNativeContextMenu. r=harry
https://hg.mozilla.org/integration/autoland/rev/3398a9afe0fe
Make nsXULPopupManager::ShowPopupAtScreen open a native context menu, if preffed on. r=tnikkel

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3783483587da
https://hg.mozilla.org/mozilla-central/rev/72928f201429
https://hg.mozilla.org/mozilla-central/rev/39473c31bf74
https://hg.mozilla.org/mozilla-central/rev/8ed5205576b3
https://hg.mozilla.org/mozilla-central/rev/fa01e8ccb917
https://hg.mozilla.org/mozilla-central/rev/2ba76e86d89f
https://hg.mozilla.org/mozilla-central/rev/8dcaad49a297
https://hg.mozilla.org/mozilla-central/rev/96dfc588c55b
https://hg.mozilla.org/mozilla-central/rev/3398a9afe0fe