Bug 839640 - Editing an attribute that contains '&' fails; r=jwalker

browser/devtools/markupview/MarkupView.jsm

@@ -1178,28 +1178,21 @@ ElementEditor.prototype = {
    * @param Element aAttrNode the attribute editor that created this
    *        set of attributes, used to place new attributes where the
    *        user put them.
-   * @throws SYNTAX_ERR if aValue is not well-formed.
    */
   _applyAttributes: function EE__applyAttributes(aValue, aAttrNode)
   {
-    // Create a dummy node for parsing the attribute list.
-    let dummyNode = this.doc.createElement("div");
-
-    let parseTag = (this.node.namespaceURI.match(/svg/i) ? "svg" :
-                   (this.node.namespaceURI.match(/mathml/i) ? "math" : "div"));
-    let parseText = "<" + parseTag + " " + aValue + "/>";
-    // Throws exception if parseText is not well-formed.
-    dummyNode.innerHTML = parseText;
-    let parsedNode = dummyNode.firstChild;
-
-    let attrs = parsedNode.attributes;
+    let attrs = escapeAttributeValues(aValue);
 
     this.undo.startBatch();
 
-    for (let i = 0; i < attrs.length; i++) {
+    for (let attr of attrs) {
+      let attribute = {
+        name: attr.name,
+        value: attr.value
+      };
       // Create an attribute editor next to the current attribute if needed.
-      this._createAttribute(attrs[i], aAttrNode ? aAttrNode.nextSibling : null);
-      this._setAttribute(this.node, attrs[i].name, attrs[i].value);
+      this._createAttribute(attribute, aAttrNode ? aAttrNode.nextSibling : null);
+      this._setAttribute(this.node, attr.name, attr.value);
     }
 
     this.undo.endBatch();
@@ -1424,6 +1417,100 @@ DocumentWalker.prototype = {
   nextSibling: function DW_nextSibling() this.walker.nextSibling(),
 
   // XXX bug 785143: not doing previousNode or nextNode, which would sure be useful.
+};
+
+/**
+ * Properly escape attribute values.
+ *
+ * @param  {String} attr
+ *         The attributes for which the values are to be escaped.
+ * @return {Array}
+ *         An array of attribute names and their escaped values.
+ */
+function escapeAttributeValues(attr) {
+  let name = null;
+  let value = null;
+  let result = "";
+  let attributes = [];
+
+  while(attr.length > 0) {
+    let match;
+    let dirty = false;
+
+    // Trim quotes and spaces from attr start
+    match = attr.match(/^["\s]+/);
+    if (match && match.length == 1) {
+      attr = attr.substr(match[0].length);
+    }
+
+    // Name
+    if (!dirty) {
+      match = attr.match(/^([\w-]+)="/);
+      if (match && match.length == 2) {
+        if (name) {
+          // We had a name without a value e.g. disabled. Let's set the value to "";
+          value = "";
+        } else {
+          name = match[1];
+          attr = attr.substr(match[0].length);
+        }
+        dirty = true;
+      }
+    }
+
+    // Value (in the case of multiple attributes)
+    if (!dirty) {
+      match = attr.match(/^(.+?)"\s+[\w-]+="/);
+      if (match && match.length > 1) {
+        value = typeof match[1] == "undefined" ? match[2] : match[1];
+        attr = attr.substr(value.length);
+        value = simpleEscape(value);
+        dirty = true;
+      }
+    }
+
+    // Final value
+    if (!dirty && attr.indexOf("=\"") == -1) {
+      // No more attributes, get the remaining value minus it's ending quote.
+      if (attr.charAt(attr.length - 1) == '"') {
+        attr = attr.substr(0, attr.length - 1);
+      }
+
+      if (!name) {
+        name = attr;
+        value = "";
+      } else {
+        value = simpleEscape(attr);
+      }
+      attr = "";
+      dirty = true;
+    }
+
+    if (name !== null && value !== null) {
+      attributes.push({name: name, value: value});
+      name = value = null;
+    }
+
+    if (!dirty) {
+      // This should never happen but we exit here if it does.
+      return attributes;
+    }
+  }
+  return attributes;
+}
+
+/**
+ * Escape basic html entities <, >, " and '.
+ * @param  {String} value
+ *         Value to escape.
+ * @return {String}
+ *         Escaped value.
+ */
+function simpleEscape(value) {
+  return value.replace(/</g, "&lt;")
+              .replace(/>/g, "&gt;")
+              .replace(/"/g, "&quot;")
+              .replace(/'/g, "&apos;");
 }
 
 /**

browser/devtools/markupview/test/browser_inspector_markup_edit.html

@@ -39,5 +39,6 @@
     <div id="retag-me">
       <div id="retag-me-2"></div>
     </div>
+    <div id="node25"></div>
   </body>
 </html>

browser/devtools/markupview/test/browser_inspector_markup_edit.js

@@ -42,12 +42,27 @@ function test() {
     EventUtils.sendKey("return", inspector.panelWin);
   }
 
+  /**
+   * Check that the appropriate attributes are assigned to a node.
+   *
+   * @param  {HTMLNode} aElement
+   *         The node to check.
+   * @param  {Object} aAttributes
+   *         An object containing the arguments to check.
+   *         e.g. {id="id1",class="someclass"}
+   *
+   * NOTE: When checking attribute values bare in mind that node.getAttribute()
+   *       returns attribute values provided by the HTML parser. The parser only
+   *       provides unescaped entities so & will return &.
+   */
   function assertAttributes(aElement, aAttributes)
   {
     let attrs = Object.getOwnPropertyNames(aAttributes);
-    is(aElement.attributes.length, attrs.length, "Node has the correct number of attributes");
+    is(aElement.attributes.length, attrs.length,
+      "Node has the correct number of attributes");
     for (let attr of attrs) {
-      is(aElement.getAttribute(attr), aAttributes[attr], "Node has the correct " + attr + " attribute.");
+      is(aElement.getAttribute(attr), aAttributes[attr],
+        "Node has the correct " + attr + " attribute.");
     }
   }
 
@@ -76,7 +91,8 @@ function test() {
     },
 
     {
-      desc: "Try change an attribute to a badly formed string",
+      desc: 'Try changing an attribute to a quote (") - this should result ' +
+            'in it being set to an empty string',
       before: function() {
         assertAttributes(doc.querySelector("#node22"), {
           id: "node22",
@@ -92,7 +108,7 @@ function test() {
       after: function() {
         assertAttributes(doc.querySelector("#node22"), {
           id: "node22",
-          class: "unchanged"
+          class: ""
         });
       }
     },
@@ -141,7 +157,9 @@ function test() {
     },
 
     {
-      desc: "Try add a badly formed attribute by clicking the empty space after a node",
+      desc: 'Try add an attribute containing a quote (") attribute by ' +
+            'clicking the empty space after a node - this should result ' +
+            'in it being set to an empty string',
       before: function() {
         assertAttributes(doc.querySelector("#node23"), {
           id: "node23",
@@ -156,6 +174,8 @@ function test() {
       after: function() {
         assertAttributes(doc.querySelector("#node23"), {
           id: "node23",
+          class: "newclass",
+          style: ""
         });
       }
     },
@@ -176,6 +196,7 @@ function test() {
       after: function() {
         assertAttributes(doc.querySelector("#node24"), {
           id: "node24",
+          class: ""
         });
       }
     },
@@ -198,6 +219,27 @@ function test() {
         is(node.nodeValue, "New text", "Text should be changed.");
       },
     },
+
+    {
+      desc: "Add an attribute value containing < > &uuml; \" & '",
+      before: function() {
+        assertAttributes(doc.querySelector("#node25"), {
+          id: "node25",
+        });
+      },
+      execute: function(after) {
+        inspector.once("markupmutation", after);
+        let editor = markup.getContainer(doc.querySelector("#node25")).editor;
+        let attr = editor.newAttr;
+        editField(attr, 'src="somefile.html?param1=<a>&param2=&uuml;"bl\'ah"');
+      },
+      after: function() {
+        assertAttributes(doc.querySelector("#node25"), {
+          id: "node25",
+          src: "somefile.html?param1=&lt;a&gt;&param2=&uuml;&quot;bl&apos;ah"
+        });
+      }
+    },
   ];
 
   // Create the helper tab for parsing...