1190667 - Implement contextMenus API for open extension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

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

https://developer.chrome.com/extensions/contextMenus

Comment 1

[Mindaugas <LA-MJ>]

I'd like to add some background information on trouble with this API on chrome. Sorry if I am too much out of line here.

There is an inherent race condition if add-on developers want to modify the context menu that is about to come up. The issue is detailed in a neglected chromium bug[1].

Since google code project is closing down I'll quote my post with a potential use case:

> I want to use it in my extension to generate bb-code from element(s) under the mouse pointer at the time of the click.
> There are a couple of different generation options so I use different menu entries for them.
> Currently I use onmousedown[event.button=2] and users using windows don't complain. 
> But for me on linux the menu comes up before I'm finished generating the menu entries 90% of the time.
> Switching to oncontextmenu seems to be better for linux but breaks windows

[1] https://code.google.com/p/chromium/issues/detail?id=60758

Comment 2

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

Thanks for pointing this out. We should add an onBeforeShow event. If the extension takes too long to respond, we can show the menu and add any extension-created items later if they appear.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: contextMenus v1

Things I don't have yet:
- radio buttons / checkboxes
- event page version
- icon :(
- support for update (we should look into the problem mentioned above)
- tests

At least tests and icons should be added before checking it in probably. But I guess it's ready for an initial review, and those can be added in a separated patch from the basics. The r? flag is actually more like a feedback? flag, but I'm afraid I will have to focus on e10s m8's for a bit before I can come back to this and add the missing features.

Comment 4

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

Comment on attachment 8653402 [details] [diff] [review]
contextMenus v1

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

This looks good. I think we should have tests before landing, as well as a working update implementation. Also, please brace all conditionals.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Attachment: contextMenus. v2

I've added update and some initial tests. Not a full test coverage but I think a good start.

Comment 6

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

Comment on attachment 8657102 [details] [diff] [review]
contextMenus. v2

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

I'd like to do another round. I think it's close, but it just needs a bit more work.

::: browser/base/content/nsContextMenu.js
@@ +44,5 @@
>        else {
>          this.hasPageMenu = PageMenuParent.buildAndAddToPopup(this.target, aXulMenu);
>        }
> +
> +      ExtensionUtils.contextMenuItems.populateMenu(aXulMenu, this, gContextMenuContentData);

Probably want to pass this.browser in here so we can access the tab easily.

Also, going through ExtensionUtils here seems pretty awkward. Could we instead notify an observer? There's code here that does something like that:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#93

I also don't like how the internal state of nsContextMenu.js is exposed here. Can you pack up all the properties that we need in ext-contextMenus.js (onVideo, etc.) into an object literal here and just pass that in? You could combine stuff from |this| and gContextMenuContentData.

::: browser/components/extensions/ext-contextMenus.js
@@ +12,5 @@
> +// Note: we want to enumerate all the menu items so
> +// this cannot be a weak map.
> +let contextMenuMap = new Map();
> +
> +// Not really used yet, will be used for event pages.

It looks like this is used even for normal background pages.

@@ +19,5 @@
> +// If id is not specified for an item we use an integer.
> +let nextID = 0;
> +
> +// We want to make sure that ids are unique.
> +let IDs = new Set();

Maybe call it |usedIDs|? Also, this needs to be per-extension. Instead, though, what if we make contextMenuMap be a Map[Extension -> Map[ID, MenuItem]]? Then you don't need this separate IDs map. And some of the loops over all items could be replaced with map lookups.

@@ +36,5 @@
> +    this._xulMenu = xulMenu;
> +    let doc = xulMenu.ownerDocument;
> +    for (let [ext, menuItemSet] of contextMenuMap) {
> +      for (let item of menuItemSet) {
> +        if (item.enabled &&

Are !enabled items actually not shown, or are they just grayed out?

@@ +40,5 @@
> +        if (item.enabled &&
> +            item.enabledForContext(contextMenu) &&
> +            item.enabledForContent(contextMenu, contentData)) {
> +          let element;
> +          if (item.isMenu) {

Chrome's behavior is as follows: if the extension adds one context menu item, it goes in the menu directly. If it adds multiple items, they all end up in a submenu. I think we should replicate that behavior.

@@ +50,5 @@
> +            // Storing the menupopup in a map, so we can find it for its
> +            // menu item children later.
> +            this._parentMap.set(item.id, menupopup);
> +          } else {
> +            element = (item.type == "separator") ? doc.createElement("menuseparator")

Maybe put this on three lines instead of two.

@@ +54,5 @@
> +            element = (item.type == "separator") ? doc.createElement("menuseparator")
> +                                                 : doc.createElement("menuitem");
> +          }
> +
> +          element.setAttribute("label", item.title);

Please put a FIXME here to handle %s in the title. And another somewhere for setting the icon.

@@ +98,5 @@
> +    this._itemsToCleanUp.clear();
> +  },
> +
> +  _itemsToCleanUp: new Set(),
> +  _parentMap: new Map()

Can this be a local variable? If not, it should be cleared in "popuphidden".

@@ +111,5 @@
> +
> +  if (!(contextMenu.isContentSelected ||
> +        contextMenu.onTextInput || contextMenu.onLink || contextMenu.onImage ||
> +        contextMenu.onVideo || contextMenu.onAudio || contextMenu.onCanvas)) {
> +    contexts.add("page");

I think we want to add "page" unconditionally. The only time we wouldn't is if the context menu is opened for the browserAction toolbar item, which we don't support in this patch anyway.

@@ +147,5 @@
> +}
> +
> +// These sets are only used for argument validation.
> +let contextTypes = new Set(["all", "page", "frame", "selection", "link",
> +  "editable", "image", "video", "audio", "browser_action", "page_action"]);

Please format these one per line (although I don't think we need them).

@@ +165,5 @@
> +  // flag is for the later case.
> +  init(createProperties, update=false) {
> +    let item = this;
> +    // return true if the prop was set on |this|
> +    function parseProp(propName, defaultValue=null) {

Spaces around =

@@ +169,5 @@
> +    function parseProp(propName, defaultValue=null) {
> +      if (propName in createProperties) {
> +        item[propName] = createProperties[propName];
> +        return true;
> +      } else if (!update && defaultValue!==null) {

Spaces around !==

@@ +194,5 @@
> +
> +    parseProp("title");
> +    parseProp("checked", false);
> +    parseProp("contexts", ["all"]);
> +    for (let c of this.contexts) {

We haven't done any kind of argument validation so far. I'd rather do it in a more structured way in bug 1190677. So maybe take out this one and the itemTypes one.

@@ +233,5 @@
> +    parseProp("enabled", true);
> +  },
> +
> +  remove() {
> +    contextMenuMap.get(this.extension).delete(this);

This doesn't remove the children of a menu item. The result could be that you get an exception when trying to build those orphaned menu items (since their parent item will be missing from the parent map).

@@ +236,5 @@
> +  remove() {
> +    contextMenuMap.get(this.extension).delete(this);
> +  },
> +
> +  get data() {

Just remove this for now.

@@ +253,5 @@
> +      mediaType = "image";
> +    }
> +
> +    let url;
> +    let win = contextMenu.target.ownerDocument.defaultView;

Is there any way we can do this without a CPOW? I think we should do a better job for this page/frame URL stuff. The frame URL should be available from gContextMenuContentData.docLocation. The page URL can be found in browser.currentURI.

@@ +260,5 @@
> +    }
> +
> +    return {
> +      menuItemId: this.id,
> +      parentMenuItemId: this.parentId,

This will always create a property with value |undefined|. I'd rather we not define the property at all if it doesn't apply. Same comment for some of the other properties here.

@@ +263,5 @@
> +      menuItemId: this.id,
> +      parentMenuItemId: this.parentId,
> +      mediaType: mediaType,
> +      linkUrl: contextMenu.linkURL,
> +      srcUrl: contextMenu.target.src,

This also uses a CPOW. Could contextMenu.mediaURL be used instead? At least that reuses data we already got from a previous CPOW.

@@ +270,5 @@
> +      selectionText: contextMenu.textSelected,
> +      editable: contextMenu.onEditableArea,
> +      wasChecked: false,
> +      checked: false,
> +      tab: undefined

This would be TabManager.convert(this.extension, <selectedTab>). The selected tab should be pretty easy to get if you pass the <browser> element in.

@@ +288,5 @@
> +  enabledForContent(contextMenu, contentData) {
> +    if (this.documentUrlMatchPattern &&
> +        !this.documentUrlMatchPattern.matches(contentData.documentURIObject)) {
> +      return false;
> +  }

Indent

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +9,5 @@
> +      "permissions": ["contextMenus"]
> +    },
> +
> +    background: function() {
> +

No blank line here.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: contextMenus. v3

Thanks for the previous review by the way, it was super useful. I think I've addressed all the comments, added a few more tests as well.

(In reply to Bill McCloskey (:billm) from comment #6)
> Also, going through ExtensionUtils here seems pretty awkward. Could we
> instead notify an observer? There's code here that does something like that:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#93

I'm not sure about the name of this new notification. I used chrome in the name because it's happening in the chrome process as opposed to the other one that is fired in the content process. I'm not happy with it though...

> what if we make contextMenuMap be a Map[Extension -> Map[ID,
> MenuItem]]?

I was considering this, after I finished my previous patch... I agree this it the right approach, I've take this approach in the current version.

> Are !enabled items actually not shown, or are they just grayed out?

Yeah, you are right they probably should be just grayed out.

> Chrome's behavior is as follows: if the extension adds one context menu
> item, it goes in the menu directly. If it adds multiple items, they all end
> up in a submenu. I think we should replicate that behavior.

We don't seem to have a name prop in the manifest, so I kind of skipped this one. I've added it in the current version but I could only use the addon id for the name of the top menu.

> This doesn't remove the children of a menu item. The result could be that
> you get an exception when trying to build those orphaned menu items (since
> their parent item will be missing from the parent map).

You were right here too, chrome removes the children as well, I've copied that behavior.

Comment 8

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

Comment on attachment 8666613 [details] [diff] [review]
contextMenus. v3

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

This looks good. I think we need a follow-up to improve how we keep track of the parent/child relationships. But I think this is ready to land.

::: browser/base/content/nsContextMenu.js
@@ +64,5 @@
> +        linkUrl: this.linkURL,
> +        selectionText: this.isTextSelected ? this.selectionInfo.text : undefined,
> +      };
> +      subject.wrappedJSObject = subject;
> +      Services.obs.notifyObservers(subject, "chrome-contextmenu", null);

How about on-build-contextmenu?

::: browser/components/extensions/ext-contextMenus.js
@@ +38,5 @@
> +  build: function(contextData) {
> +    // TODO: icons should be set for items
> +    let xulMenu = contextData.menu;
> +    xulMenu.addEventListener("popuphidden", this);
> +    this._xulMenu = xulMenu;

I'd rather not save this since it could cause leak issues. I don't really see how the event target could end up being wrong since we only add the listener to the menu we're interested in.

@@ +45,5 @@
> +      let parentMap = new Map();
> +      let topLevelItems = new Set();
> +      for (let [id, item] of menuItemMap) {
> +        if (item.enabledForContext(contextData) &&
> +            item.enabledForContent(contextData)) {

Can we just have a single function |enabled|? I'm not sure I understand the distinction, and it's confusing that the words are so similar.

@@ +47,5 @@
> +      for (let [id, item] of menuItemMap) {
> +        if (item.enabledForContext(contextData) &&
> +            item.enabledForContent(contextData)) {
> +          let element;
> +          if (item.isMenu) {

In theory |remove| should unset the isMenu attribute if there are no longer any children. I don't think you need to fix this now though. See below.

@@ +73,5 @@
> +          let parentId = item.parentId;
> +          if (parentId && parentMap.has(parentId)) {
> +            // If parentId is set we have to look up its parent
> +            // and appened to its menupopup.
> +            let parentElement = parentMap.get(parentId);

We're assuming that parents are always added before children. That's not really guaranteed because you can |update| the parentId I think. Again, I don't think you need to fix this here. But please file a follow-up on coming up with a better way to manage the menu hierarchy. We probably should be keeping a tree data structure up-to-date at all times--not just when the menu is built.

@@ +93,5 @@
> +      }
> +      if (topLevelItems.size > 1) {
> +        // If more than one top level items are visible, callopse them.
> +        let top = doc.createElement("menu");
> +        top.setAttribute("label", ext.id);

Extensions have a "name" attribute in their manifest. Please add a name() getter on Extension that does |return this.localize(this.manifest.name);|.

@@ +113,5 @@
> +  },
> +
> +  handleEvent: function(event) {
> +    let target = event.target;
> +    if (this._xulMenu != target) {

Don't think we need this.

@@ +192,5 @@
> +
> +    if (update && "id" in createProperties) {
> +      throw new Error("Id of a MenuItem cannot be changed");
> +    } else if (!update) {
> +      let isIsUsed = contextMenuMap.get(this.extension).has(createProperties.id);

isIdUsed?

@@ +211,5 @@
> +      this.onclick = createProperties.wrappedJSObject.onclick;
> +    }
> +
> +    parseProp("parentId");
> +    if (this.parentId) {

Can you instead say |if (this.parseProp("parentId")|. Also, I think we should explicitly set parentId to undefined or null if it's not present. See below.

In theory we're supposed to check for cycles at this point, but this should be part of the follow-up.

@@ +223,5 @@
> +        throw new Error("MenuItem with this parentId not found");
> +      }
> +    }
> +
> +    if (parseProp("documentUrlPatterns") && this.documentUrlPatterns) {

I don't see a reason to check if this.documentUrlPatterns is defined. I don't think that explicitly passing undefined properties is reasonable. If it is, we should change parseProp to return false in that case. Same for targetUrlPatterns.

@@ +244,5 @@
> +    function hasAncestorWithId(item, id) {
> +      if (checked.has(item)) {
> +        return checked.get(item);
> +      }
> +      if (!item.parentId) {

What if parentId is 0 or ""? Let's do an |=== undefined| check or something.

@@ +264,5 @@
> +    }
> +
> +    let toRemove = new Set();
> +    toRemove.add(this.id);
> +    for (let [id, i] of menuMap) {

Why not call it item instead of i?

@@ +321,5 @@
> +    }
> +    return false;
> +  },
> +
> +  enabledForContent(contextData, contentData) {

You only pass one param in here.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #8)
> This looks good. I think we need a follow-up to improve how we keep track of
> the parent/child relationships. But I think this is ready to land.

Yeah I agree.

> How about on-build-contextmenu?

I like it!

> In theory |remove| should unset the isMenu attribute if there are no longer
> any children. I don't think you need to fix this now though. See below.

And if the last child changes its parentId that should do the same...

> We're assuming that parents are always added before children. That's not
> really guaranteed because you can |update| the parentId I think. Again, I

Yeah this simple structure made sense without update, but everything gets more
complicated with it. Good thing is that from this point cleaning up the tree
structure will be a good first bug for someone who is not familiar with ff but
can deal with some complexity. 

> don't think you need to fix this here. But please file a follow-up on coming
> up with a better way to manage the menu hierarchy. We probably should be
> keeping a tree data structure up-to-date at all times--not just when the
> menu is built.

The idea was to keep around a list as the partially ordered set that we use for
guarding against possible cycles in the graph. But with the current
data structure that's not really possible (we're using a map instead of a list now)

> In theory we're supposed to check for cycles at this point, but this should
> be part of the follow-up.

Yeah I wanted to avoid this... Well we can still keep around a partially ordered list for this
check, but I'm fine with other approaches as well to avoid cycles.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Sorry for the bugspam, I didn't want to remove the 1161828 from the block list.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fad12ceae9b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8fad12ceae9b

Comment 13

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus