Bug 1147128 - Make sure attribute shows up in markup view after removing and setting to the previous value;r=mratcliffe
authorBrian Grinstead <bgrinstead@mozilla.com>
Fri, 27 Mar 2015 07:30:09 -0700
changeset 235951 9630284e15e6ff436a1ec2b0a9c2e884eac69d04
parent 235950 697c11bd51f6ad213e375efa2e1fd2c253f98544
child 235952 fde5601c5dd4bf12138d70bb24644c90a49888f4
push id12035
push userbgrinstead@mozilla.com
push dateFri, 27 Mar 2015 14:30:21 +0000
treeherderfx-team@9630284e15e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmratcliffe
bugs1147128
milestone39.0a1
Bug 1147128 - Make sure attribute shows up in markup view after removing and setting to the previous value;r=mratcliffe
browser/devtools/markupview/markup-view.js
browser/devtools/markupview/test/browser_markupview_css_completion_style_attribute.js
browser/devtools/markupview/test/browser_markupview_mutation_01.js
browser/devtools/markupview/test/browser_markupview_mutation_02.js
browser/devtools/markupview/test/browser_markupview_tag_edit_02.js
browser/devtools/markupview/test/browser_markupview_tag_edit_07.js
browser/devtools/markupview/test/browser_markupview_tag_edit_08.js
browser/devtools/markupview/test/browser_markupview_tag_edit_09.js
browser/devtools/markupview/test/helper_attributes_test_runner.js
--- a/browser/devtools/markupview/markup-view.js
+++ b/browser/devtools/markupview/markup-view.js
@@ -2323,17 +2323,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.attrElements = new Map();
   this.animationTimers = {};
 
   // The templates will fill the following properties
   this.elt = null;
   this.tag = null;
   this.closeTag = null;
   this.attrList = null;
   this.newAttr = null;
@@ -2403,67 +2403,80 @@ ElementEditor.prototype = {
       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 nodeAttributes = this.node.attributes || [];
+
+    // Keep the data model in sync with attributes on the node.
+    let currentAttributes = new Set(nodeAttributes.map(a=>a.name));
+    for (let name of this.attrElements.keys()) {
+      if (!currentAttributes.has(name)) {
+        this.removeAttribute(name);
+      }
+    }
+
+    // Only loop through the current attributes on the node.  Missing
+    // attributes have already been removed at this point.
+    for (let attr of nodeAttributes) {
+      let el = this.attrElements.get(attr.name);
       let valueChanged = el && el.querySelector(".attr-value").innerHTML !== attr.value;
       let isEditing = el && el.querySelector(".editable").inplaceEditor;
       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();
-    }
   },
 
   _startModifyingAttributes: function() {
     return this.node.startModifyingAttributes();
   },
 
   /**
    * Get the element used for one of the attributes of this element
    * @param string attrName The name of the attribute to get the element for
    * @return DOMElement
    */
   getAttributeElement: function(attrName) {
     return this.attrList.querySelector(
       ".attreditor[data-attr=" + attrName + "] .attr-value");
   },
 
+  /**
+   * Remove an attribute from the attrElements object and the DOM
+   * @param string attrName The name of the attribute to remove
+   */
+  removeAttribute: function(attrName) {
+    let attr = this.attrElements.get(attrName);
+    if (attr) {
+      this.attrElements.delete(attrName);
+      attr.remove();
+    }
+  },
+
   _createAttribute: function(aAttr, aBefore = null) {
     // Create the template editor, which will save some variables here.
     let data = {
       attrName: aAttr.name,
     };
     this.template("attribute", data);
     var {attr, inner, name, val} = data;
 
@@ -2536,28 +2549,23 @@ ElementEditor.prototype = {
       }
     });
 
     // Figure out where we should place the attribute.
     let before = aBefore;
     if (aAttr.name == "id") {
       before = this.attrList.firstChild;
     } else if (aAttr.name == "class") {
-      let idNode = this.attrs["id"];
+      let idNode = this.attrElements.get("id");
       before = idNode ? idNode.nextSibling : this.attrList.firstChild;
     }
     this.attrList.insertBefore(attr, before);
 
