Bugzilla

(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. 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:

```cpp
  // 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?

(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:

```cpp
  // 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?