Closed Bug 1702041 Opened 3 years ago Closed 3 years ago

popuphiding / popuphidden events can be dispatched during ShowPopup if the same native context menu is reopened


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




89 Branch
Tracking Status
firefox89 --- fixed


(Reporter: mstange, Assigned: mstange)



(Whiteboard: [proton-context-menus][mac:mr1])


(4 files)

I'm not aware of any user-facing breakage caused by this, but it is a theoretical issue that is worth fixing.

Steps to reproduce:

  1. Be on macOS, and set widget.macos.native-context-menus and browser.proton.contextmenus.enabled to true.
  2. Use a build which contains the fixes for bug 1700715.
  3. Open a menu by right-clicking the toolbar.
  4. Right-click the toolbar again, in a different place, so that the same context menu is shown elsewhere.

Expected results:
The popuphiding + popuphidden events for the old menu should be fired before the call to ShowPopup, and ideally also before the contextmenu event starts.

Actual results:
The popuphiding + popuphidden events fire during ShowPopup which is called while handling the contextmenu event.

This is more in line with how non-native popups are managed. The popup manager
knows about everything and is the source of any state changes, and it updates
the nsMenuPopupFrame when necessary.
Concretely, this change makes these two upcoming changes easier:

  • "Rolling up" native context menus. The popup manager is the nsIRollupListener.
  • Returning something sensible from nsXULPopupManager::GetLastTriggerNode while
    a native context menu is open.

Depends on D110300

This is more in line with how non-native menus operate.
In these profiles, you can see the popuphiding / popuphidden events being
dispatched before we enter the contextmenu event.
Before patch:
After patch:

Depends on D110301

This is what non-native menus do. document.popupNode is a very awkward API; it's
global state that gets updated from multiple places and which is hard to reason
For that reason, I think it's safest to stick closely to what non-native menus
are doing. I'm not aware of any user-facing breakage that this patch fixes.

Depends on D110302

Pushed by
Move NativeMenu management out of nsMenuPopupFrame and into nsXULPopupManager. r=tnikkel
Add NativeMenu::Close(). r=harry
Call NativeMenu::Close() before firing the contextmenu event. r=harry
Clear document.popupNode when a native menu closes. r=tnikkel
Blocks: 1831760
You need to log in before you can comment on or make changes to this bug.