Initial support for native context menus
Categories
(Core :: Widget: Cocoa, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox89 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
References
(Regressed 1 open bug)
Details
(Whiteboard: [proton-context-menus][mac:mr1])
Attachments
(11 files, 4 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
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
Assignee | ||
Comment 2•4 years ago
|
||
Depends on D108718
Assignee | ||
Comment 3•4 years ago
|
||
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
Assignee | ||
Comment 4•4 years ago
|
||
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
Assignee | ||
Comment 5•4 years ago
|
||
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
Assignee | ||
Comment 6•4 years ago
|
||
Depends on D108722
Assignee | ||
Comment 7•4 years ago
|
||
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
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 .
Updated•4 years ago
|
Comment 10•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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.
Assignee | ||
Comment 13•4 years ago
|
||
Depends on D108722
Assignee | ||
Comment 14•4 years ago
|
||
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
Assignee | ||
Comment 15•4 years ago
|
||
Bug 660452 tracks turning this on for the appropriate menus.
Depends on D108721
Assignee | ||
Comment 16•4 years ago
|
||
Depends on D109179
Assignee | ||
Comment 17•4 years ago
|
||
Depends on D109180
Assignee | ||
Comment 18•4 years ago
|
||
Depends on D109181
Assignee | ||
Comment 19•4 years ago
|
||
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:
- 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. - 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
Updated•4 years ago
|
Comment 20•4 years ago
|
||
Comment 21•4 years ago
|
||
bugherder |
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
Updated•2 years ago
|
Description
•