1705120 - Selecting an item in a native menu on top of a non-native menu does not close the non-native menu

Status: VERIFIED FIXED

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

Description

[Harry Twyford]

STR:

1. Set widget.macos.native-context-menus to true.
2. Open a folder in the bookmarks toolbar. Right click on one of the bookmarks.
3. Click Open in New Tab.

Expected: The native menu closes, the bookmarks toolbar folder menu closes, and the link opens in a new tab.

Actual: The native menu closes and the link opens in a new tab. The bookmarks toolbar folder menu remains open.

This is causing browser/components/enterprisepolicies/tests/browser/managedbookmarks/browser_policy_managedbookmarks.js to fail. Once the synthesized clicks on the context menu in that test are replaced with activateItem, the test still fails. The first part of the test selects Open in New Tab in the native context menu. The next part wants to test something else, so it reopens the bookmarks folder menu and waits for the folder menu's popupshowing event. Since the event never fires (since the folder menu is still open), the test hangs.

Comment 1

[:Gijs (he/him)]

Is this still happening?

Comment 2

[Markus Stange [:mstange]]

Yes it is.

Comment 3

[Markus Stange [:mstange]]

I consider this rather low priority because menus-on-menus is a rather fringe interaction and this bug has a super easy workaround: After choosing a menu item (which closes the native menu), the next click will close the non-native menu.
It is however a regression so I'm planning to look at it once the other regressions are fixed.

The fix will probably involve changing nsXULPopupManager::OnNativeMenuClosed to hide the rest of the "popup chain".

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Avoid spurious accessibility notifications from the Dock menu. r=Gijs!,morgan

macOS calls -[MacApplicationDelegate applicationDockMenu:] shortly after startup.
We fire popupshowing/shown/hiding/hidden events for the Dock menu when this
happens, mostly for consistency with other menus. Accessibility code was
processing these events and turning them into unintended accessibility notifications.

The Dock menu is a "native menu", but it is not covered by the workaround from
bug 1703482 because it's not a native context menu. It's its own special thing
and makes use of nsIMacDockSupport and nsIStandaloneNativeMenu and some JS glue
code.

Prior to the "Simplify nsMenuX state management" patch from bug 1699551, we were
not firing the popup DOM events for the Dock menu, mostly by accident.
In fact, it looks like we were only ever firing the events if a DOM modification
had occurred in the menu between menu construction and the current menu opening.
That's definitely not what we want and I'm surprised we got away with it for so
long.

Comment 5

[Phabricator Automation]

Comment on attachment 9218807 [details]
Bug 1705120 - Avoid spurious accessibility notifications from the Dock menu. r=Gijs!,morgan

Revision D113599 was moved to bug 1706337. Setting attachment 9218807 [details] to obsolete.

Comment 6

[Julien Cristau [:jcristau]]

There's an uplift request for the regressing bug.

Comment 7

[Emil Ghitta, QA [:emilghitta]]

Attachment: Screenshot 2021-05-04 at 12.14.03.png

Encountered something (maybe) similar to this bug.

It seems that the "Edit Bookmark", "Add Bookmark" & "Add bookmark folder" modals are opened in background if those context menu options are clicked on a bookmark which is placed withing a bookmark folder or via "Show more bookmarks" menu.

Comment 8

[:Gijs (he/him)]

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

I consider this rather low priority because menus-on-menus is a rather fringe interaction and this bug has a super easy workaround: After choosing a menu item (which closes the native menu), the next click will close the non-native menu.

I sort of agree, but using the context menu on bookmarks items isn't that fringe, and this makes this a little confusing/tedious/broken-seeming.

The fix will probably involve changing nsXULPopupManager::OnNativeMenuClosed to hide the rest of the "popup chain".

