310370 - DOM Inspector - Cannot delete multiple selected nodes simultaneously

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[XFox]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728

In the DOM Inspector, you can select multiple nodes (ctrl+click) in the tree.
However, when you try to delete (del key or right click: delete), only the most
recently selected node is deleted. Afterewards, the others remain highlighted
but are not active (pressing del does nothing for the still remaining
highlighted nodes in the tree).

Reproducible: Always

Steps to Reproduce:
1. Open DOM Inspector and load a page.
2. Expand the tree atleast one level.
3. Hold ctrl and click multiple nodes, each will be highlighted for your selection.
4. Press the del key.

Actual Results:  
Only the most recently selected node in the multiple selection gets deleted.

Expected Results:  
Each selected node should have been deleted in a single delete command.

Comment 1

[zug_treno]

(In reply to comment #0)
> Afterewards, the others remain highlighted
> but are not active (pressing del does nothing for the still remaining
> highlighted nodes in the tree).

Fixed by bug 297660?

Comment 2

[Martijn Wargers (dead)]

Yes, that part was fixed by bug 297660.
Next time, please only use the most recent trunk build (Mozilla1.7 is well over
a year old) to test if the bug still exists.
http://ftp.scarlet.be/pub/mozilla.org/firefox/nightly/latest-trunk/

The multiple selection not deleting the whole multiple selection is a valid bug,
though.

Comment 3

[Shawn Wilsher :sdwilsh]

I can confirm this.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: Patch to fix

Not only can I confirm, but I can submit a patch :)

Someone care to assign this to me please?

Comment 5

[Martijn Wargers (dead)]

Sure, nice job, Shawn!

Comment 6

[Bryan]

are any of these fixes going to make their way to FF2?

~B

Comment 7

[Jason Barnabe (np)]

Comment on attachment 225933 [details] [diff] [review]
Patch to fix

> cmdEditDelete.prototype =
> {
>   node: null,
>   nextSibling: null,
>   parentNode: null,
>+
>   

Fix that.

>+      for (var i = 0; i < this.node.length; ++i) {
>+        if (this.nextSibling[i])
>+          this.parentNode[i].insertBefore(this.node[i], this.nextSibling[i]);
>+        else
>+          this.parentNode[i].appendChild(this.node[i]);
>+      }

insertBefore acts like appendChild if the second argument is null, so do insertBefore unconditionally.

Comment 8

[timeless]

Comment on attachment 225933 [details] [diff] [review]
Patch to fix

seems fine w/ me (address np's comment), thanks for contributing

Comment 9

[[e-mail]]

Comment on attachment 225933 [details] [diff] [review]
Patch to fix

>+
>   
Oops ;-)

>   doTransaction: function()
>   {
>+    var node = this.node ? this.node : viewer.selectedNodes;
>     if (node) {
>       this.node = node;
>+      this.nextSibling = [];
>+      this.parentNode = [];
>+      for (var i = 0; i < node.length; ++i) {
>+        this.nextSibling.push(node[i].nextSibling);
>+        this.parentNode.push(node[i].parentNode);
>+        node[i].parentNode.removeChild(node[i]);
>+      }
>+      var ind = node.length - 1;
>+      var selectNode = this.nextSibling[ind];
>+      if (!selectNode) selectNode = node[ind].previousSibling;
>+      if (!selectNode) selectNode = this.parentNode[ind];
>       viewer.selectElementInTree(selectNode);
>     }
>   },
Ok, so there are several problems here:
1) you should rename all occurences of node to nodes since it's an array
2) you save all the nodes and their next siblings, but you might end up deleting the next siblings, which means you need to undo in reverse order
3) when determining the node to select you get its previous sibling although the node has already been deleted by then
Note: if you're careful, you might find it easier to delete in reverse order

>+    viewer.selectElementInTree(this.node[0]);
I guess there's no easy way to select all the undeleted nodes?

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: Revised patch

r=timeless

>address np's comment
check - see remarks below

>Oops ;-)
Indeed

>1) you should rename all occurences of node to nodes since it's an array
check - you didn't have me do that on Bug 341256, and it hasn't committed, so if you want me to fix that there, let me know.

>2) you save all the nodes and their next siblings, but you might end up
>deleting the next siblings, which means you need to undo in reverse order
fixed by keeping track of previous siblings

>3) when determining the node to select you get its previous sibling although
>the node has already been deleted by then
take next now (at least I think I addressed this properly)

>I guess there's no easy way to select all the undeleted nodes?
not without rewriting parts of selectElementInTree, but I haven't been able to get my head around that code yet, so I'll go with a no comment for now.

After having some issues, I finally got this working.  I could not delete backwards due to some length complications that occurred with undo.  So, delete goes up the tree, and undo goes down.  Basically, if you deleted a child node of another node you were deleting, problems occurred on undo.

Since I'm keeping track of previousSiblings now instead of nextSiblings, I have to have the conditioning in there to make sure the previousSibling exists for appendChild/insertBefore.

