1066383 - HTML contextmenu attribute does not work in e10s

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

+++ This bug was initially created as a clone of Bug #1047751 +++

See http://davidwalsh.name/html5-context-menu for a description of this feature. I'm spinning this off from bug 1047751.

Comment 1

[Neil Deakin]

Attachment: Implement contextmenu attribute

This seems to work. This adds some code duplication with nsXULContextMenuBuilder.cpp A later patch can remove that file and just use PageMenu for both remote and non-remote cases.

Comment 2

[Dave Townsend [:mossop]]

Can you give this a points estimate Neil?

Comment 3

[Marco Mucci [:MarcoM]]

Hi Neil, can you mark this bug for QE verification.

Comment 4

[Neil Deakin]

Attachment: Updated patch

This patch implements this for multiprocess by spliting the generation into two steps. I moved it into a js component as I thought this would be easier to generate a js object but I'm not sure the change was worthwhile, as now multiple js serialization/deserialization occurs.

I also want to write a test.

Comment 5

[Jan Varga [:janv]]

Comment on attachment 8519613 [details] [diff] [review]
Updated patch

Review of attachment 8519613 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is exactly what I had in mind when I was implementing HTML5 context menu feature. The extra nsIXULContextMenuBuilder was added because at that time Firefox on Android was still using XUL, so someone could write a separate XUL builder for it.

Comment 6

[Neil Deakin]

Attachment: Context menu attribute support

Added a test and fixed a few bugs. The existing contextmenu test crashes which I'm still investigating but it shouldn't be too hard to fix.

Comment 7

[Neil Deakin]

Attachment: Context menu attribute support, v3

The crash was because the new files weren't added to the installer and createBuilder was returning null. I marked the method as allowing a null return value, which can happen anyway.

Comment 8

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

Comment on attachment 8531784 [details] [diff] [review]
Context menu attribute support

Review of attachment 8531784 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Neil - this looks good, just a few questions / points...

::: browser/base/content/content.js
@@ +39,5 @@
>  
>  // Load the form validation popup handler
>  var formSubmitObserver = new FormSubmitObserver(content, this);
>  
> +// The last context menu builder that was used. 

Nit: trailing whitespace

::: browser/base/content/test/general/browser_contextmenu_childprocess.js
@@ +1,1 @@
> +const gBaseURL = "https://example.com/browser/browser/base/content/test/general/";

Missing license at the top of this file - Creative Commons, I guess? Or whatever we decided to release tests under.

