1704206 - Add an API for activating menu items that doesn't require synthesizing events

Status: RESOLVED FIXED

(Core :: XUL, task)

Description

[Markus Stange [:mstange]]

On macOS, native context menus do not respond to synthesized events, such as mousemove, click, or key presses. However, we have many automated tests which use synthesized events to interact with context menus.
In order to make these tests work (bug 1700724), we need to have APIs to do the following:

1. Activate a menuitem in an open context menu
2. Activate a menuitem in an open context menu with some key modifiers
3. Open a submenu of a context menu

Our current tests currently use EventUtils.synthesizeMouseAtCenter for all three; opening a submenu is often done via a synthesized mousemove event. Some tests use menuitem.click() for 1.

I propose two one new APIs: (Edited after comment 13)

1. XULPopupElement.activateItem(menuItem, modifiers) for activating menu items.
2. XULMenuElement.openSubmenu() for opening submenus.

(Edit:) For opening submenus, we already have menu.openMenu(true) which we can make work for native menus as well.

Moreover, if no key modifiers are needed, menuitem.click() can still be used for activating items (but not for opening submenus).

The intention of these APIs is that they will work on both native and non-native menus.

WebIDL:

dictionary ActivateMenuItemModifiers {
  boolean altKey = false;
  boolean metaKey = false;
  boolean ctrlKey = false;
  boolean shiftKey = false;
};

[ChromeOnly,
 Exposed=Window]
interface XULPopupElement : XULElement
{
  /**
   * Activate the item itemElement. This is the recommended way to "click" a
   * menuitem in automated tests that involve menus.
   * Fires the command event for the item and then closes the menu.
   * Only call this while this popup is open.
   *
   * @param itemElement The activated menuitem.
   * @param modifierKeys Which modifier keys should be set on the command event.
   */
  void activateItem(Element? itemElement,
                    optional ActivateMenuItemModifiers modifierKeys = {});
};

[ChromeOnly,
 Exposed=Window]
interface XULMenuElement : XULElement {
  /**
   * Open this menu's menupopup as a submenu of a currently open menu.
   * Should only be called if this is a <menu> inside an open <menupopup>.
   */
  void openSubmenu();
};

For openSubmenu(), alternatively we could also make it a method on XULPopupElement so that it's called as popup.openSubmenu(menu). I'm not sure which one is preferable.

In this bug I'm planning to implement these two APIs and make them work for non-native menus.
In bug 1704209 and bug 1704212, I'm going to implement them for native context menus as well.
And in other follow-up bugs (tracked by bug 1704211), I'm planning to convert our existing tests to the new APIs.

I'm interested in feedback on these APIs. I'm going to go ahead with uploading patches that implement them as outlined above, because anything is better than nothing, and then I can address any non-blocking feedback in follow-ups.

Comment 1

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

This looks like a good idea. I wonder if we have any tests that try to key around inside of menus? If so, would we want to add support for sending native key events to the menus?

Comment 2

[Markus Stange [:mstange]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #1)

I wonder if we have any tests that try to key around inside of menus? If so, would we want to add support for sending native key events to the menus?

I don't think sending key events to native menus is something we can support. Furthermore, there shouldn't be a need to test key events on native context menus - the key events are fully handled by the system and we don't even see them, so we also can't screw anything up with them.
I've seen a few tests which synthesize the ESC key to close menus; those can call hidePopup() instead. I haven't yet run into tests which have a good reason to use the arrow keys to change which item is highlighted; there's this test which uses arrow keys, but only to make sure that the tab bar doesn't handle them while the menu is open, and that's a given with native menus.

Comment 3

[Neil Deakin]

1. XULPopupElement.activateItem(menuItem, modifiers) for activating menu items.

The term 'activate' in the menu code means 'highlight the menu but don't trigger the command' so this seems an unfortunate name, although I can't think of a better one. This is only exposed as 'active' via the menuactive attribute, DOMMenuItemActive and related events though, so maybe we should just change our internal code to use a different name instead.

2. XULMenuElement.openSubmenu() for opening submenus.

How would this be different than the existing openMenu method? That could just detect that the element was in a submenu could it not?

Comment 4

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #3)

2. XULMenuElement.openSubmenu() for opening submenus.

How would this be different than the existing openMenu method? That could just detect that the element was in a submenu could it not?

Good idea! I will give that a try.

Comment 5

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #3)

1. XULPopupElement.activateItem(menuItem, modifiers) for activating menu items.

The term 'activate' in the menu code means 'highlight the menu but don't trigger the command' so this seems an unfortunate name, although I can't think of a better one. This is only exposed as 'active' via the menuactive attribute, DOMMenuItemActive and related events though, so maybe we should just change our internal code to use a different name instead.

I have filed bug 1704223 about this.

Comment 6

[Neil Deakin]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #1)

This looks like a good idea. I wonder if we have any tests that try to key around inside of menus? If so, would we want to add support for sending native key events to the menus?

We have lots of tests that navigate menus with keys in toolkit/content/tests/chrome that do this, for example test_popup_attribute.xhtml
The tests do other tests of menus and popups as well, such as verify positioning. Would any of these tests apply to native menus?

Comment 7

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #6)\

We have lots of tests that navigate menus with keys in toolkit/content/tests/chrome that do this, for example test_popup_attribute.xhtml
The tests do other tests of menus and popups as well, such as verify positioning. Would any of these tests apply to native menus?

The parts of the test that act on context menus will not apply. We'll need to skip those parts of the tests. We can write equivalent tests for native menus (bug 1700727) which test the aspects that we can influence and observe.
But for now, we will still be using non-native menus for anchored popups, so those parts of the test we will want to continue to run.

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1704206 - Simplify nsMenuFrame::Execute. r=tnikkel

Depends on D111502

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1704206 - Use mozilla::Modifiers in nsXULMenuCommandEvent. r=tnikkel

Depends on D111515

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1704206 - Make it possible to call CreateMenuCommandEvent without an event. r=tnikkel

Depends on D111516

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1704206 - Add nsMenuFrame::ActivateItem(). r=tnikkel

Depends on D111517

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1704206 - Add XULPopupElement.activateItem as a way to activate menu items. r=tnikkel

Depends on D111518

Comment 13

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #4)

(In reply to Neil Deakin from comment #3)

2. XULMenuElement.openSubmenu() for opening submenus.

How would this be different than the existing openMenu method? That could just detect that the element was in a submenu could it not?

Good idea! I will give that a try.

This works great. There's no need for a new openSubmenu API.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c836358e2c3c
Simplify nsMenuFrame::Execute. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/c74ba1a1ce88
Use mozilla::Modifiers in nsXULMenuCommandEvent. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2103a5f1fb3e
Make it possible to call CreateMenuCommandEvent without an event. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/f1166f6eacc1
Add nsMenuFrame::ActivateItem(). r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e710553e8c1d
Add XULPopupElement.activateItem as a way to activate menu items. r=tnikkel,emilio

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c836358e2c3c
https://hg.mozilla.org/mozilla-central/rev/c74ba1a1ce88
https://hg.mozilla.org/mozilla-central/rev/2103a5f1fb3e
https://hg.mozilla.org/mozilla-central/rev/f1166f6eacc1
https://hg.mozilla.org/mozilla-central/rev/e710553e8c1d