Comment 11

[[e-mail]]

Comment on attachment 226070 [details] [diff] [review]
Revised patch

>+      var ind = nodes.length - 1;
>+      var selectNode = this.previousSiblings[ind];
>+      if (!selectNode) selectNode = nodes[ind].nextSibling;
>+      if (!selectNode) selectNode = this.parentNodes[ind];
Ah, but you're still referring to the last deleted node's next sibling after you've actually deleted. You're now also preferring the previous sibling.

>+        if (this.previousSiblings[i])
>+          this.parentNodes[i].insertBefore(this.nodes[i],
>+                                           this.previousSiblings[i].nextSibling);
>+        else
>+          this.parentNodes[i].appendChild(this.nodes[i]);
Except if the node had no previous sibling this means it was the first node...

Comment 12

[[e-mail]]

(In reply to comment #10)
>1) you should rename all occurences of node to nodes since it's an array
>check - you didn't have me do that on Bug 341256, and it hasn't committed, so
>if you want me to fix that there, let me know.
I admit, I overlooked it, so that would be good.
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).

Comment 13

[[e-mail]]

Comment on attachment 226070 [details] [diff] [review]
Revised patch

>       viewer.selectElementInTree(selectNode);
>-      node.parentNode.removeChild(node);
It turns out that this is a crucial point in the old code; you must select the new node before removing the old node, otherwise the right hand panel flips into JS Object view. And I think the previousSibling stuff is wrong anyway.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> (From update of attachment 226070 [details] [diff] [review] [edit])
> >       viewer.selectElementInTree(selectNode);
> >-      node.parentNode.removeChild(node);
> It turns out that this is a crucial point in the old code; you must select the
> new node before removing the old node, otherwise the right hand panel flips
> into JS Object view. And I think the previousSibling stuff is wrong anyway.

Yeah, I've noticed that but haven't had much time to actually work on this.  This bug has been a lot more difficult than I had originally anticipated.  Hope to have something by the end of next week.

Comment 15

[Shawn Wilsher :sdwilsh]

So, I have put a lot of thought into how the best way to accomplish this might be.  As of now, there are a few issues:
1) Currently discussed algorithms neglect the case that a node is selected that is  the parent to another node that is also selected.  Ideally, we should check for this and just delete the parent since the child nodes won't matter anymore.  This has caused much grief for me.
2) Going up or down the tree for deleting has serious complications for undo.  Storing either the previous sibling or the next sibiling isn't reliable as you could possible delete one or both leaving you with no reference node for reinsertion.
3) There is no easy way to select the previously selected nodes upon undo when there are  with the current code.  It will work fine with only one node selected.

These problems are not impossible, but it isn't easy to address all of them.

To deal with issue #2, we could possible store the parent node if more than one node is deleted.  For undo, we simply replae the new parent node with the old one and we'll be ok.  This would be difficult to implement, but I think it is possible.

As for #3, well the code to select a node seemed really complex to begin with, so I'm not exactly eager to tackle that.  I'm open to suggestions.

Comment 16

[Shawn Wilsher :sdwilsh]

So, there's no good way to determine a common parent node between an arbitrary number of nodes.  Would it be a bad idea to store the whole document for undo?

In that case, I'd have to figure out how to select all the nodes again upon undo, but I think that's possible (should be, just a matter of figuring it out).

Comment 17

[Jason Barnabe (np)]

(In reply to comment #16)
> So, there's no good way to determine a common parent node between an arbitrary
> number of nodes. 

Do you mean ancestor? If so, I can think of an algorithm, but no, I can't think of any existing magic function that would do it.

>Would it be a bad idea to store the whole document for undo?

So you'd make a copy of the document? Sounds expensive. Also, is it even possible to swap out one document for another?

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> (In reply to comment #16)
> > So, there's no good way to determine a common parent node between an arbitrary
> > number of nodes. 
> 
> Do you mean ancestor? If so, I can think of an algorithm, but no, I can't think
> of any existing magic function that would do it.

Yes, I meant ancestor, and I haven't had much coming up with an algorithm.  I'll implement it if you want to toss out some pointers.

> >Would it be a bad idea to store the whole document for undo?
> 
> So you'd make a copy of the document? Sounds expensive. Also, is it even
> possible to swap out one document for another?

I haven't tested it, but I was afraid it was too expensive.  Ideally, finding the common ancestor is probably our best bet.

Comment 19

[Jason Barnabe (np)]

while (node1) {
  while (node2) {
    if (node1 == node2) {
      return node1;
    }
    node2 = node2.parentNode;
  }
  node1 = node1.parentNode;
}
//if you get here, they're somehow not in the same document


For a third node, just use the result of that function as  the first argument, and the third node as the second argument.

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

This is much nicer.  Not sure if it's complete yet, but it's certainly a step in the right direction.

Comment 21

[Shawn Wilsher :sdwilsh]

So there is at least one issue with this.  When you select more than one node, when you undo, the ancestor is not expanded (we don't know how far we'd have to expand and what not).  I don't know of an easy way around this.

New patch to address timeless's irc review momentairly.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

Addresses comments on irc.

Some test cases (all using the minefield start page)

1) Expand the div with id "container"
2) Select a node and delete it
3) Undo it
Expected results:
node is reslected and reinserted (same behavior before this patch)

