1705842 - Support dynamic changes to native menu contents while the menu is open

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, task, P3)

Description

[Markus Stange [:mstange]]

The contents of a native (context) menu are determined by the XUL elements inside the <menupopup>. At the moment, we only synchronize these contents when the menu opens (after the popupshowing event). If any elements are added or removed while the menu is open, this won't be reflected in the native menu until the next opening.

Unfortunately, there is a WebExtension API that requires these changes to work: browser.menus.refresh()
Changing a menu while it's open does not lead to a good user experience; it can display as a flicker, give a sense of instability, and in some cases cause accidental misclicks.
Nevertheless, this API exists and there is a test for it, and supporting it with native menus is possible.

In this bug I'm going to implement support for dynamic changes of open native menus.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Add nsMenuItemX::IsVisible() and nsMenuX::IsVisible(). r=harry

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Move command registration and NSMenuItem target/action initialization into nsMenuItemX. r=harry

This means that it'll happen even if the menu item is not visible. This will make
it possible to make a menu item visible after initial menu construction.

Depends on D112447

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Make InsertChildNativeMenuItem support nsMenuItemX, too. r=harry

Depends on D112448

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Combine the two methods and properly update mVisibleItemsCount, and add some asserts. r=harry

Depends on D112449

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Allow showing and hiding items with the collapsed/hidden attributes while a menu is open. r=harry

Depends on D112450

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Make sure that items that are un-hidden while the menu is open get their icon set up. r=harry

Before, we were only calling SetUpIcon if the item was visible during menu
construction on menu opening, but not if the item became visible after menu
opening.

Depends on D112451

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Change internal visibility before calling MenuChildChangedVisibility. r=harry

This will make a difference once code inside MenuChildChangedVisibility starts
calling IsVisible(). The method name has "Changed" in it, so it should be called
after the change.

Depends on D112452

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Remove unnecessary null check and submenu assignment. r=harry

Depends on D112453

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Create generic CreateMenuChild and AddMenuChild methods. r=harry

Depends on D112454

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Add WillInsertChild and WillRemoveChild. r=harry

Depends on D112455

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Support inserting and removing menu elements while the menu is open. r=harry

This makes the WebExtension API menus.refresh() work.

Depends on D112456

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1705842 - Allow opening empty menus, by putting a placeholder item in place. r=harry

This is exercised by one of the subtests of
browser/components/extensions/test/browser/browser_ext_menus_replace_menu.js :

The menu is opened while it's empty, and then the content items are added
asynchronously while the menu is open.

If we try to open an NSMenu with zero NSMenuItems, macOS immediately closes the
menu after opening it. With a placeholder item it stays open as expected.

Depends on D112457

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17312473a6d5
Add nsMenuItemX::IsVisible() and nsMenuX::IsVisible(). r=harry
https://hg.mozilla.org/integration/autoland/rev/04c8316f671f
Move command registration and NSMenuItem target/action initialization into nsMenuItemX. r=harry
https://hg.mozilla.org/integration/autoland/rev/e6fe584b0c8c
Make InsertChildNativeMenuItem support nsMenuItemX, too. r=harry
https://hg.mozilla.org/integration/autoland/rev/bff1875da8b0
Combine the two methods and properly update mVisibleItemsCount, and add some asserts. r=harry
https://hg.mozilla.org/integration/autoland/rev/359a257f20b1
Allow showing and hiding items with the collapsed/hidden attributes while a menu is open. r=harry
https://hg.mozilla.org/integration/autoland/rev/6c46f730e7bf
Make sure that items that are un-hidden while the menu is open get their icon set up. r=harry
https://hg.mozilla.org/integration/autoland/rev/2cd75d5e7509
Change internal visibility before calling MenuChildChangedVisibility. r=harry
https://hg.mozilla.org/integration/autoland/rev/9445397a5ab5
Remove unnecessary null check and submenu assignment. r=harry
https://hg.mozilla.org/integration/autoland/rev/b7186c85ec33
Create generic CreateMenuChild and AddMenuChild methods. r=harry
https://hg.mozilla.org/integration/autoland/rev/d0b9c629a807
Add WillInsertChild and WillRemoveChild. r=harry
https://hg.mozilla.org/integration/autoland/rev/a144273d02b0
Support inserting and removing menu elements while the menu is open. r=harry
https://hg.mozilla.org/integration/autoland/rev/a7e2ed98d5e1
Allow opening empty menus, by putting a placeholder item in place. r=harry

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/17312473a6d5
https://hg.mozilla.org/mozilla-central/rev/04c8316f671f
https://hg.mozilla.org/mozilla-central/rev/e6fe584b0c8c
https://hg.mozilla.org/mozilla-central/rev/bff1875da8b0
https://hg.mozilla.org/mozilla-central/rev/359a257f20b1
https://hg.mozilla.org/mozilla-central/rev/6c46f730e7bf
https://hg.mozilla.org/mozilla-central/rev/2cd75d5e7509
https://hg.mozilla.org/mozilla-central/rev/9445397a5ab5
https://hg.mozilla.org/mozilla-central/rev/b7186c85ec33
https://hg.mozilla.org/mozilla-central/rev/d0b9c629a807
https://hg.mozilla.org/mozilla-central/rev/a144273d02b0
https://hg.mozilla.org/mozilla-central/rev/a7e2ed98d5e1

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Hey mstange, can we presume this is part of the set of patches that need to have beta uplift requested for MR1?

Comment 17

[Markus Stange [:mstange]]

Definitely. The current plan is to uplift all native menu fixes in one big chunk in bug 1700679, next Monday morning (European time).

Comment 18

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/764d560ff23a
https://hg.mozilla.org/releases/mozilla-beta/rev/b9ca957e4eee
https://hg.mozilla.org/releases/mozilla-beta/rev/da94c8b32bc7
https://hg.mozilla.org/releases/mozilla-beta/rev/8d54b54c7c68
https://hg.mozilla.org/releases/mozilla-beta/rev/bfd76aa40aac
https://hg.mozilla.org/releases/mozilla-beta/rev/1bfa71e9eceb
https://hg.mozilla.org/releases/mozilla-beta/rev/6b7a0d5f582c
https://hg.mozilla.org/releases/mozilla-beta/rev/b28c07d949fc
https://hg.mozilla.org/releases/mozilla-beta/rev/445ea75af7fe
https://hg.mozilla.org/releases/mozilla-beta/rev/ffee17049404
https://hg.mozilla.org/releases/mozilla-beta/rev/18136992ec9d
https://hg.mozilla.org/releases/mozilla-beta/rev/63e72af0f6e2