Closed Bug 1702041 Opened 8 months ago Closed 8 months ago

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

Categories

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

All
macOS
defect

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

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

Attachments

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

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

Pushed by mstange@themasta.com:
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
You need to log in before you can comment on or make changes to this bug.