Mark Banner (:standard8) Assignee
	Description • 7 years ago

We are currently working on enabling tests for Screenshots (a legacy based add-on with WebExtensions).

Currently the browser_contextmenu_input.js test fails due to various reasons (see bug 1358116), but one of them is that WebExtensions doesn't define IDs on the elements it adds to the context menu.

I think it would be reasonable to add ids for the WebExtension items, especially with more system add-ons working towards being WebExtensions & other things may need to reference the menu items.

	Kris Maglione [:kmag]
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8860331 [details]
Bug 1358213 - Give WebExtension created menuitems in the context menu an ID.

https://reviewboard.mozilla.org/r/132362/#review135490

::: browser/components/extensions/ext-contextMenus.js:199
(Diff revision 1)
>        element.setAttribute("label", label);
>      }
>  
> +    if (item.id && item.extension && item.extension.id) {
> +      element.setAttribute("id",
> +        makeWidgetId(item.extension.id) + "_" + item.id);

Please use a template string for this.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_chrome.js:50
(Diff revision 1)
>  
>      const popup = yield openSubmenu(submenu);
>      is(popup, submenu.firstChild, "Correct submenu opened");
>      is(popup.children.length, 2, "Correct number of submenu items");
>  
> +    let idPrefix = makeWidgetId(extension.id) + "_";

Template string, please.

	Pulsebot
	Comment 4 • 7 years ago

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f85a8a25e4bb
Give WebExtension created menuitems in the context menu an ID. r=kmag

	Iris Hsiao [:ihsiao]
	Comment 5 • 7 years ago
bugherder

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

	Rob Wu [:robwu]
	Comment 6 • 7 years ago

In the patch, why is the ID setter guarded by an if statement?

https://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/components/extensions/ext-menus.js#195-198

    if (item.id && item.extension && item.extension.id) {
      element.setAttribute("id",
        `${makeWidgetId(item.extension.id)}_${item.id}`);
    }

The menu can never be created without an extension, so the extension should always be set.
The extension always has an ID, so it is never falsey either.
The menu item always has an ID, so the item.id check is redundant.

The item.id check results in a bug: the element ID is not set if any of the following is true:
- The menu's ID is auto-generated for the first time (because the first ID is 0).
- The extension has set the empty to the empty string ("") - this is valid.


Mark, does the ID need to be stable, and if not, can I change its value? I am working on menu-related code and really need a ID that is guaranteed to exist and unique.

	Rob Wu [:robwu]
	Comment 7 • 7 years ago

(In reply to Rob Wu [:robwu] from comment #6)
> The item.id check results in a bug: the element ID is not set if any of the
> following is true:
> - The menu's ID is auto-generated for the first time (because the first ID
> is 0).
> - The extension has set the empty to the empty string ("") - this is valid.

And also, the ID can be numeric and a string. So if an extension only uses digits in a string, then there is a ID collision.

	Mark Banner (:standard8) Assignee
	Comment 8 • 7 years ago

(In reply to Rob Wu [:robwu] from comment #6)
> In the patch, why is the ID setter guarded by an if statement?
...
> The menu can never be created without an extension, so the extension should
> always be set.
> The extension always has an ID, so it is never falsey either.

For these, I was probably over-compensating in what it needed to be certain and safe. I've just looked, and a couple of callers do indeed pass the extension (it doesn't help there's no documentation of expectations of parameter details on the function calls).

> The menu item always has an ID, so the item.id check is redundant.

I believe at the time, I was looking at the extension requirements: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create says it is optional, but mandatory for event pages. 

https://developer.chrome.com/extensions/contextMenus also says the same.

However, it does look like there's autogeneration in here via ext-c-menus.js - something else which wasn't clear to me when I wrote this :-(

> The item.id check results in a bug: the element ID is not set if any of the
> following is true:
> - The menu's ID is auto-generated for the first time (because the first ID
> is 0).

> - The extension has set the empty to the empty string ("") - this is valid.

I guess `"id" in item` might have been a better check.

> Mark, does the ID need to be stable, and if not, can I change its value? I
> am working on menu-related code and really need a ID that is guaranteed to
> exist and unique.

If the extension specifies the id, I think it has to be stable. Otherwise, I personally would expect an id to remain consistent across sessions for a particular set of WebExtensions loaded in a browser.

Although I'm not a WebExtensions peer, so this is just my opinion.