1257613 - Build Menu API to build and show context menus from HTML tools in the toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

As we convert more tools to HTML they will need a way to open context menus.  We don't have a way to open or modify these without using a XUL <menupopup>, <commandset>, etc.

The toolbox needs to provide some way for tools to create, modify, and open context menu popups

Comment 1

[Brian Grinstead [:bgrins]]

OK, so the basic problem is that menus *require* a XUL doc but we want to be able to build the toolbox in HTML.  One approach would be:

1) Always keep a top level toolbox-wrapper.xul file that we can install menus into
2) Let tools use the HTML 5 context menu format in their markup: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu
3) The toolbox crawls the DOM of all tools and more-or-less converts the <menu> element into a <menupopup> element
3a) The toolbox also needs to detect mutations on menus to see if the page changes attributes on the items, injects new items, removes them, etc and keep the xul menus in sync
3b) The toolbox also needs to detect mutations on the tool to see if new menus are added or old ones are removed and keep the xul menus in sync
4) Toolbox binds 'contextmenu' event onto elements in tools that have a [menu] attribute, and programmatically opens the corresponding XUL menu when that happens
5) When a command event fires on one of the XUL menu items, trigger the associated 'click' event on the HTML menu item

Things I don't know about yet:

1) How do we deal with "overlays" for things like shared edit menu elements: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Overlays
2) What if tools use a shared ID for menus.. how we we avoid ID conflicts on the root level XUL wrapper

One of the advantages to this approach is that the menu would still 'work' when hosting the toolbox in a tab (although the menu items would be surrounded by the normal browser menu items).  It's also an easier migration strategy for most of the tools since they are already using a similar markup-based approach.  One of the disadvantages is it's pretty tied into the DOM and it's keeping a lot of stuff in sync across frames.

Comment 2

[Brian Grinstead [:bgrins]]

An alternate approach:

Implement a toolbox helper function for `showContextMenuAtPoint` or similar, where the arguments to that function include an array of context menu items with similar metadata as the menu items have (label, onClick, disabled).

When this function is called, generate a XUL menu on the fly and trigger the popup to open.  When a XUL menu item is selected, trigger the corresponding onClick handler that was passed into the helper.

I haven't fully finished thinking this one out.  It's less DOM-reliant and might make complex behaviors easier since the tool needs to pass in the whole set of actions.  Although it will be a harder migration path, and every time a menu is opened the tool will need to pass in all actions.

Comment 3

[Patrick Brosset <:pbro>]

The toolbox-wrapper.xul sounds like a pretty good idea to me. That way, when the toolbox is effectively in a XUL host, we can keep on using native contextual menus.

Now, when the toolbox is in a HTML host (in a tab), we can't do that. HTML5 menus are really bad, and I guess we don't know what we'll end up using in the future. So we should abstract the creation of menus as much as possible from using DOM elements. Which is why I prefer your alternate approach (comment 2).

Couldn't we have an easier migration path if instead of a function (showContextMenuAtPoint), we had an object that mimicked (to some degree) the XUL menu's API?

Comment 4

[Brian Grinstead [:bgrins]]

Those are good points Patrick.  I think we should proceed with the alternate approach and see what happens.  We can load these dynamic menus in toolbox.xul for now (and eventually move up to the wrapper when needed).

Comment 5

[Alexandre Poirot [:ochameau]]

We should look into mozbrowser API. There is a mechanism to handle context-menus.

http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js#242
https://mxr.mozilla.org/gaia/source/apps/system/js/browser_context_menu.js#41

I imagine that would allow us to use regular context menu APIs, with a very small chrome bit on top of toolbox.xul in order to show the context menu without any browser things.

Comment 6

[Brian Grinstead [:bgrins]]

I'm going to try to use the approach in Comment 2, but matching a subset of the Menu API provided by Electron: https://github.com/electron/electron/blob/master/docs/api/menu.md

Comment 7

[Brian Grinstead [:bgrins]]

Will need to be careful with running into conflicts on Bug 1211613 which is reorganizing the current inspector markup view menu

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: context-menu-api-WIP-1.patch