-    // Remove the old version of this attribute from the DOM.
-    let oldAttr = this.attrs[aAttr.name];
-    if (oldAttr && oldAttr.parentNode) {
-      oldAttr.parentNode.removeChild(oldAttr);
-    }
-
-    this.attrs[aAttr.name] = attr;
+    this.removeAttribute(aAttr.name);
+    this.attrElements.set(aAttr.name, attr);
 
     let collapsedValue;
     if (aAttr.value.match(COLLAPSE_DATA_URL_REGEX)) {
       collapsedValue = truncateString(aAttr.value, COLLAPSE_DATA_URL_LENGTH);
     } else {
       collapsedValue = truncateString(aAttr.value, COLLAPSE_ATTRIBUTE_LENGTH);
     }
 
--- a/browser/devtools/markupview/test/browser_markupview_css_completion_style_attribute.js
+++ b/browser/devtools/markupview/test/browser_markupview_css_completion_style_attribute.js
@@ -135,12 +135,12 @@ function* checkData(index, editor, inspe
       ok(editor.popup.isOpen, "Popup is open");
     } else {
       ok(editor.popup._panel.state != "open" && editor.popup._panel.state != "showing",
         "Popup is closed");
     }
   } else {
     let nodeFront = yield getNodeFront("#node14", inspector);
     let editor = getContainerForNodeFront(nodeFront, inspector).editor;
-    let attr = editor.attrs["style"].querySelector(".editable");
+    let attr = editor.attrElements.get("style").querySelector(".editable");
     is(attr.textContent, completion, "Correct value is persisted after pressing Enter");
   }
 }
--- a/browser/devtools/markupview/test/browser_markupview_mutation_01.js
+++ b/browser/devtools/markupview/test/browser_markupview_mutation_01.js
@@ -35,16 +35,42 @@ const TEST_DATA = [
     check: function*(inspector) {
       let {editor} = yield getContainerForSelector("#node1", inspector);
       ok(![...editor.attrList.querySelectorAll(".attreditor")].some(attr => {
         return attr.textContent.trim() === "newattr=\"newattrval\"";
       }), "newattr attribute removed");
     }
   },
   {
+    desc: "Re-adding an attribute",
+    test: () => {
+      let node1 = getNode("#node1");
+      node1.setAttribute("newattr", "newattrval");
+    },
+    check: function*(inspector) {
+      let {editor} = yield getContainerForSelector("#node1", inspector);
+      ok([...editor.attrList.querySelectorAll(".attreditor")].some(attr => {
+        return attr.textContent.trim() === "newattr=\"newattrval\"";
+      }), "newattr attribute found");
+    }
+  },
+  {
+    desc: "Changing an attribute",
+    test: () => {
+      let node1 = getNode("#node1");
+      node1.setAttribute("newattr", "newattrchanged");
+    },
+    check: function*(inspector) {
+      let {editor} = yield getContainerForSelector("#node1", inspector);
+      ok([...editor.attrList.querySelectorAll(".attreditor")].some(attr => {
+        return attr.textContent.trim() === "newattr=\"newattrchanged\"";
+      }), "newattr attribute found");
+    }
+  },
+  {
     desc: "Updating the text-content",
     test: () => {
       let node1 = getNode("#node1");
       node1.textContent = "newtext";
     },
     check: function*(inspector) {
       let {children} = yield getContainerForSelector("#node1", inspector);
       is(children.querySelector(".text").textContent.trim(), "newtext",
--- a/browser/devtools/markupview/test/browser_markupview_mutation_02.js
+++ b/browser/devtools/markupview/test/browser_markupview_mutation_02.js
@@ -118,17 +118,17 @@ function* assertNodeFlashing(nodeFront, 
   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");
+  ok(container.editor.attrElements.get(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");
 }
--- a/browser/devtools/markupview/test/browser_markupview_tag_edit_02.js
+++ b/browser/devtools/markupview/test/browser_markupview_tag_edit_02.js
@@ -17,17 +17,17 @@ add_task(function*() {
 
   info("Verify attributes, only ID should be there for now");
   yield assertAttributes("#test-div", {
     id: "test-div"
   });
 
   info("Focus the ID attribute and change its content");
   let {editor} = yield getContainerForSelector("#test-div", inspector);
-  let attr = editor.attrs["id"].querySelector(".editable");
+  let attr = editor.attrElements.get("id").querySelector(".editable");
   let mutated = inspector.once("markupmutation");
   setEditableFieldValue(attr,
     attr.textContent + ' class="newclass" style="color:green"', inspector);
   yield mutated;
 
   info("Verify attributes, should have ID, class and style");
   yield assertAttributes("#test-div", {
     id: "test-div",
--- a/browser/devtools/markupview/test/browser_markupview_tag_edit_07.js
+++ b/browser/devtools/markupview/test/browser_markupview_tag_edit_07.js
@@ -42,39 +42,39 @@ let TEST_DATA = [{
 }, {
   desc: "Try to add long data URL to make sure it is collapsed in attribute editor.",
   text: "style='"+DATA_URL_INLINE_STYLE+"'",
   expectedAttributes: {
     'style': DATA_URL_INLINE_STYLE
   },
   validate: (element, container, inspector) => {
     let editor = container.editor;
-    let visibleAttrText = editor.attrs["style"].querySelector(".attr-value").textContent;
+    let visibleAttrText = editor.attrElements.get("style").querySelector(".attr-value").textContent;
     is (visibleAttrText, DATA_URL_INLINE_STYLE_COLLAPSED);
   }
 }, {
   desc: "Try to add long attribute to make sure it is collapsed in attribute editor.",
   text: 'data-long="'+LONG_ATTRIBUTE+'"',
   expectedAttributes: {
     'data-long':LONG_ATTRIBUTE
   },
   validate: (element, container, inspector) => {
     let editor = container.editor;
-    let visibleAttrText = editor.attrs["data-long"].querySelector(".attr-value").textContent;
+    let visibleAttrText = editor.attrElements.get("data-long").querySelector(".attr-value").textContent;
     is (visibleAttrText, LONG_ATTRIBUTE_COLLAPSED)
   }
 }, {
   desc: "Try to add long data URL to make sure it is collapsed in attribute editor.",
   text: 'src="'+DATA_URL_ATTRIBUTE+'"',
   expectedAttributes: {
     "src": DATA_URL_ATTRIBUTE
   },
   validate: (element, container, inspector) => {
     let editor = container.editor;
-    let visibleAttrText = editor.attrs["src"].querySelector(".attr-value").textContent;
+    let visibleAttrText = editor.attrElements.get("src").querySelector(".attr-value").textContent;
     is (visibleAttrText, DATA_URL_ATTRIBUTE_COLLAPSED);
   }
 }];
 
 add_task(function*() {
   let {inspector} = yield addTab(TEST_URL).then(openInspector);
   yield runAddAttributesTests(TEST_DATA, "div", inspector)
 });
--- a/browser/devtools/markupview/test/browser_markupview_tag_edit_08.js
+++ b/browser/devtools/markupview/test/browser_markupview_tag_edit_08.js
@@ -31,29 +31,29 @@ function* testCollapsedLongAttribute(ins
 
   yield assertAttributes("#node24", {
     id: "node24",
     "class": "",
     "data-long": LONG_ATTRIBUTE
   });
 
   let {editor} = yield getContainerForSelector("#node24", inspector);
-  let attr = editor.attrs["data-long"].querySelector(".editable");
+  let attr = editor.attrElements.get("data-long").querySelector(".editable");
 
   // Check to make sure it has expanded after focus
   attr.focus();
   EventUtils.sendKey("return", inspector.panelWin);
   let input = inplaceEditor(attr).input;
   is (input.value, 'data-long="' + LONG_ATTRIBUTE + '"');
   EventUtils.sendKey("escape", inspector.panelWin);
 
   setEditableFieldValue(attr, input.value + ' data-short="ABC"', inspector);
   yield inspector.once("markupmutation");
 
-  let visibleAttrText = editor.attrs["data-long"].querySelector(".attr-value").textContent;
+  let visibleAttrText = editor.attrElements.get("data-long").querySelector(".attr-value").textContent;
   is (visibleAttrText, LONG_ATTRIBUTE_COLLAPSED)
 
   yield assertAttributes("#node24", {
     id: "node24",
     class: "",
     'data-long': LONG_ATTRIBUTE,
     "data-short": "ABC"
   });
@@ -64,17 +64,17 @@ function* testModifyInlineStyleWithQuote
 
   yield assertAttributes("#node26", {
     id: "node26",
     style: 'background-image: url("moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.mozilla.org%2F");'
   });
 
   let onMutated = inspector.once("markupmutation");
   let {editor} = yield getContainerForSelector("#node26", inspector);
-  let attr = editor.attrs["style"].querySelector(".editable");
+  let attr = editor.attrElements.get("style").querySelector(".editable");
 
   attr.focus();
   EventUtils.sendKey("return", inspector.panelWin);
 
   let input = inplaceEditor(attr).input;
   let value = input.value;
 
   is (value,
@@ -100,17 +100,17 @@ function* testEditingAttributeWithMixedQ
 
   yield assertAttributes("#node27", {
     "id": "node27",
     "class": 'Double " and single \''
   });
 
   let onMutated = inspector.once("markupmutation");
   let {editor} = yield getContainerForSelector("#node27", inspector);
-  let attr = editor.attrs["class"].querySelector(".editable");
+  let attr = editor.attrElements.get("class").querySelector(".editable");
 
   attr.focus();
   EventUtils.sendKey("return", inspector.panelWin);
 
   let input = inplaceEditor(attr).input;
   let value = input.value;
 
   is (value, "class=\"Double &quot; and single '\"", "Value contains &quot;");
--- a/browser/devtools/markupview/test/browser_markupview_tag_edit_09.js
+++ b/browser/devtools/markupview/test/browser_markupview_tag_edit_09.js
@@ -22,17 +22,17 @@ function* testWellformedMixedCase(inspec
   info("Modifying a mixed-case attribute, " +
     "expecting the attribute's case to be preserved");
 
   info("Listening to markup mutations");
   let onMutated = inspector.once("markupmutation");
 
   info("Focusing the viewBox attribute editor");
   let {editor} = yield getContainerForSelector("svg", inspector);
-  let attr = editor.attrs["viewBox"].querySelector(".editable");
+  let attr = editor.attrElements.get("viewBox").querySelector(".editable");
   attr.focus();
   EventUtils.sendKey("return", inspector.panelWin);
 
   info("Editing the attribute value and waiting for the mutation event");
   let input = inplaceEditor(attr).input;
   input.value = "viewBox=\"0 0 1 1\"";
   EventUtils.sendKey("return", inspector.panelWin);
   yield onMutated;
@@ -48,17 +48,17 @@ function* testMalformedMixedCase(inspect
   info("Modifying a malformed, mixed-case attribute, " +
     "expecting the attribute's case to be preserved");
 
   info("Listening to markup mutations");
   let onMutated = inspector.once("markupmutation");
 
   info("Focusing the viewBox attribute editor");
   let {editor} = yield getContainerForSelector("svg", inspector);
-  let attr = editor.attrs["viewBox"].querySelector(".editable");
+  let attr = editor.attrElements.get("viewBox").querySelector(".editable");
   attr.focus();
   EventUtils.sendKey("return", inspector.panelWin);
 
   info("Editing the attribute value and waiting for the mutation event");
   let input = inplaceEditor(attr).input;
   input.value = "viewBox=\"<>\"";
   EventUtils.sendKey("return", inspector.panelWin);
   yield onMutated;
--- a/browser/devtools/markupview/test/helper_attributes_test_runner.js
+++ b/browser/devtools/markupview/test/helper_attributes_test_runner.js
@@ -128,17 +128,17 @@ function* runEditAttributesTest(test, in
   info("Editing attribute " + test.name + " with value " + test.value);
 
   let container = yield getContainerForSelector(test.node, inspector);
   ok(container && container.editor, "The markup-container for " + test.node +
     " was found");
 
   info("Listening for the markupmutation event");
   let nodeMutated = inspector.once("markupmutation");
-  let attr = container.editor.attrs[test.name].querySelector(".editable");
+  let attr = container.editor.attrElements.get(test.name).querySelector(".editable");
   setEditableFieldValue(attr, test.value, inspector);
   yield nodeMutated;
 
   info("Asserting the new attributes after edition");
   yield assertAttributes(test.node, test.expectedAttributes);
 
   info("Undo the change and assert that the attributes have been changed back");
   yield undoChange(inspector);