Bug 1243131 - memory tool: select snapshot using ACCEL+{UP/DOWN};r=fitzgen
☠☠ backed out by 8ef5ae38d410 ☠ ☠
authorJulian Descottes <jdescottes@mozilla.com>
Mon, 08 Feb 2016 16:30:01 +0100
changeset 283931 c578a05bb9d6f398fccc5580df198a763af21020
parent 283930 78ae6726840abb929a4c7bae249c92d90e9ffaf1
child 283932 1e0af75b4c44da3271e51941561b582b42709a77
push id17570
push userntim.bugs@gmail.com
push dateThu, 11 Feb 2016 23:45:35 +0000
treeherderfx-team@c578a05bb9d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen
bugs1243131
milestone47.0a1
Bug 1243131 - memory tool: select snapshot using ACCEL+{UP/DOWN};r=fitzgen Adds a keydown listener on the memory panel window. Select previous/next snapshot when user presses UP/DOWN with the accelKey modifier (metaKey on OSX, ctrlKey on windows). Keydown events with modifiers are no longer listened to by the tree node elements. Updated tree node test. Added new mochitest to test the new keyboard navigation on the census view. )
devtools/client/memory/app.js
devtools/client/memory/test/browser/browser.ini
devtools/client/memory/test/browser/browser_memory_keyboard-snapshot-list.js
devtools/client/memory/test/browser/head.js
devtools/client/shared/components/test/mochitest/test_tree_06.html
devtools/client/shared/components/tree.js
--- a/devtools/client/memory/app.js
+++ b/devtools/client/memory/app.js
@@ -1,13 +1,14 @@
 /* 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/. */
 
 const { assert } = require("devtools/shared/DevToolsUtils");
