341256 - DOM Inspector allows for editing, copying, pasting, and cutting of multiple attributes of a node, but it only does the last one selected

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Shawn Wilsher :sdwilsh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 LNSE/1.0.0 RTSE/1.1.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060610 Minefield/3.0a1

From PM from np on MozillaZine:
The DOM Node view on the right allows you to select multiple attributes, but Cut, Copy, and Delete will only work on the first one selected. Also, Edit is enabled when there's a multiple selection, but again, you're only editing the first selected. Change the code so that Cut, Copy, and Delete work on all selected items and that Edit is disabled when more than one is selected. Cut and Copy should put a string representation of all attributes selected onto the clipboard, separated by spaces.

Reproducible: Always

Steps to Reproduce:
1. Select multiple attributes of a DOM node in the DOM Inspector
2. Try to copy, cut, or edit.

Actual Results:  
Only the last node selected is copied, cut, or edited.

Expected Results:  
All nodes should be copied, cut, and edit should be disabled.

Comment 1

[timeless]

personally I'd rather newlines over spaces.

unless you're using quote delims for values ....

Comment 2

[Jason Barnabe (np)]

I had imagined the string representation would be in the form

attr1="value" attr2="value"

So that you could just plop that into a HTML/XML document, just like right now copying CSS puts it in a format that you can put into a CSS document.

Comment 3

[Alex Vincent [:WeirdAl]]

Alternatively, we could just specify seltype="single" on the tree.

Comment 4

[Jason Barnabe (np)]

I think cutting, copying, or deleting multiple attributes at once is useful and expected.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: Possible Patch

