Bug 1198039 - Don't modify the DOM after dragging an element and putting it back where it started;r=pbrosset

--HG--
extra : commitid : Loe49p9JYT7

devtools/client/markupview/test/browser_markupview_dragdrop_reorder.js

@@ -23,6 +23,11 @@ add_task(function*() {
 
   parentContainer.elt.scrollIntoView(true);
 
+  info("Testing putting an element back in it's original place");
+  yield dragElementToOriginalLocation("#firstChild", inspector);
+  is(parent.children[0].id, "firstChild", "#firstChild is still the first child of #test");
+  is(parent.children[1].id, "middleChild", "#middleChild is still the second child of #test");
+
   info("Testing switching elements inside their parent");
   yield moveElementDown("#firstChild", "#middleChild", inspector);
 
@@ -95,6 +100,27 @@ function* dragContainer(selector, targetOffset, inspector) {
   return updated;
 };
 
+function* dragElementToOriginalLocation(selector, inspector) {
+  let el = yield getContainerForSelector(selector, inspector);
+  let height = el.tagLine.getBoundingClientRect().height;
+
+  info("Picking up and putting back down " + selector);
+
+  function onMutation() {
+    ok(false, "Mutation received from dragging a node back to its location");
+  }
+  inspector.on("markupmutation", onMutation);
+  yield dragContainer(selector, {x: 0, y: 0}, inspector);
+
+  // Wait a bit to make sure the event never fires.
+  // This doesn't need to catch *all* cases, since the mutation
+  // will cause failure later in the test when it checks element ordering.
+  yield new Promise(resolve => {
+    setTimeout(resolve, 500);
+  });
+  inspector.off("markupmutation", onMutation);
+}
+
 function* moveElementDown(selector, next, inspector) {
   let onMutated = inspector.once("markupmutation");
   let uiUpdate = inspector.once("inspector-updated");
@@ -106,6 +132,7 @@ function* moveElementDown(selector, next, inspector) {
 
   yield dragContainer(selector, {x: 0, y: Math.round(height) + 2}, inspector);
 
-  yield onMutated;
+  let mutations = yield onMutated;
+  is(mutations.length, 2, "2 mutations");
   yield uiUpdate;
 };

devtools/server/actors/inspector.js

@@ -2662,7 +2662,23 @@ var WalkerActor = protocol.ActorClass({
       return null;
     }
 
-    parent.rawNode.insertBefore(node.rawNode, sibling ? sibling.rawNode : null);
+    let rawNode = node.rawNode;
+    let rawParent = parent.rawNode;
+    let rawSibling = sibling ? sibling.rawNode : null;
+
+    // Don't bother inserting a node if the document position isn't going
+    // to change. This prevents needless iframes reloading and mutations.
+    if (rawNode.parentNode === rawParent) {
+      let currentNextSibling = this.nextSibling(node);
+      currentNextSibling = currentNextSibling ? currentNextSibling.rawNode :
+                                                null;
+
+      if (rawNode === rawSibling || currentNextSibling === rawSibling) {
+        return;
+      }
+    }
+
+    rawParent.insertBefore(rawNode, rawSibling);
   }, {
     request: {
       node: Arg(0, "domnode"),

devtools/server/tests/mochitest/test_inspector-insert.html

@@ -39,36 +39,63 @@ addTest(function setup() {
   });
 });
 
-addTest(function testRearrange() {
-  let longlist = null;
-  let nodeA = null;
-  let nextNode = null;
+addAsyncTest(function* testRearrange() {
+  let longlist = yield gWalker.querySelector(gWalker.rootNode, "#longlist");
+  let children = yield gWalker.children(longlist);
+  let nodeA = children.nodes[0];
+  is(nodeA.id, "a", "Got the expected node.");
 
-  promiseDone(gWalker.querySelector(gWalker.rootNode, "#longlist").then(listFront => {
-    longlist = listFront;
-  }).then(() => {
-    return gWalker.children(longlist);
-  }).then(response => {
-    nodeA = response.nodes[0];
-    is(nodeA.id, "a", "Got the expected node.");
-    // Move nodeA to the end of the list.
-    return gWalker.insertBefore(nodeA, longlist, null);
-  }).then(() => {
-    ok(!gInspectee.querySelector("#a").nextSibling, "a should now be at the end of the list.");
-    return gWalker.children(longlist);
-  }).then(response => {
-    is(nodeA, response.nodes[response.nodes.length - 1], "a should now be the last returned child.");
-    // Now move it to the middle of the list.
-    nextNode = response.nodes[13];
-    return gWalker.insertBefore(nodeA, longlist, nextNode);
-  }).then(response => {
-    let sibling = new inspector._documentWalker(gInspectee.querySelector("#a"), window).nextSibling();
-    is(sibling, nextNode.rawNode(), "Node should match the expected next node.");
-    return gWalker.children(longlist);
-  }).then(response => {
-    is(nodeA, response.nodes[13], "a should be where we expect it.");
-    is(nextNode, response.nodes[14], "next node should be where we expect it.");
-  }).then(runNextTest));
+  // Move nodeA to the end of the list.
+  yield gWalker.insertBefore(nodeA, longlist, null);
+  ok(!gInspectee.querySelector("#a").nextSibling, "a should now be at the end of the list.");
+  children = yield gWalker.children(longlist);
+  is(nodeA, children.nodes[children.nodes.length - 1], "a should now be the last returned child.");
+
+  // Now move it to the middle of the list.
+  let nextNode = children.nodes[13];
+  yield gWalker.insertBefore(nodeA, longlist, nextNode);
+  let sibling =
+    new inspector._documentWalker(gInspectee.querySelector("#a"), window).nextSibling();
+  is(sibling, nextNode.rawNode(), "Node should match the expected next node.");
+  children = yield gWalker.children(longlist);
+  is(nodeA, children.nodes[13], "a should be where we expect it.");
+  is(nextNode, children.nodes[14], "next node should be where we expect it.");
+
+  runNextTest();
+});
+
+addAsyncTest(function* testInsertInvalidInput() {
+  let longlist = yield gWalker.querySelector(gWalker.rootNode, "#longlist");
+  let children = yield gWalker.children(longlist);
+  let nodeA = children.nodes[0];
+  let nextSibling = children.nodes[1];
+
+  // Now move it to the original location and make sure no mutation happens.
+  let hasMutated = false;
+  let observer = new gInspectee.defaultView.MutationObserver(() => {
+    hasMutated = true;
+  });
+  observer.observe(longlist.rawNode(), {
+    childList: true,
+  });
+
+  yield gWalker.insertBefore(nodeA, longlist, nodeA);
+  ok(!hasMutated, "hasn't mutated");
+  hasMutated = false;
+
+  yield gWalker.insertBefore(nodeA, longlist, nextSibling);
+  ok(!hasMutated, "still hasn't mutated after inserting before nextSibling");
+  hasMutated = false;
+
+  yield gWalker.insertBefore(nodeA, longlist);
+  ok(hasMutated, "has mutated after inserting with null sibling");
+  hasMutated = false;
+
+  yield gWalker.insertBefore(nodeA, longlist);
+  ok(!hasMutated, "hasn't mutated after inserting with null sibling again");
+
+  observer.disconnect();
+  runNextTest();
 });
 
 addTest(function cleanup() {
@@ -77,7 +104,6 @@ addTest(function cleanup() {
   runNextTest();
 });
 
-
   </script>
 </head>
 <body>