Bug 1203303 - When attribute is focused DEL removes attribute not node; r=bgrins
authorPatrick Brosset <pbrosset@mozilla.com>
Thu, 22 Oct 2015 15:49:14 +0200
changeset 269156 180eb2f441d6510925775e93296c2708b40fd042
parent 269155 0dae594de5aca88fd1fb2788c79a8e8f3755b88c
child 269157 688d6e1caacaf05a5d52dc2a5b01ed32f30f72b3
push id29573
push userkwierso@gmail.com
push dateFri, 23 Oct 2015 22:26:14 +0000
treeherdermozilla-central@a829347b8691 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbgrins
bugs1203303
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1203303 - When attribute is focused DEL removes attribute not node; r=bgrins
devtools/client/markupview/markup-view.js
devtools/client/markupview/test/browser.ini
devtools/client/markupview/test/browser_markupview_keybindings_delete_attributes.js
devtools/client/markupview/test/browser_markupview_tag_edit_04.js
--- a/devtools/client/markupview/markup-view.js
+++ b/devtools/client/markupview/markup-view.js
@@ -572,20 +572,20 @@ MarkupView.prototype = {
           if (node.hidden) {
             this.walker.unhideNode(node);
           } else {
             this.walker.hideNode(node);
           }
         }
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
-        this.deleteNode(this._selectedContainer.node);
+        this.deleteNodeOrAttribute();
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE:
-        this.deleteNode(this._selectedContainer.node, true);
+        this.deleteNodeOrAttribute(true);
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_HOME:
         let rootContainer = this.getContainer(this._rootNode);
         this.navigate(rootContainer.children.firstChild.container);
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_LEFT:
         if (this._selectedContainer.expanded) {
           this.collapseNode(this._selectedContainer.node);
@@ -673,22 +673,41 @@ MarkupView.prototype = {
    * Check if a node is an input or textarea
    */
   _isInputOrTextarea : function (element) {
     let name = element.tagName.toLowerCase();
     return name === "input" || name === "textarea";
   },
 
   /**
+   * If there's an attribute on the current node that's currently focused, then
+   * delete this attribute, otherwise delete the node itself.
+   * @param {boolean} moveBackward If set to true and if we're deleting the
+   * node, focus the previous sibling after deletion, otherwise the next one.
+   */
+  deleteNodeOrAttribute: function(moveBackward) {
+    let focusedAttribute = this.doc.activeElement
+                           ? this.doc.activeElement.closest(".attreditor")
+                           : null;
+    if (focusedAttribute) {
+      // The focused attribute might not be in the current selected container.
+      let container = focusedAttribute.closest("li.child").container;
+      container.removeAttribute(focusedAttribute.dataset.attr);
+    } else {
+      this.deleteNode(this._selectedContainer.node, moveBackward);
+    }
+  },
+
+  /**
    * Delete a node from the DOM.
    * This is an undoable action.
    *
    * @param {NodeFront} aNode The node to remove.
    * @param {boolean} moveBackward If set to true, focus the previous sibling,
-   *  otherwise the next one.
+   * otherwise the next one.
    */
   deleteNode: function(aNode, moveBackward) {
     if (aNode.isDocumentElement ||
         aNode.nodeType == Ci.nsIDOMNode.DOCUMENT_TYPE_NODE ||
         aNode.isAnonymous) {
       return;
     }
 
--- a/devtools/client/markupview/test/browser.ini
+++ b/devtools/client/markupview/test/browser.ini
@@ -88,16 +88,17 @@ skip-if = e10s # Bug 1040751 - CodeMirro
 [browser_markupview_html_edit_02.js]
 [browser_markupview_html_edit_03.js]
 [browser_markupview_image_tooltip.js]
 [browser_markupview_image_tooltip_mutations.js]
 [browser_markupview_keybindings_01.js]
 [browser_markupview_keybindings_02.js]
 [browser_markupview_keybindings_03.js]
 [browser_markupview_keybindings_04.js]
+[browser_markupview_keybindings_delete_attributes.js]
 [browser_markupview_mutation_01.js]
 [browser_markupview_mutation_02.js]
 [browser_markupview_navigation.js]
 [browser_markupview_node_not_displayed_01.js]
 [browser_markupview_node_not_displayed_02.js]
 [browser_markupview_pagesize_01.js]
 [browser_markupview_pagesize_02.js]
 [browser_markupview_remove_xul_attributes.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/markupview/test/browser_markupview_keybindings_delete_attributes.js
@@ -0,0 +1,63 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that attributes can be deleted from the markup-view with the delete key
+// when they are focused.
+
+const HTML = `<div id="id" class="class" data-id="id"></div>`;
+const TEST_URL = "data:text/html;charset=utf-8," + encodeURIComponent(HTML);
+
+// List of all the test cases. Each item is an object with the following props:
+// - selector: the css selector of the node that should be selected
+// - attribute: the name of the attribute that should be focused. Do not
+//   specify an attribute that would make it impossible to find the node using
+//   selector.
+// Note that after each test case, undo is called.
+const TEST_DATA = [{
+  selector: "#id",
+  attribute: "class"
+}, {
+  selector: "#id",
+  attribute: "data-id"
+}];
+
+add_task(function*() {
+  let {inspector} = yield addTab(TEST_URL).then(openInspector);
+  let {walker} = inspector;
+
+  for (let {selector, attribute} of TEST_DATA) {
+    info("Get the container for node " + selector);
+    let {editor} = yield getContainerForSelector(selector, inspector);
+
+    info("Focus attribute " + attribute);
+    let attr = editor.attrElements.get(attribute).querySelector(".editable");
+    attr.focus();
+
+    info("Delete the attribute by pressing delete");
+    let mutated = inspector.once("markupmutation");
+    EventUtils.sendKey("delete", inspector.panelWin);
+    yield mutated;
+
+    info("Check that the node is still here");
+    let node = yield walker.querySelector(walker.rootNode, selector);
+    ok(node, "The node hasn't been deleted");
+
+    info("Check that the attribute has been deleted");
+    node = yield walker.querySelector(walker.rootNode,
+                                      selector + "[" + attribute + "]");
+    ok(!node, "The attribute does not exist anymore in the DOM");
+    ok(!editor.attrElements.get(attribute),
+       "The attribute has been removed from the container");
+
+    info("Undo the change");
+    yield undoChange(inspector);
+    node = yield walker.querySelector(walker.rootNode,
+                                      selector + "[" + attribute + "]");
+    ok(node, "The attribute is back in the DOM");
+    ok(editor.attrElements.get(attribute),
+       "The attribute is back on the container");
+  }
+});
--- a/devtools/client/markupview/test/browser_markupview_tag_edit_04.js
+++ b/devtools/client/markupview/test/browser_markupview_tag_edit_04.js
@@ -3,57 +3,103 @@
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 // 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='parent'><div id='first'></div><div id='second'></div><div id='third'></div></div>";
-
-function* checkDeleteAndSelection(inspector, key, nodeSelector, focusedNodeSelector) {
-  yield selectNode(nodeSelector, inspector);
-  yield clickContainer(nodeSelector, inspector);
-
-  info(`Deleting the element "${nodeSelector}" using the ${key} key`);
-  let mutated = inspector.once("markupmutation");
-  EventUtils.sendKey(key, inspector.panelWin);
+const HTML = `<div id="parent">
+                <div id="first"></div>
+                <div id="second"></div>
+                <div id="third"></div>
+              </div>`;
+const TEST_URL = "data:text/html;charset=utf-8," + encodeURIComponent(HTML);
 
-  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(nodeSelector), "The test node does not exist");
-
-  yield undoChange(inspector);
-  ok(content.document.querySelector(nodeSelector), "The test node is back!");
-}
+// List of all the test cases. Each item is an object with the following props:
+// - selector: the css selector of the node that should be selected
+// - key: the key to press to delete the node (delete or back_space)
+// - focusedSelector: the css selector of the node we expect to be selected as
+//   a result of the deletion
+// - setup: an optional function that will be run before selecting and deleting
+//   the node
+// Note that after each test case, undo is called.
+const TEST_DATA = [{
+  selector: "#first",
+  key: "delete",
+  focusedSelector: "#second"
+}, {
+  selector: "#second",
+  key: "delete",
+  focusedSelector: "#third"
+}, {
+  selector: "#third",
+  key: "delete",
+  focusedSelector: "#second"
+}, {
+  selector: "#first",
+  key: "back_space",
+  focusedSelector: "#second"
+}, {
+  selector: "#second",
+  key: "back_space",
+  focusedSelector: "#first"
+}, {
+  selector: "#third",
+  key: "back_space",
+  focusedSelector: "#second"
+}, {
+  setup: function*(inspector) {
+    // Removing the siblings of #first in order to test with an only child.
+    let mutated = inspector.once("markupmutation");
+    for (let node of content.document.querySelectorAll("#second, #third")) {
+      node.remove();
+    }
+    yield mutated;
+  },
+  selector: "#first",
+  key: "delete",
+  focusedSelector: "#parent"
+}, {
+  selector: "#first",
+  key: "back_space",
+  focusedSelector: "#parent"
+}];
 
 add_task(function*() {
   let {inspector} = yield addTab(TEST_URL).then(openInspector);
 
-  info("Selecting the test node by clicking on it to make sure it receives focus");
+  for (let {setup, selector, key, focusedSelector} of TEST_DATA) {
+    if (setup) {
+      yield setup(inspector);
+    }
 
-  yield checkDeleteAndSelection(inspector, "delete", "#first", "#second");
-  yield checkDeleteAndSelection(inspector, "delete", "#second", "#third");
-  yield checkDeleteAndSelection(inspector, "delete", "#third", "#second");
+    yield checkDeleteAndSelection(inspector, key, selector, focusedSelector);
+  }
+});
 
-  yield checkDeleteAndSelection(inspector, "back_space", "#first", "#second");
-  yield checkDeleteAndSelection(inspector, "back_space", "#second", "#first");
-  yield checkDeleteAndSelection(inspector, "back_space", "#third", "#second");
+function* checkDeleteAndSelection(inspector, key, selector, focusedSelector) {
+  info("Test deleting node " + selector + " with " + key + ", " +
+       "expecting " + focusedSelector + " to be focused");
 
-  // Removing the siblings of #first.
+  info("Select node " + selector + " and make sure it is focused");
+  yield selectNode(selector, inspector);
+  yield clickContainer(selector, inspector);
+
+  info("Delete the node with: " + key);
   let mutated = inspector.once("markupmutation");
-  for (let node of content.document.querySelectorAll("#second, #third")) {
-    node.remove();
-  }
-  yield mutated;
-  // Testing with an only child.
-  info("testing with an only child");
-  yield checkDeleteAndSelection(inspector, "delete", "#first", "#parent");
-  yield checkDeleteAndSelection(inspector, "back_space", "#first", "#parent");
+  EventUtils.sendKey(key, inspector.panelWin);
+  yield Promise.all([mutated, inspector.once("inspector-updated")]);
+
+  let nodeFront = yield getNodeFront(focusedSelector, inspector);
+  is(inspector.selection.nodeFront, nodeFront,
+     focusedSelector + " is selected after deletion");
 
-  yield inspector.once("inspector-updated");
-});
+  info("Check that the node was really removed");
+  let node = yield getNodeFront(selector, inspector);
+  ok(!node, "The node can't be found in the page anymore");
+
+  info("Undo the deletion to restore the original markup");
+  yield undoChange(inspector);
+  node = yield getNodeFront(selector, inspector);
+  ok(node, "The node is back");
+}