Alright, this attachment fixed all of this at least from my testing.  One thing np had mentioned on MozillaZine is that I should use cmdEditCopy from util.js (http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/utils.js#215) in place of the current function (http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/viewers/domNode/domNode.js#264).  I would do that, but I'm not sure exactly how to go about that, but if someone is willing to give me a pointer to documentation or a hint, I'll write the code.

Comment 6

[Alex Vincent [:WeirdAl]]

(In reply to comment #5)
> Created an attachment (id=225309) [edit]
> Possible Patch

Patch looks largely okay to me as-is to me.  Please fix whitespace issues (no tab characters!) and line lengths (more than 80 chars for new lines is not nice).

Comment 7

[Jason Barnabe (np)]

You need to request reviews on your patch if you think you're done. I suggest r?[e-mail] sr?[e-mail]

(In reply to comment #5)
>  I would do that, but I'm not sure exactly how to go about that, but if someone
> is willing to give me a pointer to documentation or a hint, I'll write the
> code.

Write a DOMAttribute function similar to the CSSDeclaration object function here
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/utils.js#245

Modify the instantiation here 
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/viewers/domNode/domNode.js#205
to include whatever parameter(s) you defined in the constructor of DOMAttribute.

Remove the cmdEditCopy function out of domNode.js.

Make sure this doesn't break copy/paste of DOM Elements.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Write a DOMAttribute function similar to the CSSDeclaration object function
> here
> http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/utils.js#245
> 
> Modify the instantiation here 
> http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/viewers/domNode/domNode.js#205
> to include whatever parameter(s) you defined in the constructor of
> DOMAttribute.
> 
> Remove the cmdEditCopy function out of domNode.js.
> 
> Make sure this doesn't break copy/paste of DOM Elements.
> 

This new function should probably be in /inspector/resources/content/utils.js, right?

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: Possible Patch

This is basically the previous patch with the tabs taken out (pesky things snuck in there), line wrapping at 80 chars for everything I changed, cmdEditCopy removed, and the DOMAttribute object function as requested in Comment 7 which is placed in /inspector/resources/content/utils.js.  It seemed like the right place to put it since CSSDeclaration is there as well.

This does not break copy/paste of DOM Elements as brought up as a possible concern in Comment 7.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 225360 [details] [diff] [review]
Possible Patch

? .utils.js.swp
? 341256.patch
? viewers/domNode/.domNode.js.swp
? viewers/domNode/341256.patch
? viewers/domNode/domNode(works).js
Index: utils.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/utils.js,v
retrieving revision 1.14
diff -u -8 -p -r1.14 utils.js
--- utils.js	28 May 2006 19:01:01 -0000	1.14
+++ utils.js	13 Jun 2006 01:52:42 -0000
@@ -16,16 +16,17 @@
  * The Initial Developer of the Original Code is
  * Netscape Communications Corporation.
  * Portions created by the Initial Developer are Copyright (C) 2001
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Joe Hewitt <[e-mail]> (original author)
  *   Jason Barnabe <[e-mail]>
+ *   Shawn Wilsher <[e-mail]>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
  * in which case the provisions of the GPL or the LGPL are applicable instead
  * of those above. If you wish to allow use of your version of this file only
  * under the terms of either the GPL or the LGPL, and not to allow others to
  * use your version of this file under the terms of the MPL, indicate your
@@ -256,8 +257,29 @@ function CSSDeclaration(aProperty, aValu
 }
 /**
  * Returns a usable CSS string for the CSSDeclaration.
  * @return a string in the form "property: value;"
  */
 CSSDeclaration.prototype.toString = function toString() {
   return this.property + ": " + this.value + (this.important ? " !important" : "") + ";";
 }
+
+/**
+ * Represents a DOM attribute.
+ * @param aName the name of the attribute
+ * @param aValue the value of the attribute
+ */
+function DOMAttribute(aName, aValue)
+{
+  this.flavor = "inspector/dom-attribute";
+  this.name = aName;
+  this.value = aValue;
+  this.delimiter = " ";
+}
+/**
+ * Returns a string representing an attribute name/value pair
+ * @return a string in the form of 'name="value"'
+ */
+DOMAttribute.prototype.toString = function toString()
+{
+  return this.name + "=\"" + this.value + "\"";
+};
Index: viewers/domNode/domNode.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/viewers/domNode/domNode.js,v
retrieving revision 1.19
diff -u -8 -p -r1.19 domNode.js
--- viewers/domNode/domNode.js	4 Jun 2006 05:45:33 -0000	1.19
+++ viewers/domNode/domNode.js	13 Jun 2006 01:52:42 -0000
@@ -16,16 +16,17 @@
  * The Initial Developer of the Original Code is
  * Netscape Communications Corporation.
  * Portions created by the Initial Developer are Copyright (C) 2001
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Joe Hewitt <[e-mail]> (original author)
  *   Jason Barnabe <[e-mail]>
+ *   Shawn Wilsher <[e-mail]>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
  * in which case the provisions of the GPL or the LGPL are applicable instead
  * of those above. If you wish to allow use of your version of this file only
  * under the terms of either the GPL or the LGPL, and not to allow others to
  * use your version of this file under the terms of the MPL, indicate your
@@ -87,20 +88,60 @@ DOMNodeViewer.prototype = 
   mSubject: null,
   mPanel: null,
 
   get selectedIndex()
   {
     return this.mAttrTree.currentIndex;
   },
 
+ /**
+  * Returns an array of the selected indices
+  * @return an array of indices
+  */
+  get selectedIndices()
+  {
+    var indices = [];
+    var rangeCount = this.mAttrTree.view.selection.getRangeCount();
+    for(var i = 0; i < rangeCount; ++i) {
+      var start = {};
+      var end = {};
+      this.mAttrTree.view.selection.getRangeAt(i,start,end);
+      for(var c = start.value; c <= end.value; ++c) {
+        indices.push(c);
+      }
+    }
+    return (indices.length) ? indices : null;
+  },
+
+ /**
+  * Returns a DOMAttribute from the selected index
+  * @return a DomAttribute
+  */
   get selectedAttribute()
   {
     var index = this.selectedIndex;
-    return index >= 0 ? this.mDOMView.getNodeFromRowIndex(index) : null;
+    var node = (index >= 0) ? this.mDOMView.getNodeFromRowIndex(index) : null
+    return (node) ? new DOMAttribute(node.nodeName, node.nodeValue) : null;
+  },
+
+ /**
+  * Returns an array of DOMAttributes from the selected indices
+  * @return an array of DOMAttributes
+  */
+  get selectedAttributes()
+  {
+    var indicies = this.selectedIndices;
+    var attrs = [];
+    var node;
+    for (var i = (indicies.length-1); i >= 0; --i) {
+      node = this.mDOMView.getNodeFromRowIndex(indicies[i]);
+      attrs.push(new DOMAttribute(node.nodeName, node.nodeValue));
+    }
+    return (attrs.length) ? attrs : null;
   },
 
   ////////////////////////////////////////////////////////////////////////////
   //// interface inIViewer
 
   //// attributes 
 
   get uid() { return "domNode" },
@@ -160,29 +201,38 @@ DOMNodeViewer.prototype = 
     // let's fire it. this won't do anything if it wasn't actually changed
     viewer.pane.panelset.execCommand('cmdEditNodeValue');
   },
 
   isCommandEnabled: function(aCommand)
   {
     switch (aCommand) {
       case "cmdEditPaste":
-        var canPaste = this.mPanel.panelset.clipboardFlavor == "inspector/dom-node";
+        var flavor = this.mPanel.panelset.clipboardFlavor;
+        var canPaste = (flavor == "inspector/dom-attribute" ||
+                        flavor == "inspector/dom-attributes");
         if (canPaste) {
-          var node = this.mPanel.panelset.getClipboardData();
-          canPaste = node ? node.nodeType == Node.ATTRIBUTE_NODE : false;
+          var attr = this.mPanel.panelset.getClipboardData();
+          if (flavor == "inspector/dom-attributes") {
+            for (var i = (attr.length-1); i >= 0; --i) {
+              canPaste = (attr[i] instanceof DOMAttribute) ? canPaste : false;
+            }
+          } else {
+            canPaste = attr ? attr instanceof DOMAttribute : false;
+          }
         }
         return canPaste;
       case "cmdEditInsert":
         return true;
       case "cmdEditCut":
       case "cmdEditCopy":
-      case "cmdEditEdit":
       case "cmdEditDelete":
         return this.selectedAttribute != null;
+      case "cmdEditEdit":
+        return this.mAttrTree.view.selection.count == 1;
       case "cmdEditNodeValue":
         // this function can be fired before the subject is set
         if (this.subject) {
           // something with a useful nodeValue
           if (this.subject.nodeType == Node.TEXT_NODE ||
               this.subject.nodeType == Node.CDATA_SECTION_NODE ||
               this.subject.nodeType == Node.COMMENT_NODE ||
               this.subject.nodeType == Node.PROCESSING_INSTRUCTION_NODE) {
@@ -197,17 +247,17 @@ DOMNodeViewer.prototype = 
   },
   
   getCommand: function(aCommand)
   {
     switch (aCommand) {
       case "cmdEditCut":
         return new cmdEditCut();
       case "cmdEditCopy":
-        return new cmdEditCopy();
+        return new cmdEditCopy(this.selectedAttributes);
       case "cmdEditPaste":
         return new cmdEditPaste();
       case "cmdEditInsert":
         return new cmdEditInsert();
       case "cmdEditEdit":
         return new cmdEditEdit();
       case "cmdEditDelete":
         return new cmdEditDelete();
@@ -244,82 +294,91 @@ function cmdEditCut() {}
 cmdEditCut.prototype =
 {
   cmdCopy: null,
   cmdDelete: null,
   doCommand: function()
   {
     if (!this.cmdCopy) {
       this.cmdDelete = new cmdEditDelete();
-      this.cmdCopy = new cmdEditCopy();
+      this.cmdCopy = new cmdEditCopy(viewer.selectedAttributes);
     }
-    this.cmdCopy.doCommand();
+    this.cmdCopy.doTransaction();
     this.cmdDelete.doCommand();    
   },
 
   undoCommand: function()
   {
     this.cmdDelete.undoCommand();    
   }
 };
 
-function cmdEditCopy() {}
-cmdEditCopy.prototype =
-{
-  copiedAttr: null,
-    
-  doCommand: function()
-  {
-    var copiedAttr = null;
-    if (!this.copiedAttr) {
-      copiedAttr = viewer.selectedAttribute;
-      if (!copiedAttr)
-        return true;
-      this.copiedAttr = copiedAttr;
-    } else
-      copiedAttr = this.copiedAttr;
-      
-    var text = copiedAttr.nodeName + "=\"" + InsUtil.unicodeToEntity(copiedAttr.nodeValue) + "\"";
-    viewer.pane.panelset.setClipboardData(copiedAttr, "inspector/dom-node", text);
-    return true;
-  }
-};
-
 function cmdEditPaste() {}
 cmdEditPaste.prototype =
 {
   pastedAttr: null,
   previousAttrValue: null,
   subject: null,
+  flavor: null,
   
   doCommand: function()
   {
-    var subject, pastedAttr;
+    var subject, pastedAttr, flavor;
     if (this.subject) {
       subject = this.subject;
       pastedAttr = this.pastedAttr;
+      flavor = this.flavor;
     } else {
       subject = viewer.subject;
       pastedAttr = viewer.pane.panelset.getClipboardData();
+      flavor = viewer.pane.panelset.clipboardFlavor;
       this.pastedAttr = pastedAttr;
       this.subject = subject;
-      this.previousAttrValue = viewer.subject.getAttribute(pastedAttr.nodeName);
+      this.flavor = flavor;
+      if (flavor == "inspector/dom-attributes") {
+        this.previousAttrValue = [];
+        for (var i = (pastedAttr.length-1); i >= 0; --i) {
+          this.previousAttrValue[pastedAttr[i].name] =
+            viewer.subject.getAttribute(pastedAttr[i].name);
+        }
+      } else if (flavor == "inspector/dom-attribute") {
+        this.previousAttrValue =
+          viewer.subject.getAttribute(pastedAttr.name);
+      }
     }
     
-    if (subject && pastedAttr)
-      subject.setAttribute(pastedAttr.nodeName, pastedAttr.nodeValue);      
+    if (subject && pastedAttr && flavor) {
+      if (flavor == "inspector/dom-attributes") {
+        for (var i = (pastedAttr.length-1); i >= 0; --i) {
+          subject.setAttribute(pastedAttr[i].name, pastedAttr[i].value);
+        }
+      } else if (flavor == "inspector/dom-attribute") {
+        subject.setAttribute(pastedAttr.name, pastedAttr.value);
+      }
+    }
   },
   
   undoCommand: function()
   {
     if (this.pastedAttr) {
-      if (this.previousAttrValue)
-        this.subject.setAttribute(this.pastedAttr.nodeName, this.previousAttrValue);
-      else
-        this.subject.removeAttribute(this.pastedAttr.nodeName);
+      if (this.flavor == "inspector/dom-attributes") {
+        for (var i = (this.pastedAttr.length-1); i >= 0; --i) {
+          if (this.previousAttrValue[this.pastedAttr[i].name])
+            this.subject.setAttribute(this.pastedAttr[i].name,
+                              this.previousAttrValue[this.pastedAttr[i].name]);
+          else
+            this.subject.removeAttribute(this.pastedAttr[i].nodeName);
+        }
+      } else if (this.flavor == "inspector/dom-attribute") {
+        if (this.previousAttrValue)
+          this.subject.setAttribute(this.pastedAttr.name,
+                                    this.previousAttrValue);
+        else
+          this.subject.removeAttribute(this.pastedAttr.nodeName);
+      }
     }
   }
 };
 
 function cmdEditInsert() {}
 cmdEditInsert.prototype =
 {
   attr: null,
@@ -370,28 +429,33 @@ cmdEditInsert.prototype =
 function cmdEditDelete() {}
 cmdEditDelete.prototype =
 {
   attr: null,
   subject: null,
   
   doCommand: function()
   {
-    var attr = this.attr ? this.attr : viewer.selectedAttribute;
+    var attr = this.attr ? this.attr : viewer.selectedAttributes;
     if (attr) {
       this.attr = attr;
       this.subject = viewer.subject;
-      this.subject.removeAttribute(attr.nodeName);
+      for (var i = (this.attr.length-1); i >= 0; i--) {
+        this.subject.removeAttribute(this.attr[i].name);
+      }
     }
   },
   
   undoCommand: function()
   {
-    if (this.attr)
-      this.subject.setAttribute(this.attr.nodeName, this.attr.nodeValue);
+    if (this.attr) {
+      for (var i = (this.attr.length-1); i >= 0; i--) {
+        this.subject.setAttribute(this.attr[i].name, this.attr[i].value);
+      }
+    }
   }
 };
 
 function cmdEditEdit() {}
 cmdEditEdit.prototype =
 {
   attr: null,
   previousValue: null,
@@ -401,48 +465,48 @@ cmdEditEdit.prototype =
   promptFor: function()
   {
     var attr = viewer.selectedAttribute;
     if (attr) {
       if (!gPromptService) {
         gPromptService = XPCU.getService(kPromptServiceCID, "nsIPromptService");
       }
 
-      var attrValue = { value: attr.nodeValue };
+      var attrValue = { value: attr.value };
       var dummy = { value: false };
 
       var bundle = viewer.pane.panelset.stringBundle;
       var msg = bundle.getString("enterAttrValue.message");
       var title = bundle.getString("editAttribute.title");
 
       if (gPromptService.prompt(window, title, msg, attrValue, null, dummy)) {
         this.subject = viewer.subject;
         this.newValue = attrValue.value;
-        this.previousValue = attr.nodeValue;
-        this.subject.setAttribute(attr.nodeName, attrValue.value);
-        this.attr = this.subject.getAttributeNode(attr.nodeName);
+        this.previousValue = attr.value;
+        this.subject.setAttribute(attr.name, attrValue.value);
+        this.attr = new DOMAttribute(attr.name, attrValue.value);
         return false;
       }
     }
     return true;
   },
   
   doCommand: function()
   {
     if (this.attr)
-      this.subject.setAttribute(this.attr.nodeName, this.newValue);
+      this.subject.setAttribute(this.attr.name, this.newValue);
     else
       return this.promptFor();
     return false;
   },
   
   undoCommand: function()
   {
     if (this.attr)
-      this.subject.setAttribute(this.attr.nodeName, this.previousValue);
+      this.subject.setAttribute(this.attr.name, this.previousValue);
   }
 };
 
 /**
  * Handles editing of node values.
  */
 function cmdEditNodeValue() {
   this.newValue = document.getElementById("txbTextNodeValue").value;

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: Corrected Patch File

Terribly sorry about the bug spam.  Seems I forgot to save the one file before creating the first patch, and the editing the attachment didn't do what I expected.

Comment 12

[Jason Barnabe (np)]

Comment on attachment 225365 [details] [diff] [review]
Corrected Patch File

>+function DOMAttribute(aName, aValue)
Might be better to just pass in the attribute itself and only store that. If you do so, make a copy of it so that changes after the copy won't affect your copied version.

>+DOMAttribute.prototype.toString = function toString()
>+{
>+  return this.name + "=\"" + this.value + "\"";
>+};
You forgot about the escaping the previous code did.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> (From update of attachment 225365 [details] [diff] [review] [edit])
> >+function DOMAttribute(aName, aValue)
> Might be better to just pass in the attribute itself and only store that. If
> you do so, make a copy of it so that changes after the copy won't affect your
> copied version.

So something like this?

function DOMAttribute(aNode)
{
  this.mNode = aNode.cloneNode();
  this.name = mNode.nodeName;
  this.value = mNode.nodeValue;
...
}

Comment 14

[Jason Barnabe (np)]

(In reply to comment #13)
> So something like this?
> 
> function DOMAttribute(aNode)
> {
>   this.mNode = aNode.cloneNode();
>   this.name = mNode.nodeName;
>   this.value = mNode.nodeValue;
> ...
> }

I don't see any reason to store both the node and the name/value pair. Just store the node and use nodeName and nodeValue in toString.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: Revised Patch

Fixes as per Comment 12.

Comment 16

[[e-mail]]

Comment on attachment 225435 [details] [diff] [review]
Revised Patch

>+  this.node = aNode.cloneNode(false);
I'm not sure it's worth your while copying the node; all you use it for is its name and value, it's not as if you need the node again later.

>-    return index >= 0 ? this.mDOMView.getNodeFromRowIndex(index) : null;
>+    return (index >= 0) ?
>+      new DOMAttribute(this.mDOMView.getNodeFromRowIndex(index)) : null;
Bah, I was going to comment on your previous code, but you changed it ;-)
No need for the ()s around index >= 0 though.

>-        var canPaste = this.mPanel.panelset.clipboardFlavor == "inspector/dom-node";
>+        var flavor = this.mPanel.panelset.clipboardFlavor;
>+        var canPaste = (flavor == "inspector/dom-attribute" ||
>+                        flavor == "inspector/dom-attributes");
If you're going to change the supported flavour(s) then you don't need to check the data any more.

>-    this.cmdCopy.doCommand();
>+    this.cmdCopy.doTransaction();
>     this.cmdDelete.doCommand();    
Why does copy change but not delete? I'll continue review after you answer.

>+        this.previousAttrValue =
>+          viewer.subject.getAttribute(pastedAttr.node.nodeName);
>+        if (this.previousAttrValue)
Hmm... I guess inspector already isn't a fan of empty attributes.

Comment 17

[Jason Barnabe (np)]

(In reply to comment #16)
> (From update of attachment 225435 [details] [diff] [review] [edit])
> >+  this.node = aNode.cloneNode(false);
> I'm not sure it's worth your while copying the node; all you use it for is its
> name and value, it's not as if you need the node again later.

It could also have a namespace and prefix, could it not?

> >-    this.cmdCopy.doCommand();
> >+    this.cmdCopy.doTransaction();
> >     this.cmdDelete.doCommand();    
> Why does copy change but not delete? I'll continue review after you answer.

Copy has been changed to use the common Copy in utils.js which uses doTransaction. Delete wasn't changed so it's doing whatever it was before.

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> It could also have a namespace and prefix, could it not?

I was wondering if we should include those in the textual representation as well, or perhaps change paste to just take the clonedNode from DOMAttribute and append it to the element (I'm not sure if appendChild would be the right function to use there though).  As it stands now, it just sets an attribute with the name of the node and the node value, so I think all namespacing and prefixing would be lost.  That could be a different bug though too, so I'll wait for feedback.

Comment 19

[Jason Barnabe (np)]

Actually, I think nodeName will include the prefix. Try it out on this document
http://croczilla.com/svg/samples/arcs1/arcs1.svg

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Actually, I think nodeName will include the prefix. Try it out on this document
> http://croczilla.com/svg/samples/arcs1/arcs1.svg
> 

It seemed to work, so I guess we are all set.  I have already revised the files as per Comment 16, but I was going to wait until he finished his review to submit it (if I should just upload it, let me know).

Comment 21

[[e-mail]]

(In reply to comment #18)
>(In reply to comment #17)
>>It could also have a namespace and prefix, could it not?
>(I'm not sure if appendChild would be the right function to use there though).
setAttribute looks for an existing attribute with the given nodeName, and if so, changes its value. Otherwise, it creates an attribute with the given nodeName in no namespace. setAttributeNS creates an attribute with the given nodeName and namespace, deleting any existing attribute with the same localName and namespace, even if its prefix (and therefore nodeName) was different.

Comment 22

[[e-mail]]

Comment on attachment 225435 [details] [diff] [review]
Revised Patch

>+    for(var i = 0; i < rangeCount; ++i) {
Nit: space before (

>+      this.mAttrTree.view.selection.getRangeAt(i,start,end);
Nit: space after , [I missed this reviewing inBaseTreeView.js ...]

>+    return (indices.length) ? indices : null;
Just return indices here. [getSelectedAttributes doesn't null-check!]

>+    for (var i = (indices.length-1); i >= 0; --i) {
Why are all your loops in descending order?

>+    return (attrs.length) ? attrs : null;
Just return attrs here. [Compare inBaseTreeView.js again ...]

>       case "cmdEditPaste":
>-        var canPaste = this.mPanel.panelset.clipboardFlavor == "inspector/dom-node";
>+        var flavor = this.mPanel.panelset.clipboardFlavor;
>+        var canPaste = (flavor == "inspector/dom-attribute" ||
>+                        flavor == "inspector/dom-attributes");
>         if (canPaste) {
>-          var node = this.mPanel.panelset.getClipboardData();
>-          canPaste = node ? node.nodeType == Node.ATTRIBUTE_NODE : false;
>+          var attr = this.mPanel.panelset.getClipboardData();
>+          if (flavor == "inspector/dom-attributes") {
>+            for (var i = (attr.length-1); i >= 0; --i) {
>+              canPaste = (attr[i] instanceof DOMAttribute) ? canPaste : false;
>+            }
>+          } else {
>+            canPaste = attr ? attr instanceof DOMAttribute : false;
>+          }
>         }
>         return canPaste;
You don't need extended checking here, since the flavours are unique.

>+    if (subject && pastedAttr && flavor) {
You don't need to check flavor here as you're checking it again immediately.

>-        this.attr = this.subject.getAttributeNode(attr.nodeName);
>+        this.attr = new DOMAttribute(this.subject.getAttributeNode(attr.nodeName));
You don't need this, this attr is an attribute node, not a DOMAttribute.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: Revised Patch

Revised as per comments in Comment 22.

(In reply to comment #22)
> Why are all your loops in descending order?
Namely because it is my understanding that it's more efficient to only check the length of the array once, in the beginning of the loop, and then count down.  Yeah, it probably doesn't matter for this case, but it's just habit.  I can change it if you want me to.

(In reply to comment #21)
Did you want this to use setAttributeNS?  I don't think it would be terribly difficult to do if that's what you are looking for.

Comment 24

[[e-mail]]

Comment on attachment 225823 [details] [diff] [review]
Revised Patch

(In reply to comment #23)
>(In reply to comment #22)
>>Why are all your loops in descending order?
>I can change it if you want me to.
Yes please. It looks as if your code appears to copy the attributes in reverse order, which would look odd. sr=me with this fixed.

>(In reply to comment #21)
>Did you want this to use setAttributeNS?  I don't think it would be terribly
>difficult to do if that's what you are looking for.
I think that's outside the scope of this bug. At the moment, if you delete a namespaced attribute then undo doesn't restore it correctly anyway. And of course there's no way to add a namespaced attribute.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: Superreviewed Patch

sr=neil

(In reply to Comment #24)
>I think that's outside the scope of this bug. At the moment, if you delete a
>namespaced attribute then undo doesn't restore it correctly anyway. And of
>course there's no way to add a namespaced attribute.
I think Bug 205872 is what you are talking about.  If Alex doesn't want it anymore, that can be assigned to me once this is done.  I have a good idea on what to do with it, and I'll post it later today on that bug.

Comment 26

[Shawn Wilsher :sdwilsh]

Attachment: Post-Review Patch

r=timeless (via IRC)
sr=neil (via previous patch)

Just fixes a typo from previous patch, and adds a comment regrading how edit doesn't work well with attributes that have newlines in them.

Timeless had mentioned that it should work by copying something like attr="value" from a text editor and pasting into the inspector, but I can't see being able to do that unless we change a large amount of code.  As a result, I'm not sure if it is worthwhile.

I'm told on IRC that I now need to have someone check this in, so will neil or timeless do that please?

Comment 27

[Jason Barnabe (np)]

(In reply to comment #26)
> Timeless had mentioned that it should work by copying something like
> attr="value" from a text editor and pasting into the inspector, but I can't see
> being able to do that unless we change a large amount of code.  As a result,
> I'm not sure if it is worthwhile.

Inspector doesn't actually copy nodes to the system clipboard - it just stores them internally and puts a textual representation into the system clipboard (bug 328870). The problem with the suggested approach would be that Inspector would have to decide whether to paste its internally stored node or to try to parse the system clipboard. It's doable, but I think it'd be a seperate bug considering this one just talks about the existing functionality, but for multiple selectiosn.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> It's doable, but I think it'd be a seperate bug
Filed Bug 341781, but I think it's a bit over my head for me to take.

Comment 29

[Shawn Wilsher :sdwilsh]

Attachment: Final Patch

r=timeless
sr=neil

As per Bug 310370 Comment 12.

>By the way, I wouldn't bother with your comment about editing multiline attributes, if someone really wants to do that they can set their newlines
preference to allow newlines in textboxes (which is the Linux default).

It was requested by timeless to be put in there, so I left it.

Any chance on this getting committed?  It's holding me up for another patch involving one of these files.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/extensions/inspector/resources/content/utils.js 	1.15
mozilla/extensions/inspector/resources/content/viewers/domNode/domNode.js 1.20