Bug 1139644 - Flash only relevant attributes in markup view when changed;r=pbrosset
authorBrian Grinstead <bgrinstead@mozilla.com>
Tue, 24 Mar 2015 14:57:57 -0700
changeset 264353 e36d122aeef8be95def8a17de6738686b9dfd159
parent 264352 fb06cbc424420b3cc099f59aa23f2576d3ac6fa4
child 264354 603d839658dc555daca9f4e02a6fff6257eb473a
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1139644
milestone39.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 1139644 - Flash only relevant attributes in markup view when changed;r=pbrosset
browser/devtools/markupview/markup-view.css
browser/devtools/markupview/markup-view.js
browser/devtools/markupview/test/browser_markupview_mutation_02.js
--- a/browser/devtools/markupview/markup-view.css
+++ b/browser/devtools/markupview/markup-view.css
@@ -161,17 +161,17 @@ ul.children + .tag-line::before {
   margin-right: -1em;
   padding: 1px 0;
 }
 
 .newattr:focus {
   margin-right: 0;
 }
 
-.tag-state.flash-out {
+.flash-out {
   transition: background .5s;
 }
 
 .tag-line {
   cursor: default;
 }
 
 .markupview-events {
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -776,21 +776,26 @@ MarkupView.prototype = {
   /**
    * 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) {
+    for (let {type, target, added, removed, newValue} of aMutations) {
       let container = this.getContainer(target);
 
       if (container) {
-        if (type === "attributes" || type === "characterData") {
+        if (type === "characterData") {
+          addedOrEditedContainers.add(container);
+        } else if (type === "attributes" && newValue === null) {
+          // Removed attributes should flash the entire node.
+          // New or changed attributes will flash the attribute itself
+          // in ElementEditor.flashAttribute.
           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 if their associated
@@ -1879,57 +1884,27 @@ MarkupContainer.prototype = {
 
   /**
    * Temporarily flash the container to attract attention.
    * Used for markup mutations.
    */
   flashMutation: function() {
     if (!this.selected) {
       let contentWin = this.win;
-      this.flashed = true;
+      flashElementOn(this.tagState, this.editor.elt);
       if (this._flashMutationTimer) {
         clearTimeout(this._flashMutationTimer);
         this._flashMutationTimer = null;
       }
       this._flashMutationTimer = setTimeout(() => {
-        this.flashed = false;
+        flashElementOff(this.tagState, this.editor.elt);
       }, this.markup.CONTAINER_FLASHING_DURATION);
     }
   },
 
-  set flashed(aValue) {
-    if (aValue) {
-      // Make sure the animation class is not here
-      this.tagState.classList.remove("flash-out");
-
-      // Change the background
-      this.tagState.classList.add("theme-bg-contrast");
-
-      // Change the text color
-      this.editor.elt.classList.add("theme-fg-contrast");
-      [].forEach.call(
-        this.editor.elt.querySelectorAll("[class*=theme-fg-color]"),
-        span => span.classList.add("theme-fg-contrast")
-      );
-    } else {
-      // Add the animation class to smoothly remove the background
-      this.tagState.classList.add("flash-out");
-
-      // Remove the background
-      this.tagState.classList.remove("theme-bg-contrast");
-
-      // Remove the text color
-      this.editor.elt.classList.remove("theme-fg-contrast");
-      [].forEach.call(
-        this.editor.elt.querySelectorAll("[class*=theme-fg-color]"),
-        span => span.classList.remove("theme-fg-contrast")
-      );
-    }
-  },
-
   _hovered: false,
 
   /**
    * Highlight the currently hovered tag + its closing tag if necessary
    * (that is if the tag is expanded)
    */
   set hovered(aValue) {
     this.tagState.classList.remove("flash-out");
@@ -2346,16 +2321,17 @@ TextEditor.prototype = {
 function ElementEditor(aContainer, aNode) {
   this.container = aContainer;
   this.node = aNode;
   this.markup = this.container.markup;
   this.template = this.markup.template.bind(this.markup);
   this.doc = this.markup.doc;
 
   this.attrs = {};
+  this.animationTimers = {};
 
   // The templates will fill the following properties
   this.elt = null;
   this.tag = null;
   this.closeTag = null;
   this.attrList = null;
   this.newAttr = null;
   this.closeElt = null;
@@ -2403,45 +2379,66 @@ function ElementEditor(aContainer, aNode
   });
 
   let tagName = this.node.nodeName.toLowerCase();
   this.tag.textContent = tagName;
   this.closeTag.textContent = tagName;
   this.eventNode.style.display = this.node.hasEventListeners ? "inline-block" : "none";
 
   this.update();
+  this.initialized = true;
 }
 
 ElementEditor.prototype = {
+
+  flashAttribute: function(attrName) {
+    if (this.animationTimers[attrName]) {
+      clearTimeout(this.animationTimers[attrName]);
+    }
+
+    flashElementOn(this.getAttributeElement(attrName));
+
+    this.animationTimers[attrName] = setTimeout(() => {
+      flashElementOff(this.getAttributeElement(attrName));
+    }, this.markup.CONTAINER_FLASHING_DURATION);
+  },
+
   /**
    * Update the state of the editor from the node.
    */
   update: function() {
     let attrs = this.node.attributes || [];
     let attrsToRemove = new Set(this.attrList.querySelectorAll(".attreditor"));
 
     // Only loop through the current attributes on the node, anything that's
     // been removed will be removed from this DOM because it will be part of
     // the attrsToRemove set.
     for (let attr of attrs) {
       let el = this.attrs[attr.name];
       let valueChanged = el && el.querySelector(".attr-value").innerHTML !== attr.value;
       let isEditing = el && el.querySelector(".editable").inplaceEditor;
-      let needToCreateAttributeEditor = el && (!valueChanged || isEditing);
-
-      if (needToCreateAttributeEditor) {
+      let canSimplyShowEditor = el && (!valueChanged || isEditing);
+
+      if (canSimplyShowEditor) {
         // Element already exists and doesn't need to be recreated.
         // Just show it (it's hidden by default due to the template).
         attrsToRemove.delete(el);
         el.style.removeProperty("display");
       } else {
         // Create a new editor, because the value of an existing attribute
         // has changed.
         let attribute = this._createAttribute(attr);
         attribute.style.removeProperty("display");
+
+        // Temporarily flash the attribute to highlight the change.
+        // But not if this is the first time the editor instance has
+        // been created.
+        if (this.initialized) {
+          this.flashAttribute(attr.name);
+        }
       }
     }
 
     for (let el of attrsToRemove) {
       el.remove();
     }
   },
 
@@ -2704,17 +2701,22 @@ ElementEditor.prototype = {
     // selected afterwards.
     this.markup.reselectOnRemoved(this.node, "edittagname");
     this.markup.walker.editTagName(this.node, newTagName).then(null, () => {
       // Failed to edit the tag name, cancel the reselection.
       this.markup.cancelReselectOnRemoved();
     });
   },
 
-  destroy: function() {}
+  destroy: function() {
+    for (let key in this.animationTimers) {
+      clearTimeout(this.animationTimers[key]);
+    }
+    this.animationTimers = null;
+  }
 };
 
 function nodeDocument(node) {
   return node.ownerDocument ||
     (node.nodeType == Ci.nsIDOMNode.DOCUMENT_NODE ? node : null);
 }
 
 function truncateString(str, maxLength) {
@@ -2759,16 +2761,72 @@ function parseAttributeValues(attr, doc)
     catch(e) { }
   }
 
   // Attributes return from DOMParser in reverse order from how they are entered.
   return attributes.reverse();
 }
 
 /**
+ * Apply a 'flashed' background and foreground color to elements.  Intended
+ * to be used with flashElementOff as a way of drawing attention to an element.
+ *
+ * @param  {Node} backgroundElt
+ *         The element to set the highlighted background color on.
+ * @param  {Node} foregroundElt
+ *         The element to set the matching foreground color on.
+ *         Optional.  This will equal backgroundElt if not set.
+ */
+function flashElementOn(backgroundElt, foregroundElt=backgroundElt) {
+  if (!backgroundElt || !foregroundElt) {
+    return;
+  }
+
+  // Make sure the animation class is not here
+  backgroundElt.classList.remove("flash-out");
+
+  // Change the background
+  backgroundElt.classList.add("theme-bg-contrast");
+
+  foregroundElt.classList.add("theme-fg-contrast");
+  [].forEach.call(
+    foregroundElt.querySelectorAll("[class*=theme-fg-color]"),
+    span => span.classList.add("theme-fg-contrast")
+  );
+}
+
+/**
+ * Remove a 'flashed' background and foreground color to elements.
+ * See flashElementOn.
+ *
+ * @param  {Node} backgroundElt
+ *         The element to reomve the highlighted background color on.
+ * @param  {Node} foregroundElt
+ *         The element to remove the matching foreground color on.
+ *         Optional.  This will equal backgroundElt if not set.
+ */
+function flashElementOff(backgroundElt, foregroundElt=backgroundElt) {
+  if (!backgroundElt || !foregroundElt) {
+    return;
+  }
+
+  // Add the animation class to smoothly remove the background
+  backgroundElt.classList.add("flash-out");
+
+  // Remove the background
+  backgroundElt.classList.remove("theme-bg-contrast");
+
+  foregroundElt.classList.remove("theme-fg-contrast");
+  [].forEach.call(
+    foregroundElt.querySelectorAll("[class*=theme-fg-color]"),
+    span => span.classList.remove("theme-fg-contrast")
+  );
+}
+
+/**
  * Map a number from one range to another.
  */
 function map(value, oldMin, oldMax, newMin, newMax) {
   let ratio = oldMax - oldMin;
   if (ratio == 0) {
     return value;
   }
   return newMin + (newMax - newMin) * ((value - oldMin) / ratio);
--- a/browser/devtools/markupview/test/browser_markupview_mutation_02.js
+++ b/browser/devtools/markupview/test/browser_markupview_mutation_02.js
@@ -8,16 +8,18 @@
 // corresponding DOM nodes mutate
 
 const TEST_URL = TEST_URL_ROOT + "doc_markup_flashing.html";
 
 // The test data contains a list of mutations to test.
 // Each item is an object:
 // - desc: a description of the test step, for better logging
 // - mutate: a function that should make changes to the content DOM
+// - attribute: if set, the test will expect the corresponding attribute to flash
+//   instead of the whole node
 // - flashedNode: [optional] the css selector of the node that is expected to
 //   flash in the markup-view as a result of the mutation.
 //   If missing, the rootNode (".list") will be expected to flash
 const TEST_DATA = [{
   desc: "Adding a new node should flash the new node",
   mutate: (doc, rootNode) => {
     let newLi = doc.createElement("LI");
     newLi.textContent = "new list item";
@@ -31,26 +33,39 @@ const TEST_DATA = [{
   }
 }, {
   desc: "Re-appending an existing node should only flash this node",
   mutate: (doc, rootNode) => {
     rootNode.appendChild(rootNode.firstElementChild);
   },
   flashedNode: ".list .item:last-child"
 }, {
-  desc: "Adding an attribute should flash the node",
+  desc: "Adding an attribute should flash the attribute",
+  attribute: "test-name",
   mutate: (doc, rootNode) => {
-    rootNode.setAttribute("name-" + Date.now(), "value-" + Date.now());
+    rootNode.setAttribute("test-name", "value-" + Date.now());
   }
 }, {
-  desc: "Editing an attribute should flash the node",
+  desc: "Editing an attribute should flash the attribute",
+  attribute: "class",
   mutate: (doc, rootNode) => {
     rootNode.setAttribute("class", "list value-" + Date.now());
   }
 }, {
+  desc: "Multiple changes to an attribute should flash the attribute",
+  attribute: "class",
+  mutate: (doc, rootNode) => {
+    rootNode.removeAttribute("class");
+    rootNode.setAttribute("class", "list value-" + Date.now());
+    rootNode.setAttribute("class", "list value-" + Date.now());
+    rootNode.removeAttribute("class");
+    rootNode.setAttribute("class", "list value-" + Date.now());
+    rootNode.setAttribute("class", "list value-" + Date.now());
+  }
+}, {
   desc: "Removing an attribute should flash the node",
   mutate: (doc, rootNode) => {
     rootNode.removeAttribute("class");
   }
 }];
 
 add_task(function*() {
   let {inspector} = yield addTab(TEST_URL).then(openInspector);
@@ -61,40 +76,59 @@ add_task(function*() {
 
   info("Getting the <ul.list> root node to test mutations on");
   let rootNode = getNode(".list");
   let rootNodeFront = yield getNodeFront(".list", inspector);
 
   info("Selecting the last element of the root node before starting");
   yield selectNode(".list .item:nth-child(2)", inspector);
 
-  for (let {mutate, flashedNode, desc} of TEST_DATA) {
+  for (let {mutate, flashedNode, desc, attribute} of TEST_DATA) {
     info("Starting test: " + desc);
 
     info("Mutating the DOM and listening for markupmutation event");
     let mutated = inspector.once("markupmutation");
     let updated = inspector.once("inspector-updated");
     mutate(content.document, rootNode);
     yield mutated;
 
     info("Asserting that the correct markup-container is flashing");
     let flashingNodeFront = rootNodeFront;
     if (flashedNode) {
       flashingNodeFront = yield getNodeFront(flashedNode, inspector);
     }
-    yield assertNodeFlashing(flashingNodeFront, inspector);
+
+    if (attribute) {
+      yield assertAttributeFlashing(flashingNodeFront, attribute, inspector);
+    } else {
+      yield assertNodeFlashing(flashingNodeFront, inspector);
+    }
 
     // Making sure the inspector has finished updating before moving on
     yield updated;
   }
 });
 
 function* assertNodeFlashing(nodeFront, inspector) {
   let container = getContainerForNodeFront(nodeFront, inspector);
   ok(container, "Markup container for node found");
   ok(container.tagState.classList.contains("theme-bg-contrast"),
     "Markup container for node is flashing");
 
   // Clear the mutation flashing timeout now that we checked the node was flashing
   let markup = inspector.markup;
   clearTimeout(container._flashMutationTimer);
   container._flashMutationTimer = null;
+  container.tagState.classList.remove("theme-bg-contrast");
 }
+
+function* assertAttributeFlashing(nodeFront, attribute, inspector) {
+  let container = getContainerForNodeFront(nodeFront, inspector);
+  ok(container, "Markup container for node found");
+  ok(container.editor.attrs[attribute], "Attribute exists on editor");
+
+  let attributeElement = container.editor.getAttributeElement(attribute);
+
+  ok(attributeElement.classList.contains("theme-bg-contrast"),
+    "Element for " + attribute + " attribute is flashing");
+
+  attributeElement.classList.remove("theme-bg-contrast");
+}