I looked into this, but I got a bit muddled. I don't think we want this behaviour whenever the menu is closed - just when it is closed as a result of executing a command - hitting e.g. [esc] should close one level of panels/menus at a time. I investigated why this works for the non-native case, and this appears to be because we invoke the XUL popup manager's ExecuteMenu method at https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/layout/xul/nsXULPopupManager.cpp#1432 . This collects all the parents, irrespective of type, while obeying the closemenu attributes where present, and closes all of them.

There is also a helpful comment there:

  // When a menuitem is selected to be executed, first hide all the open
  // popups, but don't remove them yet. This is needed when a menu command
  // opens a modal dialog. The views associated with the popups needed to be
  // hidden and the accesibility events fired before the command executes, but
  // the popuphiding/popuphidden events are fired afterwards.

which explains why the breakage from comment #7 with modals occurs, ie we keep the menus open when the modal opens.

I'm guessing we want to factor out the closing logic into a helper and invoke it when an item in a native menu is executed/run. I'm also guessing we want to do this shortly afterwards in the file, ie in https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/layout/xul/nsXULPopupManager.cpp#1487 , ActivateNativeMenuItem. What I'm not sure about is why we haven't done that yet - is there a particular reason? We ran into this issue with some automated tests (ie the menus wouldn't close and this broke the test which assumed that activating menu items also closed the menu in question), so I'm not sure if there was a deliberate choice to not do the same thing here that had reasons that I'm not aware of?

Comment 9

[Markus Stange [:mstange]]

Thanks a lot for the analysis!