@@ +1,3 @@
> +const gBaseURL = "https://example.com/browser/browser/base/content/test/general/";
> +
> +function test() {

Instead of using callback-based styling, I think we're trying to move our tests over to be more task-y, Promise-y. Example:

add_task(function *() {
  let tab = gBrowser.addTab();
  yield promiseTabLoadEvent(tab, gBaseURL + "subtst_contextmenu.html");
  let rect = browser.contentWindow.document.getElementById("test-pagemenu").getBoundingClientRect();
  let popupShownPromise = promiseWaitForEvent(window, "popupshown", true);
  // ... synth mouse event
  let event = yield popupShownPromise;
  checkMenu(event);
  event.target.hidePopup();
  gBrowser.removeCurrentTab();
});

@@ +8,5 @@
> +
> +  function loaded() {
> +    browser.removeEventListener("load", loaded, true);
> +
> +    let rect = browser.contentWindow.document.getElementById("test-pagemenu").getBoundingClientRect()

Maybe use contentDocument instead of contentWindow.document. Also, missing semicolon.

@@ +9,5 @@
> +  function loaded() {
> +    browser.removeEventListener("load", loaded, true);
> +
> +    let rect = browser.contentWindow.document.getElementById("test-pagemenu").getBoundingClientRect()
> +    var eventDetails = { type : "contextmenu", button : 2 };

let, not var

@@ +10,5 @@
> +    browser.removeEventListener("load", loaded, true);
> +
> +    let rect = browser.contentWindow.document.getElementById("test-pagemenu").getBoundingClientRect()
> +    var eventDetails = { type : "contextmenu", button : 2 };
> +    EventUtils.synthesizeMouse(browser, rect.left + 8, rect.top + 8,

Where are these 8's coming from? Should we just synthesize the mouse at the center of the element?

::: dom/html/htmlMenuBuilder.js
@@ +28,5 @@
> +// other UI from this object. The default UI is done in PageMenu.jsm.
> +//
> +// When a multi-process browser is used, the first step is performed by the child
> +// process and the second step is performed by the parent process.
> + 

Nit - trailing whitespace.

@@ +37,5 @@
> +
> +  PAGEMENU_ATTR: "pagemenu",
> +  GENERATEDITEMID_ATTR: "generateditemid",
> +
> +  currentNode: null,

Is there any value in defining these instance-specific things on the prototype?

@@ +42,5 @@
> +  root: null,
> +  items: {},
> +  nestedStack: [],
> +
> +  toJSONString: function()

Let's stick with the inline { style in this file.

@@ +52,5 @@
> +  {
> +    if (!this.currentNode) {
> +      this.root = {
> +        type: "menu",
> +        children: []

Let's keep the trailing comma convention here, and elsewhere in this file.

@@ +62,5 @@
> +      this.currentNode = {
> +        type: "menu",
> +        label: aLabel,
> +        children: [],
> +      }

Nit - missing semicolon

@@ +70,5 @@
> +  },
> +
> +  addItemFor: function(aElement, aCanLoadIcon) {
> +    if (!("children" in this.currentNode)) {
> +      return;

Worth logging this problem to the console?

@@ +78,5 @@
> +      type: "menuitem",
> +      label: aElement.label
> +    };
> +
> +    var elementType = aElement.type;

let, not var

@@ +116,5 @@
> +      return;
> +    }
> +
> +    let children = this.currentNode.children;
> +    if (children[children.length - 1].type == "separator") {

Better bounds-check the children array before trying to access it.

@@ +123,5 @@
> +  },
> +
> +  closeContainer: function()
> +  {
> +    this.currentNode = this.nestedStack.length ? this.nestedStack.pop() : null;

I'm not too too familiar with this API, but do we want callers to be able to call closeContainer even on the root? Like, if this.nestedStack.length == 0... do we really want to overwrite this.currentNode with null?

@@ +136,5 @@
> +  }
> +};
> +
> +var components = [HTMLContextMenuBuilder];
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory(components);

Might as well just do:

this.NSGetFactory = XPCOMUtils.generateNSGetFactory([HTMLContextMenuBuilder]);

::: dom/html/nsIMenuBuilder.idl
@@ +49,5 @@
>     */
>    void closeContainer();
>  
> +  /**
> +   * Returns a JSON string representing the menu hierarchy. It is of the form:

Looks like the documentation didn't get finished here.

::: toolkit/modules/PageMenu.jsm
@@ +15,4 @@
>    popup: null,
>    builder: null,
>  
> +  getContextMenu: function(aTarget)

Nit - let's stick with the inline-{ convention

@@ +29,5 @@
>  
> +    return null;
> +  },
> +
> +  maybeBuild: function(aTarget) {

It'd be great to get some documentation for these new methods to help distinguish them.

@@ +63,5 @@
> +
> +    return this.maybeBuildAndAttachMenuWithObject(menuObject, null, aPopup);
> +  },
> +
> +  maybeBuildAndAttachMenuWithObject: function(aMenu, aBrowser, aPopup) {

re: documentation, please also document why someone might call this with aBrowser set to null. Also, I think we need to be clear what aMenu is here (a JS object, and not actually a menu node or something like that)

@@ +77,3 @@
>      var fragment = aPopup.ownerDocument.createDocumentFragment();
> +    this.buildXULMenu(aMenu, fragment);
> +  

Nit - trailing whitespace.

@@ +95,5 @@
>  
>      return true;
>    },
>  
> +  buildXULMenu: function(aNode, aFragment) {

aFragment -> aElementForAppending, or something along those lines, since it's not necessarily a document fragment - it might be a menupopup.

@@ +96,5 @@
>      return true;
>    },
>  
> +  buildXULMenu: function(aNode, aFragment) {
> +    var document = aFragment.ownerDocument;

let, not var

@@ +141,5 @@
> +          this.buildXULMenu(child, menupopup);
> +          break;
> +      }
> +
> +      menuitem.setAttribute(this.GENERATEDITEMID_ATTR, child.id ? child.id : 0);

So for any items that have no child ID set, we set the generated item attribute to 0, and that'll screw up command dispatching since multiple items will share the same generated ID. I know that we've written our stuff so that everything has an ID, but this sounds like a footgun.

If there's no child.id, we might want to log to the error console, or warn or something.

We should also document this method to make it very very clear what we expect to be passed to it.

@@ +150,5 @@
>    handleEvent: function(event) {
>      var type = event.type;
>      var target = event.target;
>      if (type == "command" && target.hasAttribute(this.GENERATEDITEMID_ATTR)) {
> +      if (this.builder) {

In what cases is this.builder defined, and in what cases is this.browser defined but this.builder is not?

@@ +154,5 @@
> +      if (this.builder) {
> +        this.builder.click(target.getAttribute(this.GENERATEDITEMID_ATTR));
> +      }
> +      else if (this.browser) {
> +        this.browser.messageManager.sendAsyncMessage("ContextMenu::DoCustomCommand",

We've been using just single-colon delimiters for these message names. Might as well stick with the convention.

Comment 9

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

I'm afraid this review was on the older version of the patch, just so you know. The feedback likely still applies, based on the scope of what you said you changed in comment 7.

Comment 10

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

Also, I'm really kinda worried about how PageMenu.jsm is used by both content.js and browser.js. I think we should make it very clear which methods expect to be called by content / browser.

Or, if possible, we should split out the stuff that content.js needs into a separate class that content.js can use - so PageMenu can export PageMenuParent and PageMenuChild objects, for example. This might make it clearer who calls what and where.

Comment 11

[Neil Deakin]

Attachment: Context menu attribute support, v4

Comment 12

[Neil Deakin]

Comment on attachment 8535693 [details] [diff] [review]
Context menu attribute support, v4

> Where are these 8's coming from? Should we just synthesize the mouse at the
> center of the element?

synthesizeMouseAtCenter doesn't work when target is in the child process.


> I'm not too too familiar with this API, but do we want callers to be able to
> call closeContainer even on the root? Like, if this.nestedStack.length ==
> 0... do we really want to overwrite this.currentNode with null?

I changed this to set it to the root instead.

Comment 13

[Jan Varga [:janv]]

Comment on attachment 8535693 [details] [diff] [review]
Context menu attribute support, v4

Review of attachment 8535693 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +40,5 @@
>  // Load the form validation popup handler
>  var formSubmitObserver = new FormSubmitObserver(content, this);
>  
> +// The last context menu builder that was used.
> +var contextMenuBuilder;

This might be removed.

@@ +88,5 @@
> +addMessageListener("ContextMenu:DoCustomCommand", function(message) {
> +  if (contextMenuBuilder) {
> +    contextMenuBuilder.click(message.data);
> +    // Assume that there will only be one command executed so clear the builder.
> +    contextMenuBuilder = null;

Hm, this doesn't null out the builder in PageMenuChild.

@@ +139,5 @@
>          InlineSpellCheckerContent.initContextMenu(event, editFlags, this);
>      }
>  
> +    let customMenuItems = PageMenuChild.build(event.target);
> +    contextMenuBuilder = PageMenuChild.builder;

I don't like this much, I mean saving the builder locally and then calling click() on it. What about adding a click method to PageMenuChild and forward to the builder there and maybe make the builder in PageMenu a private var (_builder).

I assume PageMenuChild can't get a new builder between build() and click().

::: dom/html/HTMLMenuElement.cpp
@@ +15,5 @@
>  #include "nsIURI.h"
>  
>  NS_IMPL_NS_NEW_HTML_ELEMENT(Menu)
>  
> +#define HTMLMENUBUILDER_CID "@mozilla.org/content/html-menu-builder;1"

Nit 1: put this between |#include "nsIURI.h"| and |NS_IMPL_NS_NEW_HTML_ELEMENT(Menu)|

Nit 2: rename HTMLMENUBUILDER_CID to HTMLMENUBUILDER_CONTRACTID ?

@@ +101,5 @@
>  
>    *_retval = nullptr;
>  
>    if (mType == MENU_TYPE_CONTEXT) {
> +    nsCOMPtr<nsIMenuBuilder> builder = do_CreateInstance(HTMLMENUBUILDER_CID);

Should we assert |builder| or at least |NS_WARN_IF(!builder)| ?

@@ +115,5 @@
>    if (mType != MENU_TYPE_CONTEXT) {
>      return nullptr;
>    }
>  
> +  nsCOMPtr<nsIMenuBuilder> ret = do_CreateInstance(HTMLMENUBUILDER_CID);

Here too, actually I don't understand why the XPCOM version of this method doesn't call this one.

::: dom/html/htmlMenuBuilder.js
@@ +12,5 @@
> +// A global value that is used to identify each context menu item. It is
> +// incremented with each one that is found.
> +var gGeneratedId = 1;
> +
> +function HTMLContextMenuBuilder() {

HTMLContentMenuBuilder or HTMLMenuBuilder ?
the contract id is "@mozilla.org/content/html-menu-builder;1"
the filename id htmlMenuBuilder.js

It would be better to use consistent naming.

@@ +32,5 @@
> +
> +HTMLContextMenuBuilder.prototype =
> +{
> +  classID:        Components.ID("{51c65f5d-0de5-4edc-9058-60e50cef77f8}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIMenuBuilder, Ci.nsIXULContextMenuBuilder]),

Aren't you removing the nsIXULContextMenuBuilder ?

@@ +83,5 @@
> +      }
> +    }
> +
> +    let icon = aElement.icon;
> +    if (icon && aCanLoadIcon) {

Hm, this branch executes even when icon="", is that right ?

::: dom/webidl/HTMLMenuElement.webidl
@@ +43,5 @@
>     * Currently, it returns nsXULContextMenuBuilder for context menus.
>     * Toolbar menus are not yet supported (the method returns null).
>     */
>    [ChromeOnly]
> +  MenuBuilder? createBuilder();

Update the comment above since you removed nsXULContextMenuBuilder ?

::: toolkit/modules/PageMenu.jsm
@@ +1,5 @@
>  /* 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/. */
>  
> +this.EXPORTED_SYMBOLS = ["PageMenuParent", "PageMenuChild"];

I like the distinction PageMenu(Parent|Child).

@@ +54,5 @@
> +    let menuString = this.builder.toJSONString();
> +    if (!menuString) {
> +      return null;
> +    }
> +

The serialization/deserialization could be avoided for the single process case, roght ?
Maybe add a comment for that.

@@ +100,5 @@
> +    for (let child of children) {
> +      let menuitem;
> +      switch (child.type) {
> +        case "menuitem":
> +          if (!child.id) { 

Nit: extra white space

@@ +116,5 @@
> +          if (child.label) {
> +            menuitem.setAttribute("label", child.label);
> +          }
> +          if (child.icon) {
> +            menuitem.setAttribute("image", child.icon);

menuitem-icon or menuitem-iconic ?

@@ +168,5 @@
>  
>        this.popup.removeEventListener("popuphidden", this);
>        this.popup.removeEventListener("command", this);
>  
> +      this.browser = null;

Nit, move after |this.builder = null;| to be consistent with the declaration.

@@ +258,5 @@
> +    return this.buildAndAttachMenuWithObject(menuObject, null, aPopup);
> +  },
> +
> +  /*
> +   * Given a json menu object and popup, add the context menu to the popup. This

Nit: JSON ?

@@ +278,5 @@
> +PageMenuChild.prototype = {
> +  __proto__ : PageMenu.prototype,
> +
> +  /*
> +   * Given a target node, return a json object for the custom menu commands. The

dtto

@@ +281,5 @@
> +  /*
> +   * Given a target node, return a json object for the custom menu commands. The
> +   * object will consist of a hierarchical structure of menus, menuitems or
> +   * separators. Supported properties of each are:
> +   *   Menu: children, id, label, type="menu"

I think id is not supported.

@@ +282,5 @@
> +   * Given a target node, return a json object for the custom menu commands. The
> +   * object will consist of a hierarchical structure of menus, menuitems or
> +   * separators. Supported properties of each are:
> +   *   Menu: children, id, label, type="menu"
> +   *   Menuitems: checkbox, checked, disabled, id, image, label, type="menuitem"

image or icon ?

@@ +283,5 @@
> +   * object will consist of a hierarchical structure of menus, menuitems or
> +   * separators. Supported properties of each are:
> +   *   Menu: children, id, label, type="menu"
> +   *   Menuitems: checkbox, checked, disabled, id, image, label, type="menuitem"
> +   *   Separators: id, type="separator"

Id doesn't seem to be supported.

Comment 14

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

Comment on attachment 8535693 [details] [diff] [review]
Context menu attribute support, v4

Review of attachment 8535693 [details] [diff] [review]:
-----------------------------------------------------------------

Jan gave you an excellent review. I'm happy with this patch - address his concerns, and I think we're done here. Great job, Neil!

Comment 15

[Jan Varga [:janv]]

Yeah great job, Neil!

Comment 16

[Neil Deakin]

Comment on attachment 8535693 [details] [diff] [review]
Context menu attribute support, v4

The change to HTMLMenuElement.webidl seems to need another reviewer.

Comment 17

[Neil Deakin]

Attachment: 8535693: Context menu attribute support, v5

Checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/8455b9eef8f8

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4867935&repo=mozilla-inbound

Comment 19

[Neil Deakin]

Attachment: Context menu attribute support, v6

The last patch wasn't for this bug. This patch is right and fixes the broken installer paths.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

CLOBBER touch because apparently the build system doesn't always detect the removal of an IDL file.
https://hg.mozilla.org/integration/mozilla-inbound/rev/961dff3f8c30

Comment 21

[Neil Deakin]

Attachment: removewarning

Fix warning on some platforms.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70fe31e4b65e
https://hg.mozilla.org/mozilla-central/rev/961dff3f8c30

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0

Confirming the fix on latest Nightly, build ID: 20141228030216.