Bug 1680022 - Add idMap property to getViewUpdate and prohibit dynamic results...

Bug 1680022 - Add idMap property to getViewUpdate and prohibit dynamic results from setting their own IDs. r=adw, a=jcristau

Differential Revision: https://phabricator.services.mozilla.com/D100402

browser/components/urlbar/UrlbarProviderExtension.jsm

@@ -181,10 +181,13 @@ class UrlbarProviderExtension extends UrlbarProvider {
    * describing the view update.  See the base UrlbarProvider class for more.
    *
    * @param {UrlbarResult} result The result whose view will be updated.
+   * @param {Map} idsByName
+   *   A Map from an element's name, as defined by the provider; to its ID in
+   *   the DOM, as defined by the browser.
    * @returns {object} An object describing the view update.
    */
-  async getViewUpdate(result) {
-    return this._notifyListener("getViewUpdate", result);
+  async getViewUpdate(result, idsByName) {
+    return this._notifyListener("getViewUpdate", result, idsByName);
   }
 
   /**

browser/components/urlbar/UrlbarProviderTabToSearch.jsm

@@ -154,9 +154,12 @@ class ProviderTabToSearch extends UrlbarProvider {
    * describing the view update.
    *
    * @param {UrlbarResult} result The result whose view will be updated.
+   * @param {Map} idsByName
+   *   A Map from an element's name, as defined by the provider; to its ID in
+   *   the DOM, as defined by the browser.
    * @returns {object} An object describing the view update.
    */
-  getViewUpdate(result) {
+  getViewUpdate(result, idsByName) {
     return {
       icon: {
         attributes: {

browser/components/urlbar/UrlbarUtils.jsm

@@ -1640,6 +1640,12 @@ class UrlbarProvider {
    *
    * @param {UrlbarResult} result
    *   The result whose view will be updated.
+   * @param {Map} idsByName
+   *   A Map from an element's name, as defined by the provider; to its ID in
+   *   the DOM, as defined by the browser. The browser manages element IDs for
+   *   dynamic results to prevent collisions. However, a provider may need to
+   *   access the IDs of the elements created for its results. For example, to
+   *   set various `aria` attributes.
    * @returns {object}
    *   A view update object as described above.  The names of properties are the
    *   the names of elements declared in the view template.  The values of
@@ -1649,7 +1655,8 @@ class UrlbarProvider {
    *
    *   {object} [attributes]
    *     A mapping from attribute names to values.  Each name-value pair results
-   *     in an attribute being added to the element.
+   *     in an attribute being added to the element.  The `id` attribute is
+   *     reserved and cannot be set by the provider.
    *   {object} [style]
    *     A plain object that can be used to add inline styles to the element,
    *     like `display: none`.   `element.style` is updated for each name-value
@@ -1660,7 +1667,7 @@ class UrlbarProvider {
    *   {string} [textContent]
    *     A string that will be set as `element.textContent`.
    */
-  getViewUpdate(result) {
+  getViewUpdate(result, idsByName) {
     return null;
   }
 

browser/components/urlbar/UrlbarView.jsm

@@ -800,7 +800,10 @@ class UrlbarView {
    *     Names must be unique within a view template, but they don't need to be
    *     globally unique.  i.e., two different view templates can use the same
    *     names, and other DOM elements can use the same names in their IDs and
-   *     classes.
+   *     classes.  The name also suffixes the dynamic element's ID: an element
+   *     with name `data` will get the ID `urlbarView-row-{unique number}-data`.
+   *     If there is no name provided for the root element, the root element
+   *     will not get an ID.
    *   {string} tag
    *     The tag name of the object.  It is required for all objects in the
    *     structure except the root object and declares the kind of element that
@@ -808,7 +811,9 @@ class UrlbarView {
    *   {object} [attributes]
    *     An optional mapping from attribute names to values.  For each
    *     name-value pair, an attribute is added to the element created for the
-   *     object.
+   *     object. The `id` attribute is reserved and cannot be set by the
+   *     provider. Element IDs are passed back to the provider in getViewUpdate
+   *     if they are needed.
    *   {array} [children]
    *     An optional list of children.  Each item in the array must be an object
    *     as described here.  For each item, a child element as described by the
@@ -1106,27 +1111,41 @@ class UrlbarView {
   _createRowContentForDynamicType(item, result) {
     let { dynamicType } = result.payload;
     let viewTemplate = UrlbarView.dynamicViewTemplatesByName.get(dynamicType);
-    this._buildViewForDynamicType(dynamicType, item._content, viewTemplate);
+    this._buildViewForDynamicType(
+      dynamicType,
+      item._content,
+      item._elements,
+      viewTemplate
+    );
   }
 
-  _buildViewForDynamicType(type, parentNode, template) {
+  _buildViewForDynamicType(type, parentNode, elementsByName, template) {
     // Add classes to parentNode's classList.
     for (let className of template.classList || []) {
       parentNode.classList.add(className);
     }
     // Set attributes on parentNode.
     for (let [name, value] of Object.entries(template.attributes || {})) {
+      if (name == "id") {
+        // We do not allow dynamic results to set IDs for their Nodes. IDs are
+        // managed by the view to ensure they are unique.
+        Cu.reportError(
+          "Dynamic results are prohibited from setting their own IDs."
+        );
+        continue;
+      }
       parentNode.setAttribute(name, value);
     }
     if (template.name) {
       parentNode.setAttribute("name", template.name);
+      elementsByName.set(template.name, parentNode);
     }
     // Recurse into children.
     for (let childTemplate of template.children || []) {
       let child = this._createElement(childTemplate.tag);
       child.classList.add(`urlbarView-dynamic-${type}-${childTemplate.name}`);
       parentNode.appendChild(child);
-      this._buildViewForDynamicType(type, child, childTemplate);
+      this._buildViewForDynamicType(type, child, elementsByName, childTemplate);
     }
   }
 
@@ -1476,6 +1495,11 @@ class UrlbarView {
   async _updateRowForDynamicType(item, result) {
     item.setAttribute("dynamicType", result.payload.dynamicType);
 
+    let idsByName = new Map();
+    for (let [name, node] of item._elements) {
+      node.id = `${item.id}-${name}`;
+      idsByName.set(name, node.id);
+
     // First, apply highlighting. We do this before updating via getViewUpdate
     // so the dynamic provider can override the highlighting by setting the
     // textContent of the highlighted node, if it wishes.
@@ -1497,14 +1521,20 @@ class UrlbarView {
 
     // Get the view update from the result's provider.
     let provider = UrlbarProvidersManager.getProvider(result.providerName);
-    let viewUpdate = await provider.getViewUpdate(result);
+    let viewUpdate = await provider.getViewUpdate(result, idsByName);
 
     // Update each node in the view by name.
     for (let [nodeName, update] of Object.entries(viewUpdate)) {
-      let node = item.querySelector(
-        `.urlbarView-dynamic-${result.payload.dynamicType}-${nodeName}`
-      );
+      let node = item.querySelector(`#${item.id}-${nodeName}`);
       for (let [attrName, value] of Object.entries(update.attributes || {})) {
+        if (attrName == "id") {
+          // We do not allow dynamic results to set IDs for their Nodes. IDs are
+          // managed by the view to ensure they are unique.
+          Cu.reportError(
+            "Dynamic results are prohibited from setting their own IDs."
+          );
+          continue;
+        }
         node.setAttribute(attrName, value);
       }
       for (let [styleName, value] of Object.entries(update.style || {})) {

browser/components/urlbar/tests/browser/browser_dynamicResults.js

@@ -618,7 +618,11 @@ class TestProvider extends UrlbarTestUtils.TestProvider {
     }
   }
 
-  getViewUpdate(result) {
+  getViewUpdate(result, idsByName) {
+    for (let child of DYNAMIC_TYPE_VIEW_TEMPLATE.children) {
+      Assert.ok(idsByName.get(child.name), `idsByName contains ${child.name}`);
+    }
+
     return {
       selectable: {
         textContent: "Selectable",
@@ -707,6 +711,13 @@ function checkDOM(parentNode, expectedChildren) {
       ),
       "child.name should be in classList"
     );
+    // We have to use startsWith/endsWith since the middle of the ID is a random
+    // number.
+    Assert.ok(actualChild.id.startsWith("urlbarView-row-"));
+    Assert.ok(
+      actualChild.id.endsWith(child.name),
+      "The child was assigned the correct ID."
+    );
     for (let [name, value] of Object.entries(child.attributes || {})) {
       Assert.equal(actualChild.getAttribute(name), value, `attribute: ${name}`);
     }

browser/components/urlbar/tests/browser/browser_urlbar_telemetry_dynamic.js

@@ -39,7 +39,7 @@ class TestProvider extends UrlbarTestUtils.TestProvider {
     });
   }
 
-  getViewUpdate(result) {
+  getViewUpdate(result, idsByName) {
     return {
       title: {
         textContent: "This is a dynamic result.",

browser/components/urlbar/tests/ext/api.js

@@ -54,11 +54,14 @@ this.experiments_urlbar = class extends ExtensionAPI {
             name: "experiments.urlbar.onViewUpdateRequested",
             register: (fire, providerName) => {
               let provider = UrlbarProviderExtension.getOrCreate(providerName);
-              provider.setEventListener("getViewUpdate", result => {
-                return fire.async(result.payload).catch(error => {
-                  throw context.normalizeError(error);
-                });
-              });
+              provider.setEventListener(
+                "getViewUpdate",
+                (result, idsByName) => {
+                  return fire.async(result.payload, idsByName).catch(error => {
+                    throw context.normalizeError(error);
+                  });
+                }
+              );
               return () => provider.setEventListener("getViewUpdate", null);
             },
           }).api(),

browser/components/urlbar/tests/ext/schema.json

@@ -32,6 +32,11 @@
             "name": "payload",
             "type": "object",
             "description": "The result's payload."
+          },
+          {
+            "name": "idsByName",
+            "type": "object",
+            "description": "A Map from an element's name, as defined by the provider; to its ID in the DOM, as defined by the browser."
           }
         ],
         "extraParameters": [