First WIP, includes an incomplete migration of the inspector menu to the new API

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1257613 - Add an API to open context menus from an HTML document; f=jdescottes

Review commit: https://reviewboard.mozilla.org/r/49361/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49361/

Comment 10

[Brian Grinstead [:bgrins]]

Note: this is really a feedback request - I still have to add tests and more docs.  Going to attach a temp conversion patch for the inspector just to make it easier to check out

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: use-menu-api-inspector-WIP.patch

Just a WIP. The inspector menu is going to change in Bug 1211613, and this patch will eventually move to Bug 1266478

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8746308 [details]
MozReview Request: Bug 1257613 - Add an API to open context menus from an HTML document; f=jdescottes

https://reviewboard.mozilla.org/r/49361/#review46259

Thanks Brian, looks like a good start. The main thing I'd like to clarify are the semantics of the popup() method.

::: devtools/client/framework/menu.js:15
(Diff revision 1)
> +  this.menuitems = [];
> +  this.id = id;
> +}
> +
> +Menu.prototype.append = function(menuitem) {
> +  // TODO: do we want to support appending a plain object?

Would be nice, but if the goal is to match Electron's API, let's restrict to instances of MenuItem for now.

::: devtools/client/framework/menu.js:40
(Diff revision 1)
> +      let menusep = doc.createElement("menuseparator");
> +      parent.appendChild(menusep);
> +    } else {
> +      let menuitem = doc.createElement("menuitem");
> +      menuitem.setAttribute("label", item.label);
> +      menuitem.addEventListener("command", () => {

Should remove this event listener when the popup is remove?

(our codebase is not consistent on this topic, we sometimes remove the event, sometimes don't so I might be wrong here)

::: devtools/client/framework/menu.js:50
(Diff revision 1)
> +        menuitem.setAttribute("type", "checkbox");
> +      }
> +      if (item.disabled) {
> +        menuitem.setAttribute("disabled", "true");
> +      }
> +      if (item.accesskey) {

Not a big deal, but accesskey, selection and selectionType are not defined in MenuItem for now.

::: devtools/client/framework/menu.js:68
(Diff revision 1)
> +      parent.appendChild(menuitem);
> +    }
> +  });
> +};
> +
> +Menu.prototype.popup = function(win, x, y) {

Electron supports a "window" parameter, but does it make sense for us? In electron's API, they have to handle applications with multiple windows, so this parameter makes sense. For devtools, context menus will always be displayed in the devtools window. Here this parameter is more a technical requirement: we need a XUL document to create popups.

I guess "win" should always be the topmost devtools window? In the end it will be the only window with a XUL document. If it's true, does it make sense to force every caller to pass this argument? 

If we keep this argument we should make clear that this is not about "where should this context menu be displayed", but rather "where can I get a XUL document to create popup elements".

::: devtools/client/framework/menu.js:70
(Diff revision 1)
> +  });
> +};
> +
> +Menu.prototype.popup = function(win, x, y) {
> +  let doc = win.document;
> +  let popup = doc.createElement("menupopup");

Is popup supposed to create context menus or only display them? 

Calling several times popup() for the same Menu instance will display several popups with the same id. Should we allow this? I guess in most cases this is not an issue because the previous context menu should have been dismissed before opening a new one, but I still have to raise the point.

Recreating a popup everytime is nice because it simplifies a lot of use cases: adding/removing menu items, setting items as disabled etc. No longer have to "update" existing elements. So I definitely think this is a nice approach, we might just want to make sure only one popup can be displayed at a time.

::: devtools/client/framework/menu.js:77
(Diff revision 1)
> +    popup.id = this.id;
> +  }
> +  this._createMenuItems(popup);
> +
> +  // Remove the menu from the DOM once it's hidden
> +  popup.addEventListener("popuphidden", (e) => {

This event listener should be removed?

::: devtools/client/framework/menu.js:85
(Diff revision 1)
> +    }
> +  }, true);
> +
> +  doc.querySelector("popupset").appendChild(popup);
> +  popup.openPopupAtScreen(x, y, true);
> +  popup.focus();

