Bug 1588773 - Use ContentDOMReference for context menu Inspect Element r=mconley,pbro

Depends on D49941

Using ContentDOMReference instead of creating an array of selectors makes inspect element more stable in case the page is modified between after the contextmenu opens.
It will also make the feature easier to make fission compatible

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

--HG--
extra : moz-landing-system : lando

browser/actors/ContextMenuChild.jsm

@@ -18,7 +18,6 @@ XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);
 XPCOMUtils.defineLazyModuleGetters(this, {
   E10SUtils: "resource://gre/modules/E10SUtils.jsm",
   BrowserUtils: "resource://gre/modules/BrowserUtils.jsm",
-  findAllCssSelectors: "resource://gre/modules/css-selector.js",
   SpellCheckHelper: "resource://gre/modules/InlineSpellChecker.jsm",
   LoginManagerChild: "resource://gre/modules/LoginManagerChild.jsm",
   WebNavigationFrames: "resource://gre/modules/WebNavigationFrames.jsm",
@@ -609,7 +608,6 @@ class ContextMenuChild extends JSWindowActorChild {
     let selectionInfo = BrowserUtils.getSelectionDetails(this.contentWindow);
     let loadContext = this.docShell.QueryInterface(Ci.nsILoadContext);
     let userContextId = loadContext.originAttributes.userContextId;
-    let popupNodeSelectors = findAllCssSelectors(aEvent.composedTarget);
 
     this._setContext(aEvent);
     let context = this.context;
@@ -678,7 +676,6 @@ class ContextMenuChild extends JSWindowActorChild {
       customMenuItems,
       contentDisposition,
       frameOuterWindowID,
-      popupNodeSelectors,
       disableSetDesktopBackground,
       parentAllowsMixedContent,
     };

browser/base/content/nsContextMenu.js

@@ -58,7 +58,6 @@ function openContextMenu(aMessage, aBrowser, aActor) {
 
   nsContextMenu.contentData = {
     context: data.context,
-    popupNodeSelectors: data.popupNodeSelectors,
     browser,
     actor,
     editFlags: data.editFlags,
@@ -268,12 +267,6 @@ class nsContextMenu {
 
     this.csp = E10SUtils.deserializeCSP(context.csp);
 
-    // Remember the CSS selectors corresponding to clicked node. this.contentData
-    // can be null if the menu was triggered by tests in which case use an empty array.
-    this.targetSelectors = this.contentData
-      ? this.contentData.popupNodeSelectors
-      : [];
-
     if (this.contentData) {
       this.browser = this.contentData.browser;
       this.selectionInfo = this.contentData.selectionInfo;
@@ -1031,14 +1024,14 @@ class nsContextMenu {
   inspectNode() {
     return nsContextMenu.DevToolsShim.inspectNode(
       gBrowser.selectedTab,
-      this.targetSelectors
+      this.targetIdentifier
     );
   }
 
   inspectA11Y() {
     return nsContextMenu.DevToolsShim.inspectA11Y(
       gBrowser.selectedTab,
-      this.targetSelectors
+      this.targetIdentifier
     );
   }
 

devtools/client/framework/devtools.js

@@ -705,18 +705,16 @@ DevTools.prototype = {
    *
    * @param {XULTab} tab
    *        The browser tab on which inspect node was used.
-   * @param {Array} selectors
-   *        An array of CSS selectors to find the target node. Several selectors can be
-   *        needed if the element is nested in frames and not directly in the root
-   *        document. The selectors are ordered starting with the root document and
-   *        ending with the deepest nested frame.
+   * @param {ElementIdentifier} domReference
+   *        Identifier generated by ContentDOMReference. It is a unique pair of
+   *        BrowsingContext ID and a numeric ID.
    * @param {Number} startTime
    *        Optional, indicates the time at which the user event related to this node
    *        inspection started. This is a `Cu.now()` timing.
    * @return {Promise} a promise that resolves when the node is selected in the inspector
    *         markup view.
    */
-  async inspectNode(tab, nodeSelectors, startTime) {
+  async inspectNode(tab, domReference, startTime) {
     const target = await TargetFactory.forTab(tab);
 
     const toolbox = await gDevTools.showToolbox(
@@ -733,7 +731,10 @@ DevTools.prototype = {
     // browser is remote or not.
     const onNewNode = inspector.selection.once("new-node-front");
 
-    const nodeFront = await inspector.walker.findNodeFront(nodeSelectors);
+    const nodeFront = await inspector.walker.getNodeActorFromContentDomReference(
+      domReference
+    );
+
     // Select the final node
     inspector.selection.setNodeFront(nodeFront, {
       reason: "browser-context-menu",
@@ -750,17 +751,16 @@ DevTools.prototype = {
    *
    * @param {XULTab} tab
    *        The browser tab on which inspect accessibility was used.
-   * @param {Array} selectors
-   *        An array of CSS selectors to find the target accessible object.
-   *        Several selectors can be needed if the element is nested in frames
-   *        and not directly in the root document.
+   * @param {ElementIdentifier} domReference
+   *        Identifier generated by ContentDOMReference. It is a unique pair of
+   *        BrowsingContext ID and a numeric ID.
    * @param {Number} startTime
    *        Optional, indicates the time at which the user event related to this
    *        node inspection started. This is a `Cu.now()` timing.
    * @return {Promise} a promise that resolves when the accessible object is
    *         selected in the accessibility inspector.
    */
-  async inspectA11Y(tab, nodeSelectors, startTime) {
+  async inspectA11Y(tab, domReference, startTime) {
     const target = await TargetFactory.forTab(tab);
 
     const toolbox = await gDevTools.showToolbox(
@@ -771,7 +771,9 @@ DevTools.prototype = {
       startTime
     );
     const inspectorFront = await toolbox.target.getFront("inspector");
-    const nodeFront = await inspectorFront.walker.findNodeFront(nodeSelectors);
+    const nodeFront = await inspectorFront.walker.getNodeActorFromContentDomReference(
+      domReference
+    );
     // Select the accessible object in the panel and wait for the event that
     // tells us it has been done.
     const a11yPanel = toolbox.getCurrentPanel();

devtools/client/inspector/test/browser.ini

@@ -149,6 +149,7 @@ skip-if = (os == 'win' && !debug) # Bug 1449754
 [browser_inspector_initialization.js]
 skip-if = (e10s && debug) # Bug 1250058 - Docshell leak on debug e10s
 [browser_inspector_inspect-object-element.js]
+[browser_inspector_inspect_mutated_node.js]
 [browser_inspector_inspect_node_contextmenu.js]
 [browser_inspector_invalidate.js]
 [browser_inspector_keyboard-shortcuts-copy-outerhtml.js]

devtools/client/inspector/test/browser_inspector_inspect_mutated_node.js

@@ -0,0 +1,75 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+/* globals getTestActorWithoutToolbox */
+
+"use strict";
+
+// Check that Inspect Element works even if the selector of the inspected
+// element changes after opening the context menu.
+// For this test, we explicitly move the element when opening the context menu.
+
+const TEST_URL =
+  `data:text/html;charset=utf-8,` +
+  encodeURIComponent(`
+
+  <div id="parent-1">
+    <span>Inspect me</span>
+  </div>
+  <div id="parent-2"></div>
+  <div id="changing-id">My ID will change</div>
+
+  <script type="text/javascript">
+    document.querySelector("span").addEventListener("contextmenu", () => {
+      // Right after opening the context menu, move the target element in the
+      // tree to change its selector.
+      window.setTimeout(() => {
+        const span = document.querySelector("span");
+        document.getElementById("parent-2").appendChild(span);
+      }, 0)
+    });
+
+    document.querySelector("#changing-id").addEventListener("contextmenu", () => {
+      // Right after opening the context menu, update the id of #changing-id
+      // to make sure we are not relying on outdated selectors to find the node
+      window.setTimeout(() => {
+        document.querySelector("#changing-id").id= "new-id";
+      }, 0)
+    });
+  </script>
+`);
+
+add_task(async function() {
+  const tab = await addTab(TEST_URL);
+  const testActor = await getTestActorWithoutToolbox(tab);
+
+  await testNodeWithChangingPath(testActor);
+  await testNodeWithChangingId(testActor);
+});
+
+async function testNodeWithChangingPath(testActor) {
+  info("Test that inspect element works if the CSS path changes");
+  const inspector = await clickOnInspectMenuItem(testActor, "span");
+
+  const selectedNode = inspector.selection.nodeFront;
+  ok(selectedNode, "A node is selected in the inspector");
+  is(
+    selectedNode.tagName.toLowerCase(),
+    "span",
+    "The selected node is correct"
+  );
+  const parentNode = await selectedNode.parentNode();
+  is(
+    parentNode.id,
+    "parent-2",
+    "The selected node is under the expected parent node"
+  );
+}
+
+async function testNodeWithChangingId(testActor) {
+  info("Test that inspect element works if the id changes");
+  const inspector = await clickOnInspectMenuItem(testActor, "#changing-id");
+
+  const selectedNode = inspector.selection.nodeFront;
+  ok(selectedNode, "A node is selected in the inspector");
+  is(selectedNode.id.toLowerCase(), "new-id", "The selected node is correct");
+}

devtools/startup/DevToolsShim.jsm

@@ -197,15 +197,14 @@ this.DevToolsShim = {
    *
    * @param {XULTab} tab
    *        The browser tab on which inspect accessibility was used.
-   * @param {Array} selectors
-   *        An array of CSS selectors to find the target accessible object. Several
-   *        selectors can be needed if the element is nested in frames and not directly
-   *        in the root document.
+   * @param {ElementIdentifier} domReference
+   *        Identifier generated by ContentDOMReference. It is a unique pair of
+   *        BrowsingContext ID and a numeric ID.
    * @return {Promise} a promise that resolves when the accessible node is selected in the
    *         accessibility inspector or that resolves immediately if DevTools are not
    *         enabled.
    */
-  inspectA11Y: function(tab, selectors) {
+  inspectA11Y: function(tab, domReference) {
     if (!this.isEnabled()) {
       if (!this.isDisabledByPolicy()) {
         DevtoolsStartup.openInstallPage("ContextMenu");
@@ -220,7 +219,7 @@ this.DevToolsShim = {
 
     this.initDevTools("ContextMenu");
 
-    return this._gDevTools.inspectA11Y(tab, selectors, startTime);
+    return this._gDevTools.inspectA11Y(tab, domReference, startTime);
   },
 
   /**
@@ -229,15 +228,13 @@ this.DevToolsShim = {
    *
    * @param {XULTab} tab
    *        The browser tab on which inspect node was used.
-   * @param {Array} selectors
-   *        An array of CSS selectors to find the target node. Several selectors can be
-   *        needed if the element is nested in frames and not directly in the root
-   *        document. The selectors are ordered starting with the root document and
-   *        ending with the deepest nested frame.
+   * @param {ElementIdentifier} domReference
+   *        Identifier generated by ContentDOMReference. It is a unique pair of
+   *        BrowsingContext ID and a numeric ID.
    * @return {Promise} a promise that resolves when the node is selected in the inspector
    *         markup view or that resolves immediately if DevTools are not enabled.
    */
-  inspectNode: function(tab, selectors) {
+  inspectNode: function(tab, domReference) {
     if (!this.isEnabled()) {
       if (!this.isDisabledByPolicy()) {
         DevtoolsStartup.openInstallPage("ContextMenu");
@@ -252,7 +249,7 @@ this.DevToolsShim = {
 
     this.initDevTools("ContextMenu");
 
-    return this._gDevTools.inspectNode(tab, selectors, startTime);
+    return this._gDevTools.inspectNode(tab, domReference, startTime);
   },
 
   _onDevToolsRegistered: function() {