Bug 1215376 - Ensure that menu elements have a unique ID draft
authorRob Wu <rob@robwu.nl>
Mon, 11 Sep 2017 01:01:41 +0200
changeset 716031 d9a1b8cc345e2295eee62701a2ecc51cf861a3c1
parent 716030 703490b5e551455bee0ebcdecaab2aa2ea5321ba
child 716032 46be6fc1990283364c624a3d3240f207cef84261
push id94307
push userbmo:rob@robwu.nl
push dateFri, 05 Jan 2018 00:24:53 +0000
bugs1215376, 1358213
milestone59.0a1
Bug 1215376 - Ensure that menu elements have a unique ID Previously, there was no guarantee for the uniqueness of the ID. Now the ID is unique, and always set. See https://bugzil.la/1358213#c6 and #c7 for examples of collisions. MozReview-Commit-ID: 7CHP6tWFDUg
browser/components/extensions/ext-menus.js
browser/components/extensions/test/browser/browser_ext_menus.js
--- a/browser/components/extensions/ext-menus.js
+++ b/browser/components/extensions/ext-menus.js
@@ -210,19 +210,17 @@ var gMenuBuilder = {
         }
 
         label = label.replace(/%s/g, selection);
       }
 
       element.setAttribute("label", label);
     }
 
-    if (item.id && item.extension && item.extension.id) {
-      element.setAttribute("id", `${makeWidgetId(item.extension.id)}_${item.id}`);
-    }
+    element.setAttribute("id", item.elementId);
 
     if (item.icons) {
       this.setMenuItemIcon(element, item.extension, contextData, item.icons);
     }
 
     if (item.type == "checkbox") {
       element.setAttribute("type", "checkbox");
       if (item.checked) {
@@ -459,16 +457,28 @@ MenuItem.prototype = {
     }
     this._id = id;
   },
 
   get id() {
     return this._id;
   },
 
+  get elementId() {
+    let id = this.id;
+    // If the ID is an integer, it is auto-generated and globally unique.
+    // If the ID is a string, it is only unique within one extension and the
+    // ID needs to be concatenated with the extension ID.
+    if (typeof id !== "number") {
+      // To avoid collisions with numeric IDs, add a prefix to string IDs.
+      id = `_${id}`;
+    }
+    return `${makeWidgetId(this.extension.id)}-menuitem-${id}`;
+  },
+
   ensureValidParentId(parentId) {
     if (parentId === undefined) {
       return;
     }
     let menuMap = gMenuMap.get(this.extension);
     if (!menuMap.has(parentId)) {
       throw new Error("Could not find any MenuItem with id: " + parentId);
     }
--- a/browser/components/extensions/test/browser/browser_ext_menus.js
+++ b/browser/components/extensions/test/browser/browser_ext_menus.js
@@ -74,17 +74,17 @@ add_task(async function test_actionConte
 
     is(submenu.tagName, "menu", "Correct submenu type");
     is(submenu.label, "parent", "Correct submenu title");
 
     const popup = await openSubmenu(submenu);
     is(popup, submenu.firstChild, "Correct submenu opened");
     is(popup.children.length, 2, "Correct number of submenu items");
 
-    let idPrefix = `${makeWidgetId(extension.id)}_`;
+    let idPrefix = `${makeWidgetId(extension.id)}-menuitem-_`;
 
     is(second.tagName, "menuitem", "Second menu item type is correct");
     is(second.label, "click 1", "Second menu item title is correct");
     is(second.id, `${idPrefix}1`, "Second menu item id is correct");
 
     is(last.label, "click 5", "Last menu item title is correct");
     is(last.id, `${idPrefix}5`, "Last menu item id is correct");
     is(separator.tagName, "menuseparator", "Separator after last menu item");