1) Expand the div with id "container"
2) Select any two nodes in it and delete them
Expected results:
Nodes go away, div#container is selected
3) Undo it
Expected results:
Nodes come back, div#container is not expanded

1) Expand the div with id "container"
2) Select one or more nodes in it
3) Select it
4) Delete
Expected results:
Next sibling of div#container is selected, div#container is deleted
5) Undo
Expected results:
div#container reappears, and is selected (not expanded).

Comment 23

[Shawn Wilsher :sdwilsh]

It should be noted that that patch doesn't quite work because I forgot a closing }.  I fixed it locally, but won't submit a new patch.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 270844 [details] [diff] [review]
v2.1

This doesn't work with redo.

Comment 25

[Shawn Wilsher :sdwilsh]

I'm starting to think that this bug is impossible to do with the undo/redo stack...

Comment 26

[Shawn Wilsher :sdwilsh]

I'm done with this for awhile (per comment #25).

If someone feels ambitious, please take a look.

Comment 27

[Yar]

(In reply to comment #2)
> Yes, that part was fixed by bug 297660.
> Next time, please only use the most recent trunk build (Mozilla1.7 is well over
> a year old) to test if the bug still exists.
> http://ftp.scarlet.be/pub/mozilla.org/firefox/nightly/latest-trunk/
> 
> The multiple selection not deleting the whole multiple selection is a valid bug,
> though.

Guys, please, take a look at this bug. It has been hanging here for a while and is still reproducible in Firefox 3.6.3 for windows XP.

Comment 28

[Colby Russell :crussell (no longer Mozilla-ing)]

Of course it's reproducible. It's written to work that way.

Comment 29

[Colby Russell :crussell (no longer Mozilla-ing)]

So I have this working, mostly, but bz says (via the newsgroups) that it's not possible to delete an anonymous element that's a child of the bound element. I guess the thing to do here, as with attempted deletion of a document node, is to delete what's possible, and tell the user about whatever we couldn't delete. We can get around notifying the user, though, in cases where the bound element is going to be removed as well, since it will be effectively the same as if we had removed the anon first.

Comment 30

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to http://groups.google.com/group/mozilla.dev.tech.xbl/browse_frm/thread/c1ec57e66448b02f)
> I'd start off by not allowing deletion of anonymous content for now,
> probably....  There's really just no way to do it, and often enough it's not
> even clear what it would mean to do it.
> 
> -Boris 

I can delete anonymous content with the DOM Inspector today, so long as its parent is another anon, and not the bound element itself (maybe you were referring to the children of bound elements I originally asked about, though). It seems to me to be technically the same as if the binding were removing nodes itself. It might be silly to do so, but just as silly as deleting arbitrary nodes on a Web page that might have script relying on its existence. Node deletion is already kind of a user-beware situation.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> so long as its parent is another anon

Or rather so long as it's a DOM child of its parent (so not anonymous wrt its parent).

> It seems to me to be technically the same as if the binding were removing nodes
> itself.

With your "so long" caveat, yes.

A binding can't remove root anonymous content nodes, though.

Comment 32

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: delete for multiple select with contiguous and non-contiguous regions

Might as well get this out there.

Comment 33

[Colby Russell :crussell (no longer Mozilla-ing)]

Oh yeah, there are some other changes here besides delete:
*XMLSerializer has been around since at least Mozilla 1.2, I think, so I dropped _toXML
*Non-Delete commands' criteria in isCommandEnabled were changed as well
*If no command is enabled in a nested menupopup (e.g., Paste), the menupopup is disabled altogether.

Comment 34

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 443302 [details] [diff] [review]
delete for multiple select with contiguous and non-contiguous regions

(From update of attachment 443302 [details] [diff] [review])
>+  function DVr_TransactionExecuting(aData, aMethod, aBegin)

Ho ho! Turns out we have nsITransactionListener for this.

Comment 35

[Colby Russell :crussell (no longer Mozilla-ing)]

So here's the deal, there's nothing wrong with this patch (well, as far as unreviewed patches go). It works. Everyone could have multi-node deletion tomorrow if the review and superreview came fast enough, and there were no nits about the patch, and I haven't overlooked anything.

But my solution was to create a transactionExecuting method on the viewer that is called once before the transaction method goes through, and once after. Fun plan, but this is exactly what nsITransactionListener is for, and so it should be used instead. The problem is that to use transaction listeners, we have to alter the inspector binding to get them registered. This is all right, but we've got a fine mess of how commands are executed due to bug 179621 being partially fixed.

Getting the binding to register listeners before fixing bug 179621 would create two headaches:
  1. Figuring out what needs to be hacked around in the inspector binding, and
  2. Probably making Phase 3 of bug 179621 more complicated.

Instead of doing this, bug 179621 needs to be resolved first, then we can proceed with multi-node deletion goodness. (And beyond that, the rest of the DOM Nodes viewer commands, bug 546170, and commands from any other viewers that do silly stuff like misuse currentIndex—bug 5333127—or assume that the originating viewer is still loaded when it's not—no bug filed for this, but I've mentioned it in bug 546170.)

Comment 36

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: use nsITransactionListeners, make some changes to the way undo delete works and onItemSelected

Other stuff that I didn't mention in the last patch:
make the viewer currentIndex-clean, especially in isCommandEnabled
use isCommandEnabled for viewer commands like cmdBlink and cmdInspectInNewWindow
change it so that the aforementioned commands are all handled by the same function, instead of different ones called by separate onpopupshowing attributes

No r?/sr? for now, because this relies on the patches for bug 179621, and I'm waiting on sr+ from Neil on attachment 445635 [details] [diff] [review].

Comment 37

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: diff against 179621 patch that got checked in; other changes

This is pretty much the same as attachment 447398 [details] [diff] [review], but I got rid of the pending listener stuff in the inspector binding by moving the transaction manager instantiation code out of the initialize method and into the constructor, which is the way I should have done it for bug 179621. I also clarified some comments and made it so that the paste commands besides paste before and after operate on the selection instead of currentIndex. This differs from the last patch, because the only paste command that I had operating on the selection was paste replace. There are two small changes from the last patch regarding showNodeInTree.

Some notes about the overall changes:
This is much larger than Shawn's patch; it increases overall file size by like 40%. I think the biggest causes are:
  1. My selection algorithm behaves differently.
  2. Shawn was finding a common ancestor and storing snapshot of the tree.

And if it makes it any better, about a third of that growth is from comments.

There's room for optimization in this, too. For example, we can reduce the number of DOM operations in the event that a deleted node is the ancestor of another deleted node by just deleting that node. There are a lot of things like that, but I'd rather get this big chunk checked in, eventually fix the other commands to work with multiple selection, and then make those kinds of incremental improvements, including recovery from external manipulation. (E.g., you insert/delete/paste some nodes, then are moving through the undo/redo and surprise!, some other script has been messing with the DOM and the sibling/parent lists we created for that operation are not up to date.)

Comment 38

[Shawn Wilsher :sdwilsh]

You might want to have Neil review this.  It's going to be a few weeks before I can still (sorry!).

Comment 39

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: Circumvent bitrot from 578775 landing; remove some cruft I included by accident in last patch, and tweak some comments

Comment 40

[[e-mail]]

Comment on attachment 461734 [details] [diff] [review]
Circumvent bitrot from 578775 landing; remove some cruft I included by accident in last patch, and tweak some comments

>-
>-          var tmClassID = "@mozilla.org/transactionmanager;1";
>-          var tmIface = "nsITransactionManager";
>-          this.mTransactionManager = XPCU.createInstance(tmClassID, tmIface);
...
>+
>+        this.mTransactionManager =
>+          XPCU.createInstance("@mozilla.org/transactionmanager;1",
>+                              "nsITransactionManager");
[Why this change?]

>+  mDisableSelectionChangeCount: 0,
[Might be better named mSelectionBatchNestCount]

>-      selectedNode = new XPCNativeWrapper(viewer.selectedNode, "nodeType",
>+      currentNode = new XPCNativeWrapper(viewer.currentNode, "nodeType",
>                                           "parentNode", "childNodes");
>-      if (selectedNode.parentNode) {
>-        parentNode = new XPCNativeWrapper(selectedNode.parentNode,
>+      if (currentNode.parentNode) {
>+        parentNode = new XPCNativeWrapper(currentNode.parentNode,
>                                           "nodeType");
As I recall, the versions of Gecko that we currently support all provide the 1-arg XPCNativeWrapper version that explicitly wraps its derived properties, so we can fix this to
currentNode = new XPCNativeWrapper(viewer.currentNode);
parentNode = currentNode.parentNode;

>+      case "cmdInspectBrowser":
>+        var selectedNode = this.selectedNode;
>+        if (!(selectedNode instanceof nsIDOMElement)) {
>+          return false;
>+        }
>+    
>+        var n = selectedNode.localName.toLowerCase();
>+        return n == "tabbrowser" || n == "browser" || n == "iframe" ||
>+               n == "frame" || n == "editor";
[This is hardly an ideal test... it's not easy to think of a good one though]

>+    var command = unwrapJSObject(aTransaction);
This is flawed. aTransaction isn't a content dom node, which is what unwrapJSObject was designed for. Just use .wrappedJSObject directly.

>+      // The approach is broken down as follows:
>+      // 1. Locating a nearby node that's visible in the tree and won't
>+      //    disappear.  Choose this as our guess.
>+      // 2. Verifying that the guess isn't a descendant of a node that will
>+      //    deleted.
The problem with this is that if it turns out you're deleting the parent of your guess, you will keep guessing all the children until you run out.

The way around this is to start with the current node, and look at its ancestors, and see whether any of them are in the deletable nodes list. Choose the highest deletable ancestor (if any) as your initial guess. Then (unless your guess itself is deletable) you will know that you will never guess a node with a deletable ancestor. In pseudocode:
var guess = this.currentNode;
for (var temp = guess; temp != root; temp = temp.parentInView)
  if (temp in deleteableNodes)
    guess = temp;

>+                this.getRowIndexFromNode(guess) >= 0) {
guess is a sibling of a visible node. So it will always be visible.

>+  getNearestSelectedIndex: function DVr_GetNearestSelectedIndex(aRowIndex)
>+  {
>+    var guessIndex, guessDistance = Number.POSITIVE_INFINITY;
>+    var indexes = this.getSelectedIndexes();
>+    for (let i = 0, n = indexes.length; i < n; ++i) {
>+      var index = indexes[i];
>+      var distance = Math.abs(aRowIndex - index);
>+      if (distance < guessDistance) {
>+        guessDistance = distance;
>+        guessIndex = index;
>+      }
>+    }
>+    return guessDistance == Number.POSITIVE_INFINITY ? -1 : guessIndex;
You can do much better than this because getSelectedIndexes always returns indexes in ascending order. There are four easy cases: 1) no selected nodes 2) one selected node 3) row on or before first selected 4) row on or after last selected. Otherwise do a (binary) search, and when you are reduced to two rows (one before, one after) return the nearest.

>+      var subject = cmd || el;
>+      if (isEnabled) {
>+        hasEnabled = true;
>+        if (subject) {
el always exists so subject does too.

>+        if (checkValid) {
>+          el.removeAttribute("hidden");
>+        }
[Could write this as
if (checkValid)
  el.hidden = !isEnabled;
]

>+  this.constructor = cmdEditDelete;
[Does setting this on the prototype not work?]

>+cmdEditDelete.sortComparator = function Delete_SortComparator(a, b)
[See below]

>+  if (!aAncestor || aAncestor == rootNode) {
>+    return 1;
>+  }
>+  return -1;
What about cousin nodes?

>+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure)
>+cmdEditDelete.NODE_NULL = 1;
>+cmdEditDelete.NO_PARENT = 2;
>+cmdEditDelete.NOT_EXPLICIT_CHILD = 3;
Why are these set on the function and not on the prototype?

>+  // We can't use XPCU, since globals won't be in our scope when this viewer
>+  // is destroyed.  So we have this instance method to log to the console, and
>+  // set up the console service on the first call.
Why is this running after the viewer is destroyed?

>+        // Can't use global class/interface constants, either.
>+        var iface = Components.interfaces.nsIConsoleService;
>+        var classID = "@mozilla.org/consoleservice;1";
>+        this.mConsoleService = Components.classes[classID].getService(iface);
Can't use Components either, so there.

>+      this.logString(aMessage);
Infinite loop in the failure case.

Comment 41

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #40)
> Comment on attachment 461734 [details] [diff] [review]
> Circumvent bitrot from 578775 landing; remove some cruft I included by accident
> in last patch, and tweak some comments
> 
> >-
> >-          var tmClassID = "@mozilla.org/transactionmanager;1";
> >-          var tmIface = "nsITransactionManager";
> >-          this.mTransactionManager = XPCU.createInstance(tmClassID, tmIface);
> ...
> >+
> >+        this.mTransactionManager =
> >+          XPCU.createInstance("@mozilla.org/transactionmanager;1",
> >+                              "nsITransactionManager");
> [Why this change?]

It was supposed to prevent a race condition that it seems will never occur.

> >+    var command = unwrapJSObject(aTransaction);
> This is flawed. aTransaction isn't a content dom node, which is what
> unwrapJSObject was designed for. Just use .wrappedJSObject directly.

Dang, I thought XPCNativeWrapper.unwrap was supposed to solve all of the world's problems.

> >+      // The approach is broken down as follows:
> >+      // 1. Locating a nearby node that's visible in the tree and won't
> >+      //    disappear.  Choose this as our guess.
> >+      // 2. Verifying that the guess isn't a descendant of a node that will
> >+      //    deleted.
> The problem with this is that if it turns out you're deleting the parent of
> your guess, you will keep guessing all the children until you run out.
> 
> The way around this is...

Ah! When I considered this, I mistook it for just trading a breadth-oriented approach for a depth-oriented one with respective disadvantages.

> >+                this.getRowIndexFromNode(guess) >= 0) {
> guess is a sibling of a visible node. So it will always be visible.

What makes you so sure? The same path is taken for redo, as well as do. When you redo, it's possible that you've since collapsed an ancestor row. You're not guaranteed that any of the nodes have corresponding rows visible in the tree, since cmdEditDelete by design operates only on nodes, and not rows of a tree view.

> >+cmdEditDelete.sortComparator = function Delete_SortComparator(a, b)
> [See below]
> 
> >+  if (!aAncestor || aAncestor == rootNode) {
> >+    return 1;
> >+  }
> >+  return -1;
> What about cousin nodes?

Not important. The clash we're trying to avoid by using the special ordering doesn't exist for cousins. (Since we use the next sibling as an insertion reference, the clash we're talking about here would be to try to do nodeParent.insertBefore(undeletedNode, insertionRef) to restore undeletedNode before we've made sure insertionRef has been restored, in the event that it was also deleted.)

Suppose we have the following tree:

A
  B
  C
D
E
  F
G

I think what you're pointing out is that the sort comparator can result in this being ordered as B,C,F,A,D,E,G or F,B,C,A,D,E,G. The good news is we don't care; our deletion/undeletion algorithm should handle both.

> >+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure)
> >+cmdEditDelete.NODE_NULL = 1;
> >+cmdEditDelete.NO_PARENT = 2;
> >+cmdEditDelete.NOT_EXPLICIT_CHILD = 3;
> Why are these set on the function and not on the prototype?

They don't have much to do with the instances. The constants are there for reasons similar to why Number.MAX_VALUE isn't Number.prototype.MAX_VALUE in JS. I'd argue that the same is enough of a reason to justify isDeletable being there as well, from a high-level perspective. And now that I think about it, the viewer does use isDeletable as a "static" method where no instance exists. See isCommandEnabled for cmdEditDelete.

> >+  // We can't use XPCU, since globals won't be in our scope when this viewer
> >+  // is destroyed.  So we have this instance method to log to the console, and
> >+  // set up the console service on the first call.
> Why is this running after the viewer is destroyed?

Recall that viewer panes are just browsers loading individual viewer XUL (which load viewer-specific JS executing in the scope of that viewer). When a command is invoked, the viewer gives its parent panelset that command, which is an nsITransaction. The transaction manager handling all of this is internal to the panelset, and so stays relevant for the duration of the inspector window's life. Even after you've switched from, e.g., the DOM Node viewer to the Style Rules viewer, the transaction manager is still holding refs in its stack to any commands the user did while the DOM Node viewer was open.

Assuming in the transaction logic that |viewer| is always going to be there is a problem I've mentioned a couple of times, probably most visibly in bug 546170.

---

You know, I'd written the above, then tested the code in question, and I'm getting errors when I try to undo after switching to another viewer. (Although I could have sworn I had this working at some point...).

TypeError: (void 0) is not a constructor

It's complaining about this line:

this.mParents[idx].insertBefore(this.nodes[idx], this.mSiblings[idx]);

Specifically, trying to access insertBefore (or anything else, really) on this.mParents[idx] gives the error. I would *think* this would be fine, even after original viewer has unloaded, because mParents (and mSiblings, etc.) are just holding refs to nodes in a document that still exists (the inspected document; i.e., not the viewer that was unloaded). But I guess not.

This seems like a "big picture" problem wrt to the way the command objects are handed about and stick around even after the viewer is destroyed. (Some DOM candy-coating that knows about these as elements/their protos are getting clobbered or something when we unload? I dunno.) Any hints on tweaks to accomplish the desired effect? I *really* think we should be able to undo/redo without regard to whether we may or may not actually have some particular view of the data.

Diddling the DOM via JS objects that were put in place before unloading works fine after the viewer is destroyed for the CSS Rules viewer, FWIW.

> 
> >+      this.logString(aMessage);
> Infinite loop in the failure case.

Huh? Where?

Comment 42

[[e-mail]]

(In reply to comment #41)
> (In reply to comment #40)
> > This is flawed. aTransaction isn't a content dom node, which is what
> > unwrapJSObject was designed for. Just use .wrappedJSObject directly.
> Dang, I thought XPCNativeWrapper.unwrap was supposed to solve all of the
> world's problems.
It only unwraps XPCNativeWrappers, not XPCWrappedNatives. Easy mistake ;-)

> > >+                this.getRowIndexFromNode(guess) >= 0) {
> > guess is a sibling of a visible node. So it will always be visible.
> What makes you so sure? The same path is taken for redo, as well as do.
Thanks for pointing that out.

> I think what you're pointing out is that the sort comparator can result in this
> being ordered as B,C,F,A,D,E,G or F,B,C,A,D,E,G. The good news is we don't
> care; our deletion/undeletion algorithm should handle both.
Thanks for explaining.

> > >+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure)
> > >+cmdEditDelete.NODE_NULL = 1;
> > >+cmdEditDelete.NO_PARENT = 2;
> > >+cmdEditDelete.NOT_EXPLICIT_CHILD = 3;
> > Why are these set on the function and not on the prototype?
> They don't have much to do with the instances. The constants are there for
> reasons similar to why Number.MAX_VALUE isn't Number.prototype.MAX_VALUE in JS.
I guess I'm too used to xpconnect exposing interface enums on the instances.

> This seems like a "big picture" problem wrt to the way the command objects are
> handed about and stick around even after the viewer is destroyed. (Some DOM
> candy-coating that knows about these as elements/their protos are getting
> clobbered or something when we unload? I dunno.) Any hints on tweaks to
> accomplish the desired effect?
The only way I know to avoid this is not to use objects created in a window scope. Instead you need one of a) C++ objects b) JS components c) JS modules.