(In reply to :Gijs (he/him) from comment #8)

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

The fix will probably involve changing nsXULPopupManager::OnNativeMenuClosed to hide the rest of the "popup chain".

I looked into this, but I got a bit muddled. I don't think we want this behaviour whenever the menu is closed - just when it is closed as a result of executing a command - hitting e.g. [esc] should close one level of panels/menus at a time.

Hmm, I see. With native menus, in both of these cases, it's the system which closes the menu, and it tells our menu delegate about the fact that the menu is closing but not whether it's closing because of an activated item or because the menu was canceled. Similarly, our OnNativeMenuClosed listener method does not indicate whether the menu closed because of an activated item or because [esc] was pressed. So maybe we need to start by detecting this difference.

I investigated why this works for the non-native case, and this appears to be because we invoke the XUL popup manager's ExecuteMenu method at https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/layout/xul/nsXULPopupManager.cpp#1432 . This collects all the parents, irrespective of type, while obeying the closemenu attributes where present, and closes all of them.

(well, not completely irrespective of type - there is an item->IsMenu() check in there.)

There is also a helpful comment there:

  // When a menuitem is selected to be executed, first hide all the open
  // popups, but don't remove them yet. This is needed when a menu command
  // opens a modal dialog. The views associated with the popups needed to be
  // hidden and the accesibility events fired before the command executes, but
  // the popuphiding/popuphidden events are fired afterwards.

which explains why the breakage from comment #7 with modals occurs, ie we keep the menus open when the modal opens.

This is indeed helpful!

So the call to HidePopupsInList will need to happen before we run the command event. Maybe we'll need to add another notification method for this, e.g. OnNativeMenuWillActivateItem.

As the comment says, HidePopupsInList only makes the popups in the parent chain invisible but doesn't fully close them. The actual closing happens when the nsXULMenuCommandEvent runs. That event is dispatched to the event loop during nsXULPopupManager::ExecuteMenu; its Run() method first dispatches the command event and then calls HidePopup.

I'm guessing we want to factor out the closing logic into a helper and invoke it when an item in a native menu is executed/run.

Yes, but I don't think we can have a single "ExecuteAndClose" helper - the individual shared pieces may need to be more fragmented than that. Furthermore, if an item is activated by the user, we'll still want to leave it up to the system to close the native menu - the system closes the menu with a fade-out animation, and if we additionally called NativeMenuMac::Close we would probably interrupt that animation because Close calls cancelTrackingWithoutAnimation. And I don't want to have code that makes it look like it's our responsibility to close the native menu when a menu item is clicked, because it's not - we literally cannot do anything to stop the menu from closing when an item is clicked.

I'm also guessing we want to do this shortly afterwards in the file, ie in https://searchfox.org/mozilla-central/rev/2f109387cc6886859d3f985ed9aca352fff653b8/layout/xul/nsXULPopupManager.cpp#1487 , ActivateNativeMenuItem. What I'm not sure about is why we haven't done that yet - is there a particular reason?

ActivateNativeMenuItem does close the menu - it does so from NativeMenuMac::ActivateItem.

We ran into this issue with some automated tests (ie the menus wouldn't close and this broke the test which assumed that activating menu items also closed the menu in question),

No, if the test was actually calling activateItem then the menu would close. We did have some bugs in earlier patches where calling activateItem and then immediately opening another menu would result in broken state, but those bugs are fixed. I think you may be thinking of cases where the test was calling menuitem.click() instead of activateItem(). And click() never closes the menu, neither with native nor with non-native menus. In those cases, the menus in question would later close because something else caused them to close in the non-native case. I've made some changes to native menu handling since then which would cause native menus to be closed in most of those cases, for example when a window is opened / closed (bug 1705838) or when another menu is opened (bug 1706816). I think one remaining difference might be if a parent element of the menu is removed from the DOM or made display:none, for example when the devtools panel disappears while a devtools menu is open, or the browser sidebar is closed while a sidebar menu is open - I think in those cases the native menu will still stay open whereas the non-native menu would have closed.

So, to sum up:

• When a native menu item is clicked, we should call HidePopupsInList before running the command event.
• When a native menu item is clicked, we should call HidePopups for the remaining open menus after the native menu has closed - but not if the native menu is closing because it was dismissed without menuitem activation.

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Make it clear that aSender is an NSMenuItem. r=harry

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Add -[MenuDelegate menu:willActivateItem:] and call it before activating an item. r=harry

Depends on D114432

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Notify nsMenuX observers when a menu item is about to be activated, and supply the activated DOM element. r=harry

Depends on D114433

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Add OnNativeMenuWillActivateItem observer method for NativeMenu::Observer and call it at the right times. r=harry

Depends on D114434

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Track whether a menu is closing because an item was activated. r=harry

Depends on D114435

Comment 15

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Propagate up the menu tree whether the menu is closing because an item was activated. r=harry

Depends on D114436

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Add an aItemWasActivated argument to OnNative(Sub)MenuClosed observer methods. r=harry

Depends on D114437

Comment 17

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Factor out part of this function into a new function called GetCloseMenuMode(). r=harry

Depends on D114438

Comment 18

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - Factor out another part of this function into a new function called HideOpenMenusBeforeExecutingMenu(). r=harry

Depends on D114439

Comment 19

[Markus Stange [:mstange]]

Attachment: Bug 1705120 - When a menu item in a native menu is activated, hide and close any non-native menus at the right times. r=harry

Depends on D114440

Comment 20

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=c0c890b8999ff8eb1eec3417bfa51638fff47cdb

Comment 21

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=e8e8ff143bd0750bfcc85b12cad72dc6eea8fc8d

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e96357d71f81
Make it clear that aSender is an NSMenuItem. r=harry
https://hg.mozilla.org/integration/autoland/rev/b3d9270b6278
Add -[MenuDelegate menu:willActivateItem:] and call it before activating an item. r=harry
https://hg.mozilla.org/integration/autoland/rev/472f5b20d585
Notify nsMenuX observers when a menu item is about to be activated, and supply the activated DOM element. r=harry
https://hg.mozilla.org/integration/autoland/rev/9cdef0e5da97
Add OnNativeMenuWillActivateItem observer method for NativeMenu::Observer and call it at the right times. r=harry
https://hg.mozilla.org/integration/autoland/rev/cf3cbceb542c
Factor out part of this function into a new function called GetCloseMenuMode(). r=harry
https://hg.mozilla.org/integration/autoland/rev/870a6eab8f8d
Factor out another part of this function into a new function called HideOpenMenusBeforeExecutingMenu(). r=harry
https://hg.mozilla.org/integration/autoland/rev/b33af6515afa
When a menu item in a native menu is activated, hide and close any non-native menus at the right times. r=harry

Comment 23

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e96357d71f81
https://hg.mozilla.org/mozilla-central/rev/b3d9270b6278
https://hg.mozilla.org/mozilla-central/rev/472f5b20d585
https://hg.mozilla.org/mozilla-central/rev/9cdef0e5da97
https://hg.mozilla.org/mozilla-central/rev/cf3cbceb542c
https://hg.mozilla.org/mozilla-central/rev/870a6eab8f8d
https://hg.mozilla.org/mozilla-central/rev/b33af6515afa

Comment 24

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

The patch landed in nightly and beta is affected.
:mstange, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 25

[Emil Ghitta, QA [:emilghitta]]

This is verfied fixed using Firefox 90.0a1 (BuildId:20210509213623) on macOS 10.14. Both the issue mentioned in comment 0 & in comment 7 are no longer reproducible.

Comment 26

[Markus Stange [:mstange]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #24)

:mstange, is this bug important enough to require an uplift?

The patches are not entirely risk-free, so I'd say no. I'm happy to be convinced otherwise though.

Comment 27

[Markus Stange [:mstange]]

Actually, the scenario in comment 7 is indeed rather broken-seeming, so let's try to uplift this after all.

Comment 28

[Shilpa Mohanty]

Request for Uplift from the PM team

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 9220502 [details]
Bug 1705120 - Make it clear that aSender is an NSMenuItem. r=harry

Beta/Release Uplift Approval Request

• User impact if declined: If the user chooses "Edit bookmark" in the context menu of a bookmark inside a bookmarks folder in the bookmarks toolbar, the modal dialog is covered by the bookmarks folder popup. This is a regression compared to 88. The user needs to click another time to dismiss the popup.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Most of the patches just put a new API in place, and the final patch only hides and closes non-native menus in the appropriate cases, and can't really affect anything else. Menu closing has extremely good test coverage.
• String changes made/needed:

Comment 30

[Pascal Chevrel:pascalc]

Markus, the patches don't graft cleanly on beta as they are on top of bug 1704102 which was marked as wontfix for 89. Do you need to uplift 1704102 as well or can you update the patches for beta. If you intend to also take bug 1704102, does it change your risk evaluation for an uplift? Thanks

Comment 31

[Markus Stange [:mstange]]

(In reply to Pascal Chevrel:pascalc from comment #30)

Markus, the patches don't graft cleanly on beta as they are on top of bug 1704102

Oh, right.

which was marked as wontfix for 89. Do you need to uplift 1704102 as well or can you update the patches for beta. If you intend to also take bug 1704102, does it change your risk evaluation for an uplift? Thanks

I'm not sure what's the best course of action. I can rebase the patch or we can uplift both bugs. I'm fine with either. Uplifting bug 1704102 adds a small amount of risk but not very much.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9220502 [details]
Bug 1705120 - Make it clear that aSender is an NSMenuItem. r=harry

Approved for 89.0b12.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/879cfc0a86ab
https://hg.mozilla.org/releases/mozilla-beta/rev/a0041d86cc14
https://hg.mozilla.org/releases/mozilla-beta/rev/24c671770b95
https://hg.mozilla.org/releases/mozilla-beta/rev/36caf75fb240
https://hg.mozilla.org/releases/mozilla-beta/rev/58dbfae51172
https://hg.mozilla.org/releases/mozilla-beta/rev/84a38b77f3ac
https://hg.mozilla.org/releases/mozilla-beta/rev/310d7fce2d0e

Comment 34

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 89.0b12 (BuildId:20210513185752 ) on macOS 11.3