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

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1702041 - Move NativeMenu management out of nsMenuPopupFrame and into nsXULPopupManager. r=tnikkel

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.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1702041 - Add NativeMenu::Close(). r=harry

Depends on D110300

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1702041 - Call NativeMenu::Close() before firing the contextmenu event. r=harry

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: https://share.firefox.dev/39pbqbb
After patch: https://share.firefox.dev/2PCXx1Z

Depends on D110301

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1702041 - Clear document.popupNode when a native menu closes. r=tnikkel

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
about.
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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/464623cbe454
Move NativeMenu management out of nsMenuPopupFrame and into nsXULPopupManager. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2c0701070921
Add NativeMenu::Close(). r=harry
https://hg.mozilla.org/integration/autoland/rev/49cacd727163
Call NativeMenu::Close() before firing the contextmenu event. r=harry
https://hg.mozilla.org/integration/autoland/rev/fb5ac5bc8ec9
Clear document.popupNode when a native menu closes. r=tnikkel

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/464623cbe454
https://hg.mozilla.org/mozilla-central/rev/2c0701070921
https://hg.mozilla.org/mozilla-central/rev/49cacd727163
https://hg.mozilla.org/mozilla-central/rev/fb5ac5bc8ec9