> Diddling the DOM via JS objects that were put in place before unloading works
> fine after the viewer is destroyed for the CSS Rules viewer, FWIW.
You might still get assertions in a debug build though.

> > >+      this.logString(aMessage);
> > Infinite loop in the failure case.
> Huh? Where?
If the getService fails, then you just keep trying over and over again.

Comment 43

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 461734 [details] [diff] [review]
I want to split off some of this before having another go.
  Bug 589299 - Make the DOM Nodes viewer currentIndex-clean.
  Bug 589300 - Consolidate onCommandPopupShowing and onContextCreate in DOM
               Nodes viewer

(In reply to comment #42)
> > This seems like a "big picture" problem wrt to the way the command objects are
> > handed about and stick around even after the viewer is destroyed. (Some DOM
> > candy-coating that knows about these as elements/their protos are getting
> > clobbered or something when we unload? I dunno.) Any hints on tweaks to
> > accomplish the desired effect?
> The only way I know to avoid this is not to use objects created in a window
> scope. Instead you need one of a) C++ objects b) JS components c) JS modules.

Then I'll probably just change the inspector—viewer contract to allow viewers to ask the inspector to load their (non-transient) transaction definitions in the inspector scope, or maybe in less ephemeral sandboxes.  Although I'll probably just disregard the fact that trying to undo won't work if you've unloaded the transaction's originating viewer.  All of that's already broken right now anyway.

