Bug 1224304 - Handle canceling the element picker better r=gl
authorGreg Tatum <tatum.creative@gmail.com>
Thu, 06 Oct 2016 13:40:53 -0500
changeset 318581 1d9e3c771a6cdfc2bb831a4bc1049de0457b3798
parent 318580 f84e984210c9366cd1de7bfccd55ef80e0fcd007
child 318582 106fc7412c9f743be5e0a4b2f87a5a196f6b034e
push id30844
push userphilringnalda@gmail.com
push dateThu, 20 Oct 2016 01:26:07 +0000
treeherdermozilla-central@6e1a56ec3487 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1224304
milestone52.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 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
devtools/client/framework/toolbox-highlighter-utils.js
devtools/client/framework/toolbox.js
devtools/client/inspector/markup/markup.js
devtools/client/inspector/test/browser.ini
devtools/client/inspector/test/browser_inspector_highlighter-cancel.js
devtools/client/inspector/test/doc_inspector_long-divs.html
devtools/client/inspector/test/head.js
devtools/server/actors/highlighters.js
devtools/shared/fronts/highlighters.js
devtools/shared/specs/highlighters.js
--- a/devtools/client/framework/toolbox-highlighter-utils.js
+++ b/devtools/client/framework/toolbox-highlighter-utils.js
@@ -88,51 +88,55 @@ exports.getHighlighterUtils = function (
         isInspectorInitialized = true;
       }
       return yield generator.apply(null, args);
     });
   };
 
   /**
    * 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);
     }
   };
 
   /**
    * Start the element picker on the debuggee target.
    * This will request the inspector actor to start listening for mouse events
    * 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;
     }
     isPicking = true;
 
     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
       // walker's pick method instead, knowing that it only responds when a node
       // is picked (instead of emitting events)
       toolbox.emit("picker-started");
       let node = yield toolbox.walker.pick();
       onPickerNodePicked({node: node});
@@ -159,21 +163,29 @@ exports.getHighlighterUtils = function (
       toolbox.walker.off("picker-node-picked", onPickerNodePicked);
       toolbox.walker.off("picker-node-canceled", onPickerNodeCanceled);
     } else {
       // If the target doesn't have the highlighter actor, use the walker's
       // cancelPick method instead
       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
    */
   function onPickerNodeHovered(data) {
     toolbox.emit("picker-node-hovered", data.node);
   }
 
   /**
@@ -185,17 +197,17 @@ exports.getHighlighterUtils = function (
     stopPicker();
   }
 
   /**
    * When the picker is canceled, stop the picker, and make sure the toolbox
    * gets the focus.
    */
   function onPickerNodeCanceled() {
-    stopPicker();
+    cancelPicker();
     toolbox.win.focus();
   }
 
   /**
    * Show the box model highlighter on a node in the content page.
    * The node needs to be a NodeFront, as defined by the inspector actor
    * @see devtools/server/actors/inspector.js
    * @param {NodeFront} nodeFront The node to highlight
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -118,16 +118,20 @@ function Toolbox(target, selectedTool, h
   this._onBottomHostMinimized = this._onBottomHostMinimized.bind(this);
   this._onBottomHostMaximized = this._onBottomHostMaximized.bind(this);
   this._onToolSelectWhileMinimized = this._onToolSelectWhileMinimized.bind(this);
   this._onPerformanceFrontEvent = this._onPerformanceFrontEvent.bind(this);
   this._onBottomHostWillChange = this._onBottomHostWillChange.bind(this);
   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);
 
   if (!hostType) {
     hostType = Services.prefs.getCharPref(this._prefs.LAST_HOST);
   }
   if (!selectedTool) {
     selectedTool = Services.prefs.getCharPref(this._prefs.LAST_TOOL);
@@ -144,16 +148,19 @@ function Toolbox(target, selectedTool, h
 
   this.on("host-changed", this._refreshHostTitle);
   this.on("select", this._refreshHostTitle);
 
   this.on("ready", this._showDevEditionPromo);
 
   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;
 
 /**
  * The toolbox can be 'hosted' either embedded in a browser window
  * or in a separate window.
  */
 Toolbox.HostType = {
@@ -979,18 +986,49 @@ Toolbox.prototype = {
     this._pickerButton.id = "command-button-pick";
     this._pickerButton.className =
       "command-button command-button-invertable devtools-button";
     this._pickerButton.setAttribute("title", L10N.getStr("pickButton.tooltip"));
 
     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);
   },
 
   /**
    * Apply the current cache setting from devtools.cache.disabled to this
    * toolbox's tab.
    */
   _applyCacheSettings: function () {
     let pref = "devtools.cache.disabled";
--- a/devtools/client/inspector/markup/markup.js
+++ b/devtools/client/inspector/markup/markup.js
@@ -103,16 +103,17 @@ function MarkupView(inspector, frame, co
   this._onDisplayChange = this._onDisplayChange.bind(this);
   this._onMouseClick = this._onMouseClick.bind(this);
   this._onMouseUp = this._onMouseUp.bind(this);
   this._onNewSelection = this._onNewSelection.bind(this);
   this._onCopy = this._onCopy.bind(this);
   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);
   this._isImagePreviewTarget = this._isImagePreviewTarget.bind(this);
   this._onBlur = this._onBlur.bind(this);
 
   EventEmitter.decorate(this);
 
@@ -122,16 +123,17 @@ function MarkupView(inspector, frame, co
   this._elt.addEventListener("mouseout", this._onMouseOut, false);
   this._elt.addEventListener("blur", this._onBlur, true);
   this.win.addEventListener("mouseup", this._onMouseUp);
   this.win.addEventListener("copy", this._onCopy);
   this._frame.addEventListener("focus", this._onFocus, false);
   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();
   this._initTooltips();
 
   this._prefObserver = new PrefObserver("devtools.markup");
   this._prefObserver.on(ATTR_COLLAPSE_ENABLED_PREF,
                         this._onCollapseAttributesPrefChange);
@@ -184,16 +186,26 @@ MarkupView.prototype = {
   },
 
   _onToolboxPickerHover: function (event, nodeFront) {
     this.showNode(nodeFront).then(() => {
       this._showContainerAsHovered(nodeFront);
     }, 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) {
     let target = event.target;
 
     // Auto-scroll if we're dragging.
     if (this.isDragging) {
       event.preventDefault();
@@ -370,16 +382,19 @@ MarkupView.prototype = {
     }
 
     if (this._hoveredNode) {
       this.getContainer(this._hoveredNode).hovered = false;
     }
 
     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) {
     // Emulate mouseleave by skipping any relatedTarget inside the markup-view.
     if (this._elt.contains(event.relatedTarget)) {
       return;
     }
 
--- a/devtools/client/inspector/test/browser.ini
+++ b/devtools/client/inspector/test/browser.ini
@@ -21,16 +21,17 @@ support-files =
   doc_inspector_highlighter.html
   doc_inspector_highlighter_rect.html
   doc_inspector_highlighter_rect_iframe.html
   doc_inspector_highlighter_xbl.xul
   doc_inspector_infobar_01.html
   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
   doc_inspector_search.html
   doc_inspector_search-reserved.html
   doc_inspector_search-suggestions.html
   doc_inspector_search-svg.html
   doc_inspector_select-last-selected-01.html
@@ -62,16 +63,17 @@ skip-if = os == "mac" # Full keyboard na
 [browser_inspector_destroy-before-ready.js]
 [browser_inspector_expand-collapse.js]
 [browser_inspector_gcli-inspect-command.js]
 [browser_inspector_highlighter-01.js]
 [browser_inspector_highlighter-02.js]
 [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]
 [browser_inspector_highlighter-csstransform_02.js]
 [browser_inspector_highlighter-embed.js]
 [browser_inspector_highlighter-eyedropper-clipboard.js]
 subsuite = clipboard
 [browser_inspector_highlighter-eyedropper-csp.js]
new file mode 100644
--- /dev/null
+++ b/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;
+  }
+});
new file mode 100644
--- /dev/null
+++ b/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>
--- a/devtools/client/inspector/test/head.js
+++ b/devtools/client/inspector/test/head.js
@@ -64,25 +64,29 @@ registerCleanupFunction(function* () {
 var navigateTo = function (toolbox, url) {
   let activeTab = toolbox.target.activeTab;
   return activeTab.navigateTo(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();
+    });
+  }
 });
 
 /**
  * Highlight a node and set the inspector's current selection to the node or
  * the first match of the given css selector.
  * @param {String|NodeFront} selector
  * @param {InspectorPanel} inspector
  *        The instance of InspectorPanel currently loaded in the toolbox
--- a/devtools/server/actors/highlighters.js
+++ b/devtools/server/actors/highlighters.js
@@ -348,16 +348,26 @@ var HighlighterActor = exports.Highlight
       events.emit(this._walker, "picker-node-hovered", this._currentNode);
     };
 
     this._startPickerListeners();
 
     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
     // https://developer.mozilla.org/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Event_Flow_and_Targeting
     let node = event.originalTarget || event.target;
     return this._walker.attachElement(node);
   },
 
--- a/devtools/shared/fronts/highlighters.js
+++ b/devtools/shared/fronts/highlighters.js
@@ -1,25 +1,34 @@
 /* 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";
 
-const { FrontClassWithSpec } = require("devtools/shared/protocol");
+const { FrontClassWithSpec, custom } = require("devtools/shared/protocol");
 const {
   customHighlighterSpec,
   highlighterSpec
 } = require("devtools/shared/specs/highlighters");
 
 const HighlighterFront = FrontClassWithSpec(highlighterSpec, {
   // Update the object given a form representation off the wire.
   form: function (json) {
     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;
 
 const CustomHighlighterFront = FrontClassWithSpec(customHighlighterSpec, {});
 
 exports.CustomHighlighterFront = CustomHighlighterFront;
--- a/devtools/shared/specs/highlighters.js
+++ b/devtools/shared/specs/highlighters.js
@@ -23,16 +23,17 @@ const highlighterSpec = generateActorSpe
         showOnly: Option(1),
         onlyRegionArea: Option(1)
       }
     },
     hideBoxModel: {
       request: {}
     },
     pick: {},
+    pickAndFocus: {},
     cancelPick: {}
   }
 });
 
 exports.highlighterSpec = highlighterSpec;
 
 const customHighlighterSpec = generateActorSpec({
   typeName: "customhighlighter",