+const { appinfo } = require("Services");
 const { DOM: dom, createClass, createFactory, PropTypes } = require("devtools/client/shared/vendor/react");
 const { connect } = require("devtools/client/shared/vendor/react-redux");
 const { breakdowns, diffingState, viewState } = require("./constants");
 const { toggleRecordingAllocationStacks } = require("./actions/allocations");
 const { setBreakdownAndRefresh } = require("./actions/breakdown");
 const { setDominatorTreeBreakdownAndRefresh } = require("./actions/dominatorTreeBreakdown");
 const {
   selectSnapshotForDiffingAndRefresh,
@@ -48,30 +49,62 @@ const MemoryApp = createClass({
   displayName: "MemoryApp",
 
   propTypes: appModel,
 
   getDefaultProps() {
     return {};
   },
 
+  componentDidMount() {
+    // Attach the keydown listener directly to the window. When an element that
+    // has the focus (such as a tree node) is removed from the DOM, the focus
+    // falls back to the body.
+    window.addEventListener("keydown", this.onKeyDown);
+  },
+
+  componentWillUnmount() {
+    window.removeEventListener("keydown", this.onKeyDown);
+  },
+
   childContextTypes: {
     front: PropTypes.any,
     heapWorker: PropTypes.any,
     toolbox: PropTypes.any,
   },
 
   getChildContext() {
     return {
       front: this.props.front,
       heapWorker: this.props.heapWorker,
       toolbox: this.props.toolbox,
     };
   },
 
+  onKeyDown(e) {
+    let { snapshots, dispatch, heapWorker } = this.props;
+    const selectedSnapshot = snapshots.find(s => s.selected);
+    const selectedIndex = snapshots.indexOf(selectedSnapshot);
+
+    let isOSX = appinfo.OS == "Darwin";
+    let isAccelKey = (isOSX && e.metaKey) || (!isOSX && e.ctrlKey);
+    // On ACCEL+UP, select previous snapshot.
+    if (isAccelKey && e.key === "ArrowUp") {
+      let previousIndex = Math.max(0, selectedIndex - 1);
+      let previousSnapshotId = snapshots[previousIndex].id;
+      dispatch(selectSnapshotAndRefresh(heapWorker, previousSnapshotId));
+    }
+    // On ACCEL+DOWN, select next snapshot.
+    if (isAccelKey && e.key === "ArrowDown") {
+      let nextIndex = Math.min(snapshots.length - 1, selectedIndex + 1);
+      let nextSnapshotId = snapshots[nextIndex].id;
+      dispatch(selectSnapshotAndRefresh(heapWorker, nextSnapshotId));
+    }
+  },
+
   render() {
     let {
       dispatch,
       snapshots,
       front,
       heapWorker,
       breakdown,
       allocations,
--- a/devtools/client/memory/test/browser/browser.ini
+++ b/devtools/client/memory/test/browser/browser.ini
@@ -10,14 +10,15 @@ support-files =
     skip-if = debug # bug 1219554
 [browser_memory_breakdowns_01.js]
 [browser_memory_clear_snapshots.js]
 [browser_memory_diff_01.js]
 [browser_memory_dominator_trees_01.js]
 [browser_memory_dominator_trees_02.js]
 [browser_memory_filter_01.js]
 [browser_memory_keyboard.js]
+[browser_memory_keyboard-snapshot-list.js]
 [browser_memory_no_allocation_stacks.js]
 [browser_memory_no_auto_expand.js]
     skip-if = debug # bug 1219554
 [browser_memory_percents_01.js]
 [browser_memory_simple_01.js]
 [browser_memory_transferHeapSnapshot_e10s_01.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/memory/test/browser/browser_memory_keyboard-snapshot-list.js
@@ -0,0 +1,91 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Test that using ACCEL+UP/DOWN, the user can navigate between snapshots.
+
+"use strict";
+
+const {
+  snapshotState
+} = require("devtools/client/memory/constants");
+const {
+  takeSnapshotAndCensus
+} = require("devtools/client/memory/actions/snapshot");
+
+const TEST_URL = "http://example.com/browser/devtools/client/memory/test/browser/doc_steady_allocation.html";
+
+this.test = makeMemoryTest(TEST_URL, function* ({ panel }) {
+  const heapWorker = panel.panelWin.gHeapAnalysesClient;
+  const front = panel.panelWin.gFront;
+  const store = panel.panelWin.gStore;
+  const { dispatch } = store;
+  const doc = panel.panelWin.document;
+
+  info("Take 3 snapshots");
+  dispatch(takeSnapshotAndCensus(front, heapWorker));
+  dispatch(takeSnapshotAndCensus(front, heapWorker));
+  dispatch(takeSnapshotAndCensus(front, heapWorker));
+
+  yield waitUntilState(store, state =>
+    state.snapshots.length == 3 &&
+    state.snapshots.every(s => s.state === snapshotState.SAVED_CENSUS));
+  ok(true, "All snapshots are in SAVED_CENSUS state");
+
+  yield waitUntilSnapshotSelected(store, 2);
+  ok(true, "Third snapshot selected after creating all snapshots.");
+
+  info("Press ACCEL+UP key, expect second snapshot selected.");
+  EventUtils.synthesizeKey("VK_UP", { accelKey: true }, panel.panelWin);
+  yield waitUntilSnapshotSelected(store, 1);
+  ok(true, "Second snapshot selected after alt+UP.");
+
+  info("Press ACCEL+UP key, expect first snapshot selected.");
+  EventUtils.synthesizeKey("VK_UP", { accelKey: true }, panel.panelWin);
+  yield waitUntilSnapshotSelected(store, 0);
+  ok(true, "First snapshot is selected after ACCEL+UP");
+
+  info("Check ACCEL+UP is a noop when the first snapshot is selected.");
+  EventUtils.synthesizeKey("VK_UP", { accelKey: true }, panel.panelWin);
+  // We assume the snapshot selection should be synchronous here.
+  is(getSelectedSnapshotIndex(store), 0, "First snapshot is still selected");
+
+  info("Press ACCEL+DOWN key, expect second snapshot selected.");
+  EventUtils.synthesizeKey("VK_DOWN", { accelKey: true }, panel.panelWin);
+  yield waitUntilSnapshotSelected(store, 1);
+  ok(true, "Second snapshot is selected after ACCEL+DOWN");
+
+  info("Click on first node.");
+  let firstNode = doc.querySelector(".tree .heap-tree-item-name");
+  EventUtils.synthesizeMouseAtCenter(firstNode, {}, panel.panelWin);
+  yield waitUntilState(store, state => state.snapshots[1].census.focused ===
+      state.snapshots[1].census.report.children[0]
+  );
+  ok(true, "First root is selected after click.");
+
+  info("Press DOWN key, expect second root focused.");
+  EventUtils.synthesizeKey("VK_DOWN", {}, panel.panelWin);
+  yield waitUntilState(store, state => state.snapshots[1].census.focused ===
+      state.snapshots[1].census.report.children[1]
+  );
+  ok(true, "Second root is selected after pressing DOWN.");
+  is(getSelectedSnapshotIndex(store), 1, "Second snapshot is still selected");
+
+  info("Press UP key, expect second root focused.");
+  EventUtils.synthesizeKey("VK_UP", {}, panel.panelWin);
+  yield waitUntilState(store, state => state.snapshots[1].census.focused ===
+      state.snapshots[1].census.report.children[0]
+  );
+  ok(true, "First root is selected after pressing UP.");
+  is(getSelectedSnapshotIndex(store), 1, "Second snapshot is still selected");
+
+  info("Press ACCEL+DOWN key, expect third snapshot selected.");
+  EventUtils.synthesizeKey("VK_DOWN", { accelKey: true }, panel.panelWin);
+  yield waitUntilSnapshotSelected(store, 2);
+  ok(true, "ThirdĖ† snapshot is selected after ACCEL+DOWN");
+
+  info("Check ACCEL+DOWN is a noop when the last snapshot is selected.");
+  EventUtils.synthesizeKey("VK_DOWN", { accelKey: true }, panel.panelWin);
+  // We assume the snapshot selection should be synchronous here.
+  is(getSelectedSnapshotIndex(store), 2, "Third snapshot is still selected");
+
+});
--- a/devtools/client/memory/test/browser/head.js
+++ b/devtools/client/memory/test/browser/head.js
@@ -137,8 +137,31 @@ function setBreakdown (window, type) {
  * displayed.
  *
  * @param {Document} document
  */
 function getDisplayedSnapshotStatus(document) {
   const status = document.querySelector(".snapshot-status");
   return status ? status.textContent.trim() : null;
 }
+
+/**
+ * Get the index of the currently selected snapshot.
+ *
+ * @return {Number}
+ */
+function getSelectedSnapshotIndex(store) {
+  let snapshots = store.getState().snapshots;
+  let selectedSnapshot = snapshots.find(s => s.selected);
+  return snapshots.indexOf(selectedSnapshot);
+}
+
+/**
+ * Returns a promise that will resolve when the snapshot with provided index
+ * becomes selected.
+ *
+ * @return {Promise}
+ */
+function waitUntilSnapshotSelected(store, snapshotIndex) {
+  return waitUntilState(store, state =>
+    state.snapshots[snapshotIndex] &&
+    state.snapshots[snapshotIndex].selected === true);
+}
--- a/devtools/client/shared/components/test/mochitest/test_tree_06.html
+++ b/devtools/client/shared/components/test/mochitest/test_tree_06.html
@@ -271,16 +271,45 @@ window.onload = Task.async(function* () 
       "--H:false",
       "--I:false",
       "-D:false",
       "--J:false",
       "M:false",
       "-N:false",
       "--O:false",
     ], "After the RIGHT, K should be focused.");
+
+    // Check that keys are ignored if any modifier is present.
+    let keysWithModifier = [
+      { key: "ArrowDown", altKey: true },
+      { key: "ArrowDown", ctrlKey: true },
+      { key: "ArrowDown", metaKey: true },
+      { key: "ArrowDown", shiftKey: true },
+    ];
+    for (let key of keysWithModifier) {
+      Simulate.keyDown(document.querySelector(".tree"), key);
+      yield forceRender(tree);
+      isRenderedTree(document.body.textContent, [
+        "A:false",
+        "-B:false",
+        "--E:false",
+        "---K:true",
+        "---L:false",
+        "--F:false",
+        "--G:false",
+        "-C:false",
+        "--H:false",
+        "--I:false",
+        "-D:false",
+        "--J:false",
+        "M:false",
+        "-N:false",
+        "--O:false",
+      ], "After DOWN + (alt|ctrl|meta|shift), K should remain focused.");
+    }
   } catch(e) {
     ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
   } finally {
     SimpleTest.finish();
   }
 });
 </script>
 </pre>
--- a/devtools/client/shared/components/tree.js
+++ b/devtools/client/shared/components/tree.js
@@ -400,16 +400,21 @@ const Tree = module.exports = createClas
    *
    * @param {Event} e
    */
   _onKeyDown(e) {
     if (this.props.focused == null) {
       return;
     }
 
+    // Allow parent nodes to use navigation arrows with modifiers.
+    if (e.altKey || e.ctrlKey || e.shiftKey || e.metaKey) {
+      return;
+    }
+
     // Prevent scrolling when pressing navigation keys. Guard against mocked
     // events received when testing.
     if (e.nativeEvent && e.nativeEvent.preventDefault) {
       ViewHelpers.preventScrolling(e.nativeEvent);
     }
 
     switch (e.key) {
       case "ArrowUp":