Bug 1139644 - Flash only relevant attributes in markup view when changed;r=pbrosset

browser/devtools/markupview/markup-view.css

@@ -166,7 +166,7 @@ ul.children + .tag-line::before {
   margin-right: 0;
 }
 
-.tag-state.flash-out {
+.flash-out {
   transition: background .5s;
 }
 

browser/devtools/markupview/markup-view.js

@@ -781,11 +781,16 @@ MarkupView.prototype = {
     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
@@ -1884,47 +1889,17 @@ MarkupContainer.prototype = {
   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,
 
   /**
@@ -2351,6 +2326,7 @@ function ElementEditor(aContainer, aNode) {
   this.doc = this.markup.doc;
 
   this.attrs = {};
+  this.animationTimers = {};
 
   // The templates will fill the following properties
   this.elt = null;
@@ -2408,9 +2384,23 @@ function ElementEditor(aContainer, aNode) {
   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.
    */
@@ -2425,9 +2415,9 @@ ElementEditor.prototype = {
       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);
+      let canSimplyShowEditor = el && (!valueChanged || isEditing);
 
-      if (needToCreateAttributeEditor) {
+      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);
@@ -2437,6 +2427,13 @@ ElementEditor.prototype = {
         // 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);
+        }
       }
     }
 
@@ -2709,7 +2706,12 @@ ElementEditor.prototype = {
     });
   },
 
-  destroy: function() {}
+  destroy: function() {
+    for (let key in this.animationTimers) {
+      clearTimeout(this.animationTimers[key]);
+    }
+    this.animationTimers = null;
+  }
 };
 
 function nodeDocument(node) {
@@ -2763,6 +2765,62 @@ function parseAttributeValues(attr, doc) {
   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.
  */

browser/devtools/markupview/test/browser_markupview_mutation_02.js

@@ -13,6 +13,8 @@ const TEST_URL = TEST_URL_ROOT + "doc_markup_flashing.html";
 // 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
@@ -36,15 +38,28 @@ const TEST_DATA = [{
   },
   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) => {
@@ -66,7 +81,7 @@ add_task(function*() {
   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");
@@ -80,7 +95,12 @@ add_task(function*() {
     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;
@@ -97,4 +117,18 @@ function* assertNodeFlashing(nodeFront, inspector) {
   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");
 }