Is this focus() really needed, the popupmenu seems to get the focus when opened by default.

::: devtools/client/framework/menu.js:87
(Diff revision 1)
> +
> +  doc.querySelector("popupset").appendChild(popup);
> +  popup.openPopupAtScreen(x, y, true);
> +  popup.focus();
> +
> +  return popup;

Should we really return and expose the xul "menupopup" ? Even if we have to use XUL in this implementation, we should not encourage callers to rely on XUL elements.

Comment 13

[Julian Descottes [:jdescottes]]

Some additional comments.

#1: In the inspector context menu, we enable/disable menu-items depending on the context.
How do we see this working out here:
- should we loop through menu.menuitems
- should we create an item getter (by item id?) on the Menu prototype

#2: The debugger is listening to the popup events for some of its context menus (see debugger.xul and options-view.js). We will probably have to fire similar events on the Menu. Or if we keep returning a popup object at in popup(), it could be its role (would like to avoid returning the raw XUL popup in this case still).

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #13)
> Some additional comments.
> 
> #1: In the inspector context menu, we enable/disable menu-items depending on
> the context.
> How do we see this working out here:
> - should we loop through menu.menuitems
> - should we create an item getter (by item id?) on the Menu prototype
> 

I was thinking we would make a brand new menu / menuitems list every time the context menu event happens, conditionally setting 'disabled' attribute when needed.

> #2: The debugger is listening to the popup events for some of its context
> menus (see debugger.xul and options-view.js). We will probably have to fire
> similar events on the Menu. Or if we keep returning a popup object at in
> popup(), it could be its role (would like to avoid returning the raw XUL
> popup in this case still).

