Bug 1094622 - Deleting node in inspector now selects previous sibling and not parent. r=bgrins

---
 browser/devtools/markupview/markup-view.js         | 15 +++++----
 .../test/browser_markupview_tag_edit_04.js         | 38 +++++++++++++++-------
 browser/devtools/markupview/test/head.js           |  2 +-
 toolkit/devtools/server/actors/inspector.js        | 14 +++++++-
 .../tests/mochitest/test_inspector-remove.html     | 23 +++++++++----
 5 files changed, 65 insertions(+), 27 deletions(-)

browser/devtools/markupview/markup-view.js

@@ -521,16 +521,17 @@ MarkupView.prototype = {
     // Retain the node so we can undo this...
     this.walker.retainNode(aNode).then(() => {
       let parent = aNode.parentNode();
-      let sibling = null;
+      let nextSibling = null;
       this.undo.do(() => {
-        if (container.selected) {
-          this.navigate(this.getContainer(parent));
-        }
-        this.walker.removeNode(aNode).then(nextSibling => {
-          sibling = nextSibling;
+        this.walker.removeNode(aNode).then(siblings => {
+          let focusNode = siblings.previousSibling || parent;
+          nextSibling = siblings.nextSibling;
+          if (container.selected) {
+            this.navigate(this.getContainer(focusNode));
+          }
         });
       }, () => {
-        this.walker.insertBefore(aNode, parent, sibling);
+        this.walker.insertBefore(aNode, parent, nextSibling);
       });
     }).then(null, console.error);
   },

browser/devtools/markupview/test/browser_markupview_tag_edit_04.js

@@ -4,27 +4,41 @@
 
 "use strict";
 
-// Tests that a node can be deleted from the markup-view with the delete key
+// Tests that a node can be deleted from the markup-view with the delete key.
+// Also checks that after deletion the correct element is highlighted.
+// The next sibling is preferred, but the parent is a fallback.
 
-const TEST_URL = "data:text/html,<div id='delete-me'></div>";
+const TEST_URL = "data:text/html,<div id='parent'><div id='first'></div><div id='second'></div><div id='third'></div></div>";
 
-let test = asyncTest(function*() {
-  let {toolbox, inspector} = yield addTab(TEST_URL).then(openInspector);
+function* checkDeleteAndSelection(inspector, nodeSelector, focusedNodeSelector) {
+  yield selectNode(nodeSelector, inspector);
+  yield clickContainer(nodeSelector, inspector);
 
-  info("Selecting the test node by clicking on it to make sure it receives focus");
-  let node = content.document.querySelector("#delete-me");
-  yield clickContainer("#delete-me", inspector);
-
-  info("Deleting the element with the keyboard");
+  info("Deleting the element \"" + nodeSelector + "\" with the keyboard");
   let mutated = inspector.once("markupmutation");
   EventUtils.sendKey("delete", inspector.panelWin);
-  yield mutated;
+
+  yield Promise.all([mutated, inspector.once("inspector-updated")]);
+
+  let nodeFront = yield getNodeFront(focusedNodeSelector, inspector);
+  is(inspector.selection.nodeFront, nodeFront,
+    focusedNodeSelector + " should be selected after " + nodeSelector + " node gets deleted.");
 
   info("Checking that it's gone, baby gone!");
-  ok(!content.document.querySelector("#delete-me"), "The test node does not exist");
+  ok(!content.document.querySelector(nodeSelector), "The test node does not exist");
 
   yield undoChange(inspector);
-  ok(content.document.querySelector("#delete-me"), "The test node is back!");
+  ok(content.document.querySelector(nodeSelector), "The test node is back!");
+}
+
+let test = asyncTest(function*() {
+  let {inspector} = yield addTab(TEST_URL).then(openInspector);
+
+  info("Selecting the test node by clicking on it to make sure it receives focus");
+
+  yield checkDeleteAndSelection(inspector, "#first", "#parent");
+  yield checkDeleteAndSelection(inspector, "#second", "#first");
+  yield checkDeleteAndSelection(inspector, "#third", "#second");
 
   yield inspector.once("inspector-updated");
 });

browser/devtools/markupview/test/head.js

@@ -252,7 +252,7 @@ let clickContainer = Task.async(function*(selector, inspector) {
   let nodeFront = yield getNodeFront(selector, inspector);
   let container = getContainerForNodeFront(nodeFront, inspector);
 
-  let updated = inspector.once("inspector-updated");
+  let updated = container.selected ? promise.resolve() : inspector.once("inspector-updated");
   EventUtils.synthesizeMouseAtCenter(container.tagLine, {type: "mousedown"},
     inspector.markup.doc.defaultView);
   EventUtils.synthesizeMouseAtCenter(container.tagLine, {type: "mouseup"},

toolkit/devtools/server/actors/inspector.js

@@ -56,6 +56,7 @@ const protocol = require("devtools/server/protocol");
 const {Arg, Option, method, RetVal, types} = protocol;
 const {LongStringActor, ShortLongString} = require("devtools/server/actors/string");
 const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
+const {Task} = Cu.import("resource://gre/modules/Task.jsm", {});
 const object = require("sdk/util/object");
 const events = require("sdk/event/core");
 const {Unknown} = require("sdk/platform/xpcom");
@@ -2876,7 +2877,18 @@ var WalkerFront = exports.WalkerFront = protocol.FrontClass(WalkerActor, {
       walkerActor._orphaned.add(this.conn._transport._serverConnection.getActor(top.actorID));
     }
     return returnNode;
-  }
+  },
+
+  removeNode: protocol.custom(Task.async(function* (node) {
+    let previousSibling = yield this.previousSibling(node);
+    let nextSibling = yield this._removeNode(node);
+    return {
+      previousSibling: previousSibling,
+      nextSibling: nextSibling,
+    };
+  }), {
+    impl: "_removeNode"
+  }),
 });
 
 /**

toolkit/devtools/server/tests/mochitest/test_inspector-remove.html

@@ -28,6 +28,12 @@ function assertOwnership() {
   return assertOwnershipTrees(gWalker);
 }
 
+function ignoreNode(node) {
+  // Duplicate the walker logic to skip blank nodes...
+  return node.nodeType === Components.interfaces.nsIDOMNode.TEXT_NODE &&
+    !/[^\s]/.test(node.nodeValue);
+}
+
 addTest(function setup() {
   let url = document.getElementById("inspectorContent").href;
   attachURL(url, function(err, client, tab, doc) {
@@ -48,11 +54,15 @@ addTest(function testRemoveSubtree() {
   let longlistID = null;
 
   let nextSibling = gInspectee.querySelector("#longlist").nextSibling;
-  // Duplicate the walker logic to skip blank nodes...
-  while (nextSibling && nextSibling.nodeType === Components.interfaces.nsIDOMNode.TEXT_NODE && !/[^\s]/.exec(nextSibling.nodeValue)) {
+  while (nextSibling && ignoreNode(nextSibling)) {
     nextSibling = nextSibling.nextSibling;
   }
 
+  let previousSibling = gInspectee.querySelector("#longlist").previousSibling;
+  while (previousSibling && ignoreNode(previousSibling)) {
+    previousSibling = previousSibling.previousSibling;
+  }
+
   promiseDone(gWalker.querySelector(gWalker.rootNode, "#longlist").then(listFront => {
     longlist = listFront;
     longlistID = longlist.actorID;
@@ -62,13 +72,14 @@ addTest(function testRemoveSubtree() {
     originalOwnershipSize = assertOwnership();
     ok(originalOwnershipSize > 26, "Should have at least 26 items in our ownership tree");
     return gWalker.removeNode(longlist);
-  }).then(nextSiblingFront => {
-    is(nextSiblingFront.rawNode(), nextSibling, "Should have returned the next sibling.");
+  }).then(siblings => {
+    is(siblings.previousSibling.rawNode(), previousSibling, "Should have returned the previous sibling.");
+    is(siblings.nextSibling.rawNode(), nextSibling, "Should have returned the next sibling.");
     return waitForMutation(gWalker, isChildList);
   }).then(() => {
-    // Our ownership size should now be 26 fewer (we forgot about #longlist + 26 children, but learned about #longlist's next sibling)
+    // Our ownership size should now be 25 fewer (we forgot about #longlist + 26 children, but learned about #longlist's prev/next sibling)
     let newOwnershipSize = assertOwnership();
-    is(newOwnershipSize, originalOwnershipSize - 26, "Ownership tree should have dropped by 27 nodes");
+    is(newOwnershipSize, originalOwnershipSize - 25, "Ownership tree should have dropped by 25 nodes");
     // Now verify that some nodes have gone away
     return checkMissing(gClient, longlistID);
   }).then(runNextTest));