Bug 1023116 - Always briefly highlight nodes upon selection in the markup-view; r=bgrins
authorPatrick Brosset <pbrosset@mozilla.com>
Mon, 23 Jun 2014 17:20:01 +0200
changeset 190175 c4388b880938e9e86ff5d3cd2c740d29e89dea2b
parent 190174 943bd08b116aa6a62025da75b18e68472c9c4c17
child 190176 c5148c587ba88c950eba3a152c6005642cf3c537
push id27002
push userkwierso@gmail.com
push dateTue, 24 Jun 2014 00:56:44 +0000
treeherdermozilla-central@fa6a7a2f476c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbgrins
bugs1023116
milestone33.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1023116 - Always briefly highlight nodes upon selection in the markup-view; r=bgrins
browser/devtools/markupview/markup-view.js
browser/devtools/markupview/test/browser.ini
browser/devtools/markupview/test/browser_markupview_highlight_hover_03.js
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -151,31 +151,38 @@ MarkupView.prototype = {
       } else {
         this._hideBoxModel();
       }
     }
     this._showContainerAsHovered(container.node);
   },
 
   _hoveredNode: null,
+
+  /**
+   * Show a NodeFront's container as being hovered
+   * @param {NodeFront} nodeFront The node to show as hovered
+   */
   _showContainerAsHovered: function(nodeFront) {
-    if (this._hoveredNode !== nodeFront) {
-      if (this._hoveredNode) {
-        this._containers.get(this._hoveredNode).hovered = false;
-      }
-      this._containers.get(nodeFront).hovered = true;
+    if (this._hoveredNode === nodeFront) {
+      return;
+    }
 
-      this._hoveredNode = nodeFront;
+    if (this._hoveredNode) {
+      this.getContainer(this._hoveredNode).hovered = false;
     }
+
+    this.getContainer(nodeFront).hovered = true;
+    this._hoveredNode = nodeFront;
   },
 
   _onMouseLeave: function() {
     this._hideBoxModel(true);
     if (this._hoveredNode) {
-      this._containers.get(this._hoveredNode).hovered = false;
+      this.getContainer(this._hoveredNode).hovered = false;
     }
     this._hoveredNode = null;
   },
 
   /**
    * Show the box model highlighter on a given node front
    * @param {NodeFront} nodeFront The node to show the highlighter for
    * @param {Object} options Options for the highlighter
@@ -300,16 +307,21 @@ MarkupView.prototype = {
 
   /**
    * Highlight the inspector selected node.
    */
   _onNewSelection: function() {
     let selection = this._inspector.selection;
 
     this.htmlEditor.hide();
+    if (this._hoveredNode && this._hoveredNode !== selection.nodeFront) {
+      this.getContainer(this._hoveredNode).hovered = false;
+      this._hoveredNode = null;
+    }
+
     let done = this._inspector.updating("markup-view");
     if (selection.isNode()) {
       if (this._shouldNewSelectionBeHighlighted()) {
         this._brieflyShowBoxModel(selection.nodeFront, {});
       }
 
       this.showNode(selection.nodeFront, true).then(() => {
         if (selection.reason !== "treepanel") {
@@ -372,17 +384,17 @@ MarkupView.prototype = {
           }
         }
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
       case Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE:
         this.deleteNode(this._selectedContainer.node);
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_HOME:
-        let rootContainer = this._containers.get(this._rootNode);
+        let rootContainer = this.getContainer(this._rootNode);
         this.navigate(rootContainer.children.firstChild.container);
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_LEFT:
         if (this._selectedContainer.expanded) {
           this.collapseNode(this._selectedContainer.node);
         } else {
           let parent = this._selectionWalker().parentNode();
           if (parent) {
@@ -457,25 +469,25 @@ MarkupView.prototype = {
    * This is an undoable action.
    */
   deleteNode: function(aNode) {
     if (aNode.isDocumentElement ||
         aNode.nodeType == Ci.nsIDOMNode.DOCUMENT_TYPE_NODE) {
       return;
     }
 
-    let container = this._containers.get(aNode);
+    let container = this.getContainer(aNode);
 
     // Retain the node so we can undo this...
     this.walker.retainNode(aNode).then(() => {
       let parent = aNode.parentNode();
       let sibling = null;
       this.undo.do(() => {
         if (container.selected) {
-          this.navigate(this._containers.get(parent));
+          this.navigate(this.getContainer(parent));
         }
         this.walker.removeNode(aNode).then(nextSibling => {
           sibling = nextSibling;
         });
       }, () => {
         this.walker.insertBefore(aNode, parent, sibling);
       });
     }).then(null, console.error);
@@ -526,17 +538,17 @@ MarkupView.prototype = {
    * @returns MarkupContainer The MarkupContainer object for this element.
    */
   importNode: function(aNode, aFlashNode) {
     if (!aNode) {
       return null;
     }
 
     if (this._containers.has(aNode)) {
-      return this._containers.get(aNode);
+      return this.getContainer(aNode);
     }
 
     if (aNode === this.walker.rootNode) {
       var container = new RootContainer(this, aNode);
       this._elt.appendChild(container.elt);
       this._rootNode = aNode;
     } else {
       var container = new MarkupContainer(this, aNode, this._inspector);
@@ -570,17 +582,17 @@ MarkupView.prototype = {
         // should do this).
         type = "childList";
         target = mutation.targetParent;
         if (!target) {
           continue;
         }
       }
 
-      let container = this._containers.get(target);
+      let container = this.getContainer(target);
       if (!container) {
         // Container might not exist if this came from a load event for a node
         // we're not viewing.
         continue;
       }
       if (type === "attributes" || type === "characterData") {
         container.update();
 
@@ -619,17 +631,17 @@ MarkupView.prototype = {
       // the location in which it should be shown.
       this.htmlEditor.refresh();
 
       // If a node has had its outerHTML set, the parent node will be selected.
       // Reselect the original node immediately.
       if (this._inspector.selection.nodeFront === reselectParent) {
         this.walker.children(reselectParent).then((o) => {
           let node = o.nodes[reselectChildIndex];
-          let container = this._containers.get(node);
+          let container = this.getContainer(node);
           if (node && container) {
             this.markNodeAsSelected(node, "outerhtml");
             if (container.hasChildren) {
               this.expandNode(node);
             }
           }
         });
 
@@ -638,46 +650,46 @@ MarkupView.prototype = {
   },
 
   /**
    * React to display-change events from the walker
    * @param {Array} nodes An array of nodeFronts
    */
   _onDisplayChange: function(nodes) {
     for (let node of nodes) {
-      let container = this._containers.get(node);
+      let container = this.getContainer(node);
       if (container) {
         container.isDisplayed = node.isDisplayed;
       }
     }
   },
 
   /**
    * Given a list of mutations returned by the mutation observer, flash the
    * corresponding containers to attract attention.
    */
   _flashMutatedNodes: function(aMutations) {
     let addedOrEditedContainers = new Set();
     let removedContainers = new Set();
 
     for (let {type, target, added, removed} of aMutations) {
-      let container = this._containers.get(target);
+      let container = this.getContainer(target);
 
       if (container) {
         if (type === "attributes" || type === "characterData") {
           addedOrEditedContainers.add(container);
         } else if (type === "childList") {
           // If there has been removals, flash the parent
           if (removed.length) {
             removedContainers.add(container);
           }
 
           // If there has been additions, flash the nodes
           added.forEach(added => {
-            let addedContainer = this._containers.get(added);
+            let addedContainer = this.getContainer(added);
             addedOrEditedContainers.add(addedContainer);
 
             // The node may be added as a result of an append, in which case it
             // it will have been removed from another container first, but in
             // these cases we don't want to flash both the removal and the
             // addition
             removedContainers.delete(container);
           });
@@ -706,34 +718,34 @@ MarkupView.prototype = {
       this.importNode(parent);
       this.expandNode(parent);
     }
 
     return this._waitForChildren().then(() => {
       return this._ensureVisible(aNode);
     }).then(() => {
       // Why is this not working?
-      this.layoutHelpers.scrollIntoViewIfNeeded(this._containers.get(aNode).editor.elt, centered);
+      this.layoutHelpers.scrollIntoViewIfNeeded(this.getContainer(aNode).editor.elt, centered);
     });
   },
 
   /**
    * Expand the container's children.
    */
   _expandContainer: function(aContainer) {
     return this._updateChildren(aContainer, {expand: true}).then(() => {
       aContainer.expanded = true;
     });
   },
 
   /**
    * Expand the node's children.
    */
   expandNode: function(aNode) {
-    let container = this._containers.get(aNode);
+    let container = this.getContainer(aNode);
     this._expandContainer(container);
   },
 
   /**
    * Expand the entire tree beneath a container.
    *
    * @param aContainer The container to expand.
    */
@@ -752,24 +764,24 @@ MarkupView.prototype = {
   /**
    * Expand the entire tree beneath a node.
    *
    * @param aContainer The node to expand, or null
    *        to start from the top.
    */
   expandAll: function(aNode) {
     aNode = aNode || this._rootNode;
-    return this._expandAll(this._containers.get(aNode));
+    return this._expandAll(this.getContainer(aNode));
   },
 
   /**
    * Collapse the node's children.
    */
   collapseNode: function(aNode) {
-    let container = this._containers.get(aNode);
+    let container = this.getContainer(aNode);
     container.expanded = false;
   },
 
   /**
    * Retrieve the outerHTML for a remote node.
    * @param aNode The NodeFront to get the outerHTML for.
    * @returns A promise that will be resolved with the outerHTML.
    */
@@ -811,17 +823,17 @@ MarkupView.prototype = {
    * Retrieve the index of a child within its parent's children collection.
    * @param aNode The NodeFront to find the index of.
    * @param newValue The new outerHTML to set on the node.
    * @param oldValue The old outerHTML that will be reverted to find the index of.
    * @returns A promise that will be resolved with the integer index.
    *          If the child cannot be found, returns -1
    */
   updateNodeOuterHTML: function(aNode, newValue, oldValue) {
-    let container = this._containers.get(aNode);
+    let container = this.getContainer(aNode);
     if (!container) {
       return;
     }
 
     this.getNodeChildIndex(aNode).then((i) => {
       this._outerHTMLChildIndex = i;
       this._outerHTMLNode = aNode;
 
@@ -834,17 +846,17 @@ MarkupView.prototype = {
   },
 
   /**
    * Open an editor in the UI to allow editing of a node's outerHTML.
    * @param aNode The NodeFront to edit.
    */
   beginEditingOuterHTML: function(aNode) {
     this.getNodeOuterHTML(aNode).then((oldValue)=> {
-      let container = this._containers.get(aNode);
+      let container = this.getContainer(aNode);
       if (!container) {
         return;
       }
       this.htmlEditor.show(container.tagLine, oldValue);
       this.htmlEditor.once("popuphidden", (e, aCommit, aValue) => {
         // Need to focus the <html> element instead of the frame / window
         // in order to give keyboard focus back to doc (from editor).
         this._frame.contentDocument.documentElement.focus();
@@ -875,17 +887,17 @@ MarkupView.prototype = {
   },
 
   /**
    * Mark the given node selected, and update the inspector.selection
    * object's NodeFront to keep consistent state between UI and selection.
    * @param aNode The NodeFront to mark as selected.
    */
   markNodeAsSelected: function(aNode, reason) {
-    let container = this._containers.get(aNode);
+    let container = this.getContainer(aNode);
     if (this._selectedContainer === container) {
       return false;
     }
     if (this._selectedContainer) {
       this._selectedContainer.selected = false;
     }
     this._selectedContainer = container;
     if (aNode) {
@@ -897,20 +909,20 @@ MarkupView.prototype = {
   },
 
   /**
    * Make sure that every ancestor of the selection are updated
    * and included in the list of visible children.
    */
   _ensureVisible: function(node) {
     while (node) {
-      let container = this._containers.get(node);
+      let container = this.getContainer(node);
       let parent = node.parentNode();
       if (!container.elt.parentNode) {
-        let parentContainer = this._containers.get(parent);
+        let parentContainer = this.getContainer(parent);
         if (parentContainer) {
           parentContainer.childrenDirty = true;
           this._updateChildren(parentContainer, {expand: node});
         }
       }
 
       node = parent;
     }
--- a/browser/devtools/markupview/test/browser.ini
+++ b/browser/devtools/markupview/test/browser.ini
@@ -15,16 +15,17 @@ support-files =
   head.js
   helper_attributes_test_runner.js
   helper_outerhtml_test_runner.js
 
 [browser_markupview_copy_image_data.js]
 [browser_markupview_css_completion_style_attribute.js]
 [browser_markupview_highlight_hover_01.js]
 [browser_markupview_highlight_hover_02.js]
+[browser_markupview_highlight_hover_03.js]
 [browser_markupview_html_edit_01.js]
 [browser_markupview_html_edit_02.js]
 [browser_markupview_html_edit_03.js]
 [browser_markupview_image_tooltip.js]
 [browser_markupview_mutation_01.js]
 [browser_markupview_mutation_02.js]
 [browser_markupview_navigation.js]
 [browser_markupview_node_not_displayed_01.js]
new file mode 100644
--- /dev/null
+++ b/browser/devtools/markupview/test/browser_markupview_highlight_hover_03.js
@@ -0,0 +1,54 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that once a node has been hovered over and marked as such, if it is
+// navigated away using the keyboard, the highlighter moves to the new node, and
+// if it is then navigated back to, it is briefly highlighted again
+
+const TEST_PAGE = "data:text/html;charset=utf-8," +
+                  "<p id=\"one\">one</p><p id=\"two\">two</p>";
+
+let test = asyncTest(function*() {
+  let {inspector} = yield addTab(TEST_PAGE).then(openInspector);
+
+  info("Making sure the markup-view frame is focused");
+  inspector.markup._frame.focus();
+
+  // Mock the highlighter to easily track which node gets highlighted.
+  // We don't need to test here that the highlighter is actually visible, we
+  // just care about whether the markup-view asks it to be shown
+  let highlightedNode = null;
+  inspector.toolbox._highlighter.showBoxModel = function(nodeFront) {
+    highlightedNode = nodeFront;
+    return promise.resolve();
+  };
+  inspector.toolbox._highlighter.hideBoxModel = function() {
+    return promise.resolve();
+  };
+
+  function isHighlighting(node, desc) {
+    is(highlightedNode, getContainerForRawNode(node, inspector).node, desc);
+  }
+
+  info("Hover over <p#one> line in the markup-view");
+  yield hoverContainer("#one", inspector);
+  isHighlighting(getNode("#one"), "<p#one> is highlighted");
+
+  info("Navigate to <p#two> with the keyboard");
+  let onUpdated = inspector.once("inspector-updated");
+  EventUtils.synthesizeKey("VK_DOWN", {});
+  yield onUpdated;
+  let onUpdated = inspector.once("inspector-updated");
+  EventUtils.synthesizeKey("VK_DOWN", {});
+  yield onUpdated;
+  isHighlighting(getNode("#two"), "<p#two> is highlighted");
+
+  info("Navigate back to <p#one> with the keyboard");
+  let onUpdated = inspector.once("inspector-updated");
+  EventUtils.synthesizeKey("VK_UP", {});
+  yield onUpdated;
+  isHighlighting(getNode("#one"), "<p#one> is highlighted again");
+});