Hm OK, yes we could return an object that emits particular events if we need that for compatibility, but we should also evaluate the places where this is happening to see if it's really needed (like in this case, looks like it's used for tests and also to change the UI while the popup is opened).  I don't see anything related to a close event here: https://github.com/electron/electron/blob/master/docs/api/menu.md

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #12)
> Comment on attachment 8746308 [details]
> MozReview Request: Bug 1257613 - Add an API to open context menus from an
> HTML document; f=jdescottes
> 
> https://reviewboard.mozilla.org/r/49361/#review46259
> 
> Thanks Brian, looks like a good start. The main thing I'd like to clarify
> are the semantics of the popup() method.
> 
> ::: devtools/client/framework/menu.js:15
> (Diff revision 1)
> > +  this.menuitems = [];
> > +  this.id = id;
> > +}
> > +
> > +Menu.prototype.append = function(menuitem) {
> > +  // TODO: do we want to support appending a plain object?
> 
> Would be nice, but if the goal is to match Electron's API, let's restrict to
> instances of MenuItem for now.
> 

Done

> ::: devtools/client/framework/menu.js:40
> (Diff revision 1)
> > +      let menusep = doc.createElement("menuseparator");
> > +      parent.appendChild(menusep);
> > +    } else {
> > +      let menuitem = doc.createElement("menuitem");
> > +      menuitem.setAttribute("label", item.label);
> > +      menuitem.addEventListener("command", () => {
> 
> Should remove this event listener when the popup is remove?
> 
> (our codebase is not consistent on this topic, we sometimes remove the
> event, sometimes don't so I might be wrong here)
> 

I don't think we need to since the node is being removed from the DOM and not referenced anymore.

> ::: devtools/client/framework/menu.js:50
> (Diff revision 1)
> > +        menuitem.setAttribute("type", "checkbox");
> > +      }
> > +      if (item.disabled) {
> > +        menuitem.setAttribute("disabled", "true");
> > +      }
> > +      if (item.accesskey) {
> 
> Not a big deal, but accesskey, selection and selectionType are not defined
> in MenuItem for now.

I'm not sure that we really need selection and selectionType for now, I'm going to remove them.

> 
> ::: devtools/client/framework/menu.js:68
> (Diff revision 1)
> > +      parent.appendChild(menuitem);
> > +    }
> > +  });
> > +};
> > +
> > +Menu.prototype.popup = function(win, x, y) {
> 
> Electron supports a "window" parameter, but does it make sense for us? In
> electron's API, they have to handle applications with multiple windows, so
> this parameter makes sense. For devtools, context menus will always be
> displayed in the devtools window. Here this parameter is more a technical
> requirement: we need a XUL document to create popups.
> 
> I guess "win" should always be the topmost devtools window? In the end it
> will be the only window with a XUL document. If it's true, does it make
> sense to force every caller to pass this argument? 
> 
> If we keep this argument we should make clear that this is not about "where
> should this context menu be displayed", but rather "where can I get a XUL
> document to create popup elements".

Yeah, we need a reference to some kind of window.  Ideally the toolbox window so we don't need to modify DOM in browser.xul.  Maybe we should just pass in the toolbox instead of a window to make it clear.

> ::: devtools/client/framework/menu.js:70
> (Diff revision 1)
> > +  });
> > +};
> > +
> > +Menu.prototype.popup = function(win, x, y) {
> > +  let doc = win.document;
> > +  let popup = doc.createElement("menupopup");
> 
> Is popup supposed to create context menus or only display them? 
> 
> Calling several times popup() for the same Menu instance will display
> several popups with the same id. Should we allow this? I guess in most cases
> this is not an issue because the previous context menu should have been
> dismissed before opening a new one, but I still have to raise the point.
> 
> Recreating a popup everytime is nice because it simplifies a lot of use
> cases: adding/removing menu items, setting items as disabled etc. No longer
> have to "update" existing elements. So I definitely think this is a nice
> approach, we might just want to make sure only one popup can be displayed at
> a time.
> 

They are removed in the "popuphidden" event so there shouldn't be more than one at once (once a new one is shown the popuphidden should have fired on the old one).  I also considered setting an attribute on these menus and then query selector for the menus when a new one is opened and removing all the old ones - I can do it that way if you think it's more clear.

> ::: devtools/client/framework/menu.js:77
> (Diff revision 1)
> > +    popup.id = this.id;
> > +  }
> > +  this._createMenuItems(popup);
> > +
> > +  // Remove the menu from the DOM once it's hidden
> > +  popup.addEventListener("popuphidden", (e) => {
> 
> This event listener should be removed?

See comment above

> ::: devtools/client/framework/menu.js:85
> (Diff revision 1)
> > +    }
> > +  }, true);
> > +
> > +  doc.querySelector("popupset").appendChild(popup);
> > +  popup.openPopupAtScreen(x, y, true);
> > +  popup.focus();
> 
> Is this focus() really needed, the popupmenu seems to get the focus when
> opened by default.
> 
> ::: devtools/client/framework/menu.js:87
> (Diff revision 1)
> > +
> > +  doc.querySelector("popupset").appendChild(popup);
> > +  popup.openPopupAtScreen(x, y, true);
> > +  popup.focus();
> > +
> > +  return popup;
> 
> Should we really return and expose the xul "menupopup" ? Even if we have to
> use XUL in this implementation, we should not encourage callers to rely on
> XUL elements.

I'm going to remove that return value

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8746308 [details]
MozReview Request: Bug 1257613 - Add an API to open context menus from an HTML document; f=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49361/diff/1-2/

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: use-menu-api-inspector-WIP.patch

Updated patch that passes in 'toolbox' instead of a window to popup

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8746308 [details]
MozReview Request: Bug 1257613 - Add an API to open context menus from an HTML document; f=jdescottes

https://reviewboard.mozilla.org/r/49361/#review46411

Looks good to me! I didn't list the eslint errors, but we should also fix them before landing.

::: devtools/client/framework/menu-item.js:7
(Diff revision 2)
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A partial implementation of the MenuItem API provided by electron:

nit: first comment line should be /**

::: devtools/client/framework/menu-item.js:8
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A partial implementation of the MenuItem API provided by electron:
> + * https://github.com/electron/electron/blob/master/docs/api/menu-item.md.

For the type parameter we do not support the "radio" value documented on https://github.com/electron/electron/blob/master/docs/api/menu-item.md 
We should document this discrepency. 

Overall it could be nice to add the JS doc here, even if it is a bit redundant with the electron API documentation (theirs might change etc...)

::: devtools/client/framework/menu-item.js:15
(Diff revision 2)
> +function MenuItem({
> +    accesskey = null,
> +    checked = false,
> +    click = () => {},
> +    disabled = false,
> +    label = "not set",

I think "" would be a fair default value for the label.

If we really want to highlight missing labels, we may want to wrap this value in brackets? Keeping in mind this default value will only be used if the "label" property is missing or undefined.

::: devtools/client/framework/menu.js:10
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const MenuItem = require("./menu-item");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
> +/* A partial implementation of the Menu API provided by electron:

nit:  first line should be /**

::: devtools/client/framework/menu.js:15
(Diff revision 2)
> +/* A partial implementation of the Menu API provided by electron:
> + * https://github.com/electron/electron/blob/master/docs/api/menu.md.
> + *
> + * Extra features:
> + *  - Emits an 'open' and 'close' event when the menu is opened/closed
> + *  - Takes an 'id' so tests can confirm XUL implementation is working

nit: rephrase this as @param JSDoc

::: devtools/client/framework/menu.js:56
(Diff revision 2)
> +  popup.addEventListener("popuphidden", (e) => {
> +    if (e.target === popup) {
> +      popup.remove();
> +      this.emit("close");
> +    }
> +  }, true);

Is useCapture=true needed here? If it is, can we add a comment explaining the reason?

::: devtools/client/framework/menu.js:64
(Diff revision 2)
> +    if (e.target === popup) {
> +      this.emit("open");
> +    }
> +  });
> +
> +  doc.querySelector("popupset").appendChild(popup);

In style-inspector-menu.js, we currently check for the existence of the popupset, and create it if needed. 
Is it safe to assume a popupset will always be available in the toolbox document?

::: devtools/client/framework/menu.js:89
(Diff revision 2)
> +      menuitem.setAttribute("label", item.label);
> +      menuitem.addEventListener("command", () => {
> +        item.click();
> +      });
> +
> +      if (item.type == "checkbox") {

nit: consistency, at L79 we use '===' to compare type to "separator", here we use '=='.  Pick one.

::: devtools/client/framework/menu.js:101
(Diff revision 2)
> +        menuitem.setAttribute("checked", "true");
> +      }
> +      if (item.accesskey) {
> +        menuitem.setAttribute("accesskey", item.accesskey);
> +      }
> +      if (item.id) {

The item id is not really used in our implementation for now and cannot be used as described on https://github.com/electron/electron/blob/master/docs/api/menu-item.md

Maybe we should remove it for now?

::: devtools/client/framework/test/browser_menu_api.js:21
(Diff revision 2)
> +  let tab = yield addTab(URL);
> +  let target = TargetFactory.forTab(tab);
> +  let toolbox = yield gDevTools.showToolbox(target, "webconsole");
> +
> +  yield testMenu(toolbox);
> +  yield testSubmenu(toolbox);

Maybe have another test checking the context menu can be used using the keyboard directly? This checks that the context menu has the focus when it is opened, and of course the keyboard navigation.

Both features come for free since we use a XUL popup menu, so up to you.

::: devtools/client/framework/test/browser_menu_api.js:122
(Diff revision 2)
> +  let closed = once(menu, "close");
> +
> +  info("Clicking the parent menu to expand the submenu");
> +  EventUtils.synthesizeMouseAtCenter(menus[0], {}, toolbox.doc.defaultView);
> +  yield once(menus[0], "popupshown");
> +

In the popuphidden event listener, we check that the event target is the current popup. This prevents the popup from being closed when hiding a submenu.

We could test this here by having an additional "normal" Menu Item and :
- send a mousemove event on the normal menu item (to trigger this properly, you need to first send a mousemove on menus[0]
- wait for the submenu popup to fire popuphidden
- check the main popup is still displayed
- send another mousemove on the submenu item and resume the test

This could also be tested by sending keyboard events (LEFT/RIGHT) to hide/re-open the submenu.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #18)

Thanks for the review, comments inline:

> ::: devtools/client/framework/menu-item.js:8
> (Diff revision 2)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +/* A partial implementation of the MenuItem API provided by electron:
> > + * https://github.com/electron/electron/blob/master/docs/api/menu-item.md.
> 
> For the type parameter we do not support the "radio" value documented on
> https://github.com/electron/electron/blob/master/docs/api/menu-item.md 
> We should document this discrepency. 

Added support for radio, and I've gone through the Menu API and stubbed out the non-implemented methods to throw.

> ::: devtools/client/framework/menu-item.js:15
> (Diff revision 2)
> > +function MenuItem({
> > +    accesskey = null,
> > +    checked = false,
> > +    click = () => {},
> > +    disabled = false,
> > +    label = "not set",
> 
> I think "" would be a fair default value for the label.
> 
> If we really want to highlight missing labels, we may want to wrap this
> value in brackets? Keeping in mind this default value will only be used if
> the "label" property is missing or undefined.
> 
Turns out "" is the default value as specified in their docs so I just switched it to that

> ::: devtools/client/framework/menu.js:64
> (Diff revision 2)
> > +    if (e.target === popup) {
> > +      this.emit("open");
> > +    }
> > +  });
> > +
> > +  doc.querySelector("popupset").appendChild(popup);
> 
> In style-inspector-menu.js, we currently check for the existence of the
> popupset, and create it if needed. 
> Is it safe to assume a popupset will always be available in the toolbox
> document?

Eh, yeah, but that's a good point.  BTW I think I might move the toolbox passing into the constructor so that the 'popup' method will more closely match what Electron provides.

> 
> ::: devtools/client/framework/menu.js:101
> (Diff revision 2)
> > +        menuitem.setAttribute("checked", "true");
> > +      }
> > +      if (item.accesskey) {
> > +        menuitem.setAttribute("accesskey", item.accesskey);
> > +      }
> > +      if (item.id) {
> 
> The item id is not really used in our implementation for now and cannot be
> used as described on
> https://github.com/electron/electron/blob/master/docs/api/menu-item.md
> 
> Maybe we should remove it for now?

I'll make a note that we have only partial support for id (the method it's supposed to be used on isn't implemented yet).

> 
> ::: devtools/client/framework/test/browser_menu_api.js:21
> (Diff revision 2)
> > +  let tab = yield addTab(URL);
> > +  let target = TargetFactory.forTab(tab);
> > +  let toolbox = yield gDevTools.showToolbox(target, "webconsole");
> > +
> > +  yield testMenu(toolbox);
> > +  yield testSubmenu(toolbox);
> 
> Maybe have another test checking the context menu can be used using the
> keyboard directly? This checks that the context menu has the focus when it
> is opened, and of course the keyboard navigation.
> 
> Both features come for free since we use a XUL popup menu, so up to you.
> 
> ::: devtools/client/framework/test/browser_menu_api.js:122
> (Diff revision 2)
> > +  let closed = once(menu, "close");
> > +
> > +  info("Clicking the parent menu to expand the submenu");
> > +  EventUtils.synthesizeMouseAtCenter(menus[0], {}, toolbox.doc.defaultView);
> > +  yield once(menus[0], "popupshown");
> > +
> 
> In the popuphidden event listener, we check that the event target is the
> current popup. This prevents the popup from being closed when hiding a
> submenu.
> 
> We could test this here by having an additional "normal" Menu Item and :
> - send a mousemove event on the normal menu item (to trigger this properly,
> you need to first send a mousemove on menus[0]
> - wait for the submenu popup to fire popuphidden
> - check the main popup is still displayed
> - send another mousemove on the submenu item and resume the test
> 
> This could also be tested by sending keyboard events (LEFT/RIGHT) to
> hide/re-open the submenu.

I'll take a look at that

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8746308 [details]
MozReview Request: Bug 1257613 - Add an API to open context menus from an HTML document; f=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49361/diff/2-3/

Comment 21

[Brian Grinstead [:bgrins]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4bd5f099cb

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/df99f7955b30

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df99f7955b30