Comment 44

[Colby Russell :crussell (no longer Mozilla-ing)]

I'll say it again: anonymous content ruins everything.

Here's the comment describing a problem I identified sometime between that last comment I left in August and in the last few days when I started looking at getting this fixed again:
  // XXX If we delete a child of an anonymous node and that anonymous node's
  // binding parent, the nodes array and the mParents array (and potentially
  // the mSiblings array) will continue to needlessly reference nodes from
  // that anonymous subtree; even if the binding parent ever gets restored
  // via undo, a new anonymous content tree will be created for it.  So we
  // should drop our references to the nodes in the old anonymous content
  // tree.

  // We can even go further than that.  The saved child-of-anonymous node
  // isn't going to get reinserted anywhere useful, so why reinsert it?
  // What should we do in that case?  Sync the attributes from the old anon
  // to the corresponding node in the new content tree, then update our ref
  // to that one?

I've been considering how to approach this off and on for the last day or so.  In the last few minutes, I think I've convinced myself that the following wouldn't be so awful:

Check to see if any of the deleted nodes' binding parents are also going to be deleted.  If so, just ignore any selected anons from that binding's tree wrt to the stuff we keep track of to make undo work, and prompt with, "Some of the selected nodes are part of an anonymous subtree that will be destroyed.  Deleting those anonymous nodes cannot be undone.  Would you like to continue?".

Comment 45

[[e-mail]]

(In reply to comment #44)
> Check to see if any of the deleted nodes' binding parents are also going to be
> deleted.  If so, just ignore any selected anons from that binding's tree wrt to
> the stuff we keep track of to make undo work, and prompt with, "Some of the
> selected nodes are part of an anonymous subtree that will be destroyed. 
> Deleting those anonymous nodes cannot be undone.  Would you like to continue?".
I wouldn't bother with the prompt. We already have cases in which undo fails ;-)

Comment 46

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #45)
> I wouldn't bother with the prompt.

Aw, I already have it coded up, though.

> We already have cases in which undo fails

Mm, yeah, but to me that's not good and something to be avoided.  In other cases, when we fail, we fail hard or bizarrely.  Here, prompt aside, there'd be proactive checks to detect imminent failure and deal with it appropriately/at least eliminate (hopefully all of) the worst side effects.

Comment 47

[[e-mail]]

I was originally leaning the other way and thinking of warning when anonymous content is enabled and any of the nodes that you're deleting has a binding, but I guess that would get too annoying.

(In reply to comment #46)
> (In reply to comment #45)
> > I wouldn't bother with the prompt.
> Aw, I already have it coded up, though.
Don't worry, it wasn't a strong opinion.

Comment 48

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > >+      this.logString(aMessage);
> > > Infinite loop in the failure case.
> > Huh? Where?
> If the getService fails, then you just keep trying over and over again.

Huh?  If getService fails -> this.mConsoleService = null -> |"mConsoleService" in this| is true on the recursive call and this.mConsoleService == false in the next conditional -> dump(aMessage)

Comment 49

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: Another go, now that other parts have been split off and taken care of

Comment 50

[[e-mail]]

Comment on attachment 534183 [details] [diff] [review]
Another go, now that other parts have been split off and taken care of

>+    var withoutFilter = function DVr_GetNextInNextBestChain_IsIgnorable(aNode)
>+    {
>+      return viewer.isIgnorableNode(aNode);
>+    };
>+    var withFilter =
>+      function DVr_GetNextInNextBestChain_FilteredIsIgnorable(aNode)
>+    {
>+      return viewer.isIgnorableNode(aNode) || aFilterFn(aNode);
>+    };
Why write var <name> = function <name>...; instead of just function <name>...?

>+          // Flag this transaction as no good and that it should be discarded.
>+          this.canceled = true;
Throw an exception perhaps?

>+  if ("mConsoleService" in this) {
OK, so I must have misread this the previous time. Possibly if you'd written the condition the other way around it would have been more obvious:
if (!(("mConsoleService" in this)) {
  try ...
  catch ...
}
if (this.mConsoleService)
   ...
else
  ...

Comment 51

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #50)
> Comment on attachment 534183 [details] [diff] [review] [review]
> Another go, now that other parts have been split off and taken care of
> 
> >+    var withoutFilter = function DVr_GetNextInNextBestChain_IsIgnorable(aNode)
> >+    {
> >+      return viewer.isIgnorableNode(aNode);
> >+    };
> >+    var withFilter =
> >+      function DVr_GetNextInNextBestChain_FilteredIsIgnorable(aNode)
> >+    {
> >+      return viewer.isIgnorableNode(aNode) || aFilterFn(aNode);
> >+    };
> Why write var <name> = function <name>...; instead of just function
> <name>...?

So we can get the pseudo-namespacing with the function name without having to type out that name below.

> >+          // Flag this transaction as no good and that it should be discarded.
> >+          this.canceled = true;
> Throw an exception perhaps?
> 
> >+  if ("mConsoleService" in this) {
> OK, so I must have misread this the previous time. Possibly if you'd written
> the condition the other way around it would have been more obvious:
> if (!(("mConsoleService" in this)) {
>   try ...
>   catch ...
> }
> if (this.mConsoleService)
>    ...
> else
>   ...

Do you want me to do that?

Comment 52

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: address comment 50 and fix two other errors

I also found two errors regarding cmdEditDelete.  In the constructor, the condition for prompting used the return value from Array.prototype.indexOf instead of checking that it's greater than or equal to 0.  And in cmdEditDelete.sortComparator, I used a bitwise & instead of logical &&.

Comment 53

[[e-mail]]

Comment on attachment 535947 [details] [diff] [review]
address comment 50 and fix two other errors

>+          throw new Error("User canceled transaction");
[I think I've seen you do this somewhere else and I either forgot the question or the answer, so I'll ask again: any particular advantage in throwing an Error?]

>+    this.logString(aMessage);
>+  }
>+  else {
>+    // This is not the first call.
[Not quite what I suggested, which removed the recursive call and the else clause. If you don't like that then go back to the previous version.]

Comment 54

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #53)
> Comment on attachment 535947 [details] [diff] [review] [review]
> address comment 50 and fix two other errors
> 
> >+          throw new Error("User canceled transaction");
> [I think I've seen you do this somewhere else and I either forgot the
> question or the answer, so I'll ask again: any particular advantage in
> throwing an Error?]

:

(comment #50)
> >+          // Flag this transaction as no good and that it should be discarded.
> >+          this.canceled = true;
> Throw an exception perhaps?

:) (Unless your question is "why are you throwing a generic Error [instead of something more specific]?".)

> >+    this.logString(aMessage);
> >+  }
> >+  else {
> >+    // This is not the first call.
> [Not quite what I suggested, which removed the recursive call and the else
> clause. If you don't like that then go back to the previous version.]

Oops.  I wasn't paying attention.

Comment 55

[[e-mail]]

(In reply to comment #54)
> (Unless your question is "why are you throwing a generic Error [instead
> of something more specific]?".)
Actually I meant [instead of a string].

Comment 56

[Colby Russell :crussell (no longer Mozilla-ing)]

So throw a string?

Comment 57

[[e-mail]]

(In reply to comment #56)
> So throw a string?
Well, I had this vague memory of someone giving a reason to throw an Error rather than a string, and I was rather hoping it was you ;-)

Comment 58

[Colby Russell :crussell (no longer Mozilla-ing)]

File name, line number, and stack trace, maybe?

Comment 59

[[e-mail]]

(In reply to comment #58)
> File name, line number, and stack trace, maybe?
Ah yes, that was probably it. (Although seeing as you always catch it...)

Comment 60

[Colby Russell :crussell (no longer Mozilla-ing)]

Pushed: http://hg.mozilla.org/dom-inspector/rev/800b7694f8ed