Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier. r=pbrosset, a=lizzard
authorJulian Descottes <jdescottes@mozilla.com>
Fri, 04 Mar 2016 18:15:32 +0100
changeset 319316 3792c0bc4e320496b4c0a39fb9761f16c533f3e0
parent 319315 85d1415184ba33abc82f570c087f3bc20603cefb
child 319317 f487458614db3789468259589cdad858600ed2d3
push id1079
push userjlund@mozilla.com
push dateFri, 15 Apr 2016 21:02:33 +0000
treeherdermozilla-release@575fbf6786d5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset, lizzard
bugs1230325
milestone46.0
Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier. r=pbrosset, a=lizzard 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
@@ -556,27 +556,29 @@ MarkupView.prototype = {
   _onKeyDown: function(aEvent) {
     let handled = true;
 
     // Ignore keystrokes that originated in editors.
     if (this._isInputOrTextarea(aEvent.target)) {
       return;
     }
 
-    switch(aEvent.keyCode) {
+    // Ignore keystrokes with modifiers to allow native shortcuts (such as save:
+    // accel + S) to bubble up.
+    if (aEvent.metaKey || aEvent.ctrlKey || aEvent.altKey || aEvent.shiftKey) {
+      return;
+    }
+
+    switch (aEvent.keyCode) {
       case Ci.nsIDOMKeyEvent.DOM_VK_H:
-        if (aEvent.metaKey || aEvent.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
@@ -89,16 +89,17 @@ skip-if = e10s # Bug 1040751 - CodeMirro
 [browser_markup_html_edit_03.js]
 [browser_markup_image_tooltip.js]
 [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_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]
 [browser_markup_remove_xul_attributes.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
@@ -214,16 +214,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);