Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset
authorJulian Descottes <jdescottes@mozilla.com>
Fri, 04 Mar 2016 18:15:32 +0100
changeset 287019 5a8c334ac296c94faf804e12a05c3e5c6f555155
parent 287018 4b613cad02de7b3fcd6ce622e26637a0af9ee52c
child 287020 c3550a9e2c53dd75bd74c90e2d1b5d15f2ddec5e
push id18037
push userjdescottes@mozilla.com
push dateMon, 07 Mar 2016 13:24:21 +0000
treeherderfx-team@5a8c334ac296 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1230325
milestone47.0a1
Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset Bail out from the markup view onKeyDown handler if any modifier is currently true. All shortcuts specified in this handler are intended to be used without modifier, so for now this approach is fine. Added a test checking the use case mentioned in Bug 1230325, with the S shortcut. In order to write the test, had to create an additional method on the test actor to be able to wait for events in the window of the content process. MozReview-Commit-ID: 67icou0HkfA
devtools/client/inspector/markup/markup.js
devtools/client/inspector/markup/test/browser.ini
devtools/client/inspector/markup/test/browser_markup_keybindings_scrolltonode.js
devtools/client/shared/test/test-actor.js
--- a/devtools/client/inspector/markup/markup.js
+++ b/devtools/client/inspector/markup/markup.js
@@ -600,27 +600,29 @@ MarkupView.prototype = {
     let handled = true;
     let previousNode, nextNode;
 
     // Ignore keystrokes that originated in editors.
     if (this._isInputOrTextarea(event.target)) {
       return;
     }
 
+    // Ignore keystrokes with modifiers to allow native shortcuts (such as save:
+    // accel + S) to bubble up.
+    if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) {
+      return;
+    }
+
     switch (event.keyCode) {
       case Ci.nsIDOMKeyEvent.DOM_VK_H:
-        if (event.metaKey || event.shiftKey) {
-          handled = false;
+        let node = this._selectedContainer.node;
+        if (node.hidden) {
+          this.walker.unhideNode(node);
         } else {
-          let node = this._selectedContainer.node;
-          if (node.hidden) {
-            this.walker.unhideNode(node);
-          } else {
-            this.walker.hideNode(node);
-          }
+          this.walker.hideNode(node);
         }
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
         this.deleteNodeOrAttribute();
         break;
       case Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE:
         this.deleteNodeOrAttribute(true);
         break;
--- a/devtools/client/inspector/markup/test/browser.ini
+++ b/devtools/client/inspector/markup/test/browser.ini
@@ -83,16 +83,17 @@ skip-if = e10s && os == 'win'
 [browser_markup_image_tooltip.js]
 skip-if = (e10s && os == 'mac') # bug 1252345
 [browser_markup_image_tooltip_mutations.js]
 [browser_markup_keybindings_01.js]
 [browser_markup_keybindings_02.js]
 [browser_markup_keybindings_03.js]
 [browser_markup_keybindings_04.js]
 [browser_markup_keybindings_delete_attributes.js]
+[browser_markup_keybindings_scrolltonode.js]
 [browser_markup_mutation_01.js]
 [browser_markup_mutation_02.js]
 [browser_markup_node_names.js]
 [browser_markup_navigation.js]
 [browser_markup_node_not_displayed_01.js]
 [browser_markup_node_not_displayed_02.js]
 [browser_markup_pagesize_01.js]
 [browser_markup_pagesize_02.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/markup/test/browser_markup_keybindings_scrolltonode.js
@@ -0,0 +1,87 @@
+/* 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";
+
+// Test the keyboard shortcut "S" used to scroll to the selected node.
+
+const HTML =
+  `<div style="width: 300px; height: 3000px; position:relative;">
+    <div id="scroll-top"
+      style="height: 50px; top: 0; position:absolute;">
+      TOP</div>
+    <div id="scroll-bottom"
+      style="height: 50px; bottom: 0; position:absolute;">
+      BOTTOM</div>
+  </div>`;
+const TEST_URL = "data:text/html;charset=utf-8," + encodeURIComponent(HTML);
+
+add_task(function*() {
+  let { inspector, testActor } = yield openInspectorForURL(TEST_URL);
+
+  info("Make sure the markup frame has the focus");
+  inspector.markup._frame.focus();
+
+  info("Before test starts, #scroll-top is visible, #scroll-bottom is hidden");
+  yield checkElementIsInViewport("#scroll-top", true, testActor);
+  yield checkElementIsInViewport("#scroll-bottom", false, testActor);
+
+  info("Select the #scroll-bottom node");
+  yield selectNode("#scroll-bottom", inspector);
+  info("Press S to scroll to the bottom node");
+  let waitForScroll = testActor.waitForEventOnNode("scroll");
+  yield EventUtils.synthesizeKey("S", {}, inspector.panelWin);
+  yield waitForScroll;
+  ok(true, "Scroll event received");
+
+  info("#scroll-top should be scrolled out, #scroll-bottom should be visible");
+  yield checkElementIsInViewport("#scroll-top", false, testActor);
+  yield checkElementIsInViewport("#scroll-bottom", true, testActor);
+
+  info("Select the #scroll-top node");
+  yield selectNode("#scroll-top", inspector);
+  info("Press S to scroll to the top node");
+  waitForScroll = testActor.waitForEventOnNode("scroll");
+  yield EventUtils.synthesizeKey("S", {}, inspector.panelWin);
+  yield waitForScroll;
+  ok(true, "Scroll event received");
+
+  info("#scroll-top should be visible, #scroll-bottom should be scrolled out");
+  yield checkElementIsInViewport("#scroll-top", true, testActor);
+  yield checkElementIsInViewport("#scroll-bottom", false, testActor);
+
+  info("Select #scroll-bottom node");
+  yield selectNode("#scroll-bottom", inspector);
+  info("Press shift + S, nothing should happen due to the modifier");
+  yield EventUtils.synthesizeKey("S", {shiftKey: true}, inspector.panelWin);
+
+  info("Same state, #scroll-top is visible, #scroll-bottom is scrolled out");
+  yield checkElementIsInViewport("#scroll-top", true, testActor);
+  yield checkElementIsInViewport("#scroll-bottom", false, testActor);
+});
+
+/**
+ * Verify that the element matching the provided selector is either in or out
+ * of the viewport, depending on the provided "expected" argument.
+ * Returns a promise that will resolve when the test has been performed.
+ *
+ * @param {String} selector
+ *        css selector for the element to test
+ * @param {Boolean} expected
+ *        true if the element is expected to be in the viewport, false otherwise
+ * @param {TestActor} testActor
+ *        current test actor
+ * @return {Promise} promise
+ */
+function* checkElementIsInViewport(selector, expected, testActor) {
+  let isInViewport = yield testActor.eval(`
+    let node = content.document.querySelector("${selector}");
+    let rect = node.getBoundingClientRect();
+    rect.bottom >= 0 && rect.right >= 0 &&
+      rect.top <= content.innerHeight && rect.left <= content.innerWidth;
+  `);
+
+  is(isInViewport, expected,
+    selector + " in the viewport: expected to be " + expected);
+}
--- a/devtools/client/shared/test/test-actor.js
+++ b/devtools/client/shared/test/test-actor.js
@@ -222,16 +222,38 @@ var TestActor = exports.TestActor = prot
     request: {
       event: Arg(0, "string"),
       actorID: Arg(1, "string")
     },
     response: {}
   }),
 
   /**
+   * Wait for a specific event on a node matching the provided selector.
+   * @param {String} eventName The name of the event to listen to
+   * @param {String} selector Optional:  css selector of the node which should
+   *        trigger the event. If ommitted, target will be the content window
+   */
+  waitForEventOnNode: protocol.method(function (eventName, selector) {
+    return new Promise(resolve => {
+      let node = selector ? this._querySelector(selector) : this.content;
+      node.addEventListener(eventName, function onEvent() {
+        node.removeEventListener(eventName, onEvent);
+        resolve();
+      });
+    });
+  }, {
+    request: {
+      eventName: Arg(0, "string"),
+      selector: Arg(1, "nullable:string")
+    },
+    response: {}
+  }),
+
+  /**
    * Change the zoom level of the page.
    * Optionally subscribe to the box-model highlighter's update event and waiting
    * for it to refresh before responding.
    * @param {Number} level The new zoom level
    * @param {String} actorID Optional. The highlighter actor ID
    */
   changeZoomLevel: protocol.method(function (level, actorID) {
     dumpn("Zooming page to " + level);