Bug 1224304 - Handle canceling the element picker better r=gl

This changes the behavior of the element picker so that when it is
cancelled the previously selected DOM node is re-scrolled into view.
Additionally the existing behavior of the keyboard shortcuts for the
element picker was broken when the devtools toolbox was docked. The main
content area was not being focused, so the keyboard shortcuts for the
element picker were not being used. When the toolbox is detached, the
focus event is still not fired, as it's not desirable to have the
content pop into view over the devtools.

Finally there is now an additional implementation of the Escape shortcut
when the devtools are focused. The console Escape shortcut is ignored
until the element picker has been disabled making disabling the element
picker consistent irrelevant of the context.

MozReview-Commit-ID: HxENmPBoTcD

--HG--
extra : rebase_source : 067b51d4c7324f2e59fc698dbd5bb01ad2b29205

devtools/client/framework/toolbox-highlighter-utils.js

@@ -93,13 +93,15 @@ exports.getHighlighterUtils = function (toolbox) {
 
   /**
    * Start/stop the element picker on the debuggee target.
+   * @param {Boolean} doFocus - Optionally focus the content area once the picker is
+   *                            activated.
    * @return A promise that resolves when done
    */
-  let togglePicker = exported.togglePicker = function () {
+  let togglePicker = exported.togglePicker = function (doFocus) {
     if (isPicking) {
-      return stopPicker();
+      return cancelPicker();
     } else {
-      return startPicker();
+      return startPicker(doFocus);
     }
   };
 
@@ -109,10 +111,12 @@ exports.getHighlighterUtils = function (toolbox) {
    * on the target page to highlight the hovered/picked element.
    * Depending on the server-side capabilities, this may fire events when nodes
    * are hovered.
+   * @param {Boolean} doFocus - Optionally focus the content area once the picker is
+   *                            activated.
    * @return A promise that resolves when the picker has started or immediately
    * if it is already started
    */
-  let startPicker = exported.startPicker = requireInspector(function* () {
+  let startPicker = exported.startPicker = requireInspector(function* (doFocus = false) {
     if (isPicking) {
       return;
     }
@@ -120,14 +124,14 @@ exports.getHighlighterUtils = function (toolbox) {
 
     toolbox.pickerButtonChecked = true;
     yield toolbox.selectTool("inspector");
-    toolbox.on("select", stopPicker);
+    toolbox.on("select", cancelPicker);
 
     if (isRemoteHighlightable()) {
       toolbox.walker.on("picker-node-hovered", onPickerNodeHovered);
       toolbox.walker.on("picker-node-picked", onPickerNodePicked);
       toolbox.walker.on("picker-node-canceled", onPickerNodeCanceled);
 
-      yield toolbox.highlighter.pick();
+      yield toolbox.highlighter.pick(doFocus);
       toolbox.emit("picker-started");
     } else {
       // If the target doesn't have the highlighter actor, we can use the
@@ -164,10 +168,18 @@ exports.getHighlighterUtils = function (toolbox) {
       yield toolbox.walker.cancelPick();
     }
 
-    toolbox.off("select", stopPicker);
+    toolbox.off("select", cancelPicker);
     toolbox.emit("picker-stopped");
   });
 
+  /**
+   * Stop the picker, but also emit an event that the picker was canceled.
+   */
+  let cancelPicker = exported.cancelPicker = Task.async(function* () {
+    yield stopPicker();
+    toolbox.emit("picker-canceled");
+  });
+
   /**
    * When a node is hovered by the mouse when the highlighter is in picker mode
    * @param {Object} data Information about the node being hovered
@@ -190,7 +202,7 @@ exports.getHighlighterUtils = function (toolbox) {
    * gets the focus.
    */
   function onPickerNodeCanceled() {
-    stopPicker();
+    cancelPicker();
     toolbox.win.focus();
   }
 

devtools/client/framework/toolbox.js

@@ -123,6 +123,10 @@ function Toolbox(target, selectedTool, hostType, hostOptions) {
   this._toggleMinimizeMode = this._toggleMinimizeMode.bind(this);
   this._onTabbarFocus = this._onTabbarFocus.bind(this);
   this._onTabbarArrowKeypress = this._onTabbarArrowKeypress.bind(this);
+  this._onPickerClick = this._onPickerClick.bind(this);
+  this._onPickerKeypress = this._onPickerKeypress.bind(this);
+  this._onPickerStarted = this._onPickerStarted.bind(this);
+  this._onPickerStopped = this._onPickerStopped.bind(this);
 
   this._target.on("close", this.destroy);
 
@@ -149,6 +153,9 @@ function Toolbox(target, selectedTool, hostType, hostOptions) {
 
   gDevTools.on("tool-registered", this._toolRegistered);
   gDevTools.on("tool-unregistered", this._toolUnregistered);
+
+  this.on("picker-started", this._onPickerStarted);
+  this.on("picker-stopped", this._onPickerStopped);
 }
 exports.Toolbox = Toolbox;
 
@@ -984,8 +991,39 @@ Toolbox.prototype = {
     let container = this.doc.querySelector("#toolbox-picker-container");
     container.appendChild(this._pickerButton);
 
-    this._togglePicker = this.highlighterUtils.togglePicker.bind(this.highlighterUtils);
-    this._pickerButton.addEventListener("click", this._togglePicker, false);
+    this._pickerButton.addEventListener("click", this._onPickerClick, false);
+  },
+
+  /**
+   * Toggle the picker, but also decide whether or not the highlighter should
+   * focus the window. This is only desirable when the toolbox is mounted to the
+   * window. When devtools is free floating, then the target window should not
+   * pop in front of the viewer when the picker is clicked.
+   */
+  _onPickerClick: function () {
+    let focus = this.hostType === Toolbox.HostType.BOTTOM ||
+                this.hostType === Toolbox.HostType.SIDE;
+    this.highlighterUtils.togglePicker(focus);
+  },
+
+  /**
+   * If the picker is activated, then allow the Escape key to deactivate the
+   * functionality instead of the default behavior of toggling the console.
+   */
+  _onPickerKeypress: function (event) {
+    if (event.keyCode === KeyCodes.DOM_VK_ESCAPE) {
+      this.highlighterUtils.cancelPicker();
+      // Stop the console from toggling.
+      event.stopImmediatePropagation();
+    }
+  },
+
+  _onPickerStarted: function () {
+    this.doc.addEventListener("keypress", this._onPickerKeypress, true);
+  },
+
+  _onPickerStopped: function () {
+    this.doc.removeEventListener("keypress", this._onPickerKeypress, true);
   },
 
   /**

devtools/client/inspector/markup/markup.js

@@ -108,6 +108,7 @@ function MarkupView(inspector, frame, controllerWindow) {
   this._onFocus = this._onFocus.bind(this);
   this._onMouseMove = this._onMouseMove.bind(this);
   this._onMouseOut = this._onMouseOut.bind(this);
+  this._onToolboxPickerCanceled = this._onToolboxPickerCanceled.bind(this);
   this._onToolboxPickerHover = this._onToolboxPickerHover.bind(this);
   this._onCollapseAttributesPrefChange =
     this._onCollapseAttributesPrefChange.bind(this);
@@ -127,6 +128,7 @@ function MarkupView(inspector, frame, controllerWindow) {
   this.walker.on("mutations", this._mutationObserver);
   this.walker.on("display-change", this._onDisplayChange);
   this.inspector.selection.on("new-node-front", this._onNewSelection);
+  this.toolbox.on("picker-canceled", this._onToolboxPickerCanceled);
   this.toolbox.on("picker-node-hovered", this._onToolboxPickerHover);
 
   this._onNewSelection();
@@ -189,6 +191,16 @@ MarkupView.prototype = {
     }, e => console.error(e));
   },
 
+  /**
+   * If the element picker gets canceled, make sure and re-center the view on the
+   * current selected element.
+   */
+  _onToolboxPickerCanceled: function () {
+    if (this._selectedContainer) {
+      scrollIntoViewIfNeeded(this._selectedContainer.editor.elt);
+    }
+  },
+
   isDragging: false,
 
   _onMouseMove: function (event) {
@@ -375,6 +387,9 @@ MarkupView.prototype = {
 
     this.getContainer(nodeFront).hovered = true;
     this._hoveredNode = nodeFront;
+    // Emit an event that the container view is actually hovered now, as this function
+    // can be called by an asynchronous caller.
+    this.emit("showcontainerhovered");
   },
 
   _onMouseOut: function (event) {

devtools/client/inspector/test/browser.ini

@@ -26,6 +26,7 @@ support-files =
   doc_inspector_infobar_02.html
   doc_inspector_infobar_03.html
   doc_inspector_infobar_textnode.html
+  doc_inspector_long-divs.html
   doc_inspector_menu.html
   doc_inspector_outerhtml.html
   doc_inspector_remove-iframe-during-load.html
@@ -67,6 +68,7 @@ skip-if = os == "mac" # Full keyboard navigation on OSX only works if Full Keybo
 [browser_inspector_highlighter-03.js]
 [browser_inspector_highlighter-04.js]
 [browser_inspector_highlighter-by-type.js]
+[browser_inspector_highlighter-cancel.js]
 [browser_inspector_highlighter-comments.js]
 [browser_inspector_highlighter-cssgrid_01.js]
 [browser_inspector_highlighter-csstransform_01.js]

devtools/client/inspector/test/browser_inspector_highlighter-cancel.js

@@ -0,0 +1,52 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// Test that canceling the element picker zooms back on the focused element. Bug 1224304.
+
+const TEST_URL = URL_ROOT + "doc_inspector_long-divs.html";
+
+add_task(function* () {
+  let {inspector, toolbox, testActor} = yield openInspectorForURL(TEST_URL);
+
+  yield selectAndHighlightNode("#focus-here", inspector);
+  ok((yield testActor.assertHighlightedNode("#focus-here")),
+     "The highlighter focuses on div#focus-here");
+  ok(isSelectedMarkupNodeInView(),
+     "The currently selected node is on the screen.");
+
+  // Start the picker but skip focusing manually focusing on the target, let the element
+  // picker do the focusing.
+  yield startPicker(toolbox, true);
+  yield moveMouseOver("#zoom-here");
+  ok(!isSelectedMarkupNodeInView(),
+     "The currently selected node is off the screen.");
+
+  yield cancelPickerByShortcut();
+  ok(isSelectedMarkupNodeInView(),
+     "The currently selected node is focused back on the screen.");
+
+  function cancelPickerByShortcut() {
+    info("Key pressed. Waiting for picker to be canceled.");
+    testActor.synthesizeKey({key: "VK_ESCAPE", options: {}});
+    return inspector.toolbox.once("picker-canceled");
+  }
+
+  function moveMouseOver(selector) {
+    info(`Waiting for element ${selector} to be hovered in the markup view`);
+    testActor.synthesizeMouse({
+      options: {type: "mousemove"},
+      center: true,
+      selector: selector
+    });
+    return inspector.markup.once("showcontainerhovered");
+  }
+
+  function isSelectedMarkupNodeInView() {
+    const selectedNodeContainer = inspector.markup._selectedContainer.elt;
+    const bounds = selectedNodeContainer.getBoundingClientRect();
+    return bounds.top > 0 && bounds.bottom > 0;
+  }
+});

devtools/client/inspector/test/doc_inspector_long-divs.html

@@ -0,0 +1,104 @@
+<!doctype html>
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+  <title>Inspector Long Div Listing</title>
+  <style>
+    div {
+      background-color: #0002;
+      padding-left: 1em;
+    }
+  </style>
+</head>
+<body>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div id="focus-here">focus here</div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div>
+    <div>
+      <div>
+        <div>
+          <div id="zoom-here">zoom-here</div>
+        </div>
+      </div>
+    </div>
+  </div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+
+</body>
+</html>

devtools/client/inspector/test/head.js

@@ -69,15 +69,19 @@ var navigateTo = function (toolbox, url) {
 /**
  * Start the element picker and focus the content window.
  * @param {Toolbox} toolbox
+ * @param {Boolean} skipFocus - Allow tests to bypass the focus event.
  */
-var startPicker = Task.async(function* (toolbox) {
+var startPicker = Task.async(function* (toolbox, skipFocus) {
   info("Start the element picker");
+  toolbox.win.focus();
   yield toolbox.highlighterUtils.startPicker();
-  // Make sure the content window is focused since the picker does not focus
-  // the content window by default.
-  yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
-    content.focus();
-  });
+  if (!skipFocus) {
+    // By default make sure the content window is focused since the picker may not focus
+    // the content window by default.
+    yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
+      content.focus();
+    });
+  }
 });
 
 /**

devtools/server/actors/highlighters.js

@@ -353,6 +353,16 @@ var HighlighterActor = exports.HighlighterActor = protocol.ActorClassWithSpec(hi
     return null;
   },
 
+  /**
+   * This pick method also focuses the highlighter's target window.
+   */
+  pickAndFocus: function() {
+    // Go ahead and pass on the results to help future-proof this method.
+    let pickResults = this.pick();
+    this._highlighterEnv.window.focus();
+    return pickResults;
+  },
+
   _findAndAttachElement: function (event) {
     // originalTarget allows access to the "real" element before any retargeting
     // is applied, such as in the case of XBL anonymous elements.  See also

devtools/shared/fronts/highlighters.js

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-const { FrontClassWithSpec } = require("devtools/shared/protocol");
+const { FrontClassWithSpec, custom } = require("devtools/shared/protocol");
 const {
   customHighlighterSpec,
   highlighterSpec
@@ -15,7 +15,16 @@ const HighlighterFront = FrontClassWithSpec(highlighterSpec, {
     this.actorID = json.actor;
     // FF42+ HighlighterActors starts exposing custom form, with traits object
     this.traits = json.traits || {};
-  }
+  },
+
+  pick: custom(function (doFocus) {
+    if (doFocus && this.pickAndFocus) {
+      return this.pickAndFocus();
+    }
+    return this._pick();
+  }, {
+    impl: "_pick"
+  })
 });
 
 exports.HighlighterFront = HighlighterFront;

devtools/shared/specs/highlighters.js

@@ -28,6 +28,7 @@ const highlighterSpec = generateActorSpec({
       request: {}
     },
     pick: {},
+    pickAndFocus: {},
     cancelPick: {}
   }
 });