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 266456 9630284e15e6ff436a1ec2b0a9c2e884eac69d04
parent 266455 697c11bd51f6ad213e375efa2e1fd2c253f98544
child 266457 fde5601c5dd4bf12138d70bb24644c90a49888f4
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmratcliffe
bugs1147128
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 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);