Bug 1221384 - Scroll the Tree component when focusing new items with the arrow keys; r=jsantell draft
authorNick Fitzgerald <fitzgen@gmail.com>
Mon, 04 Apr 2016 16:54:00 +0200
changeset 348308 4e9643a989b9bec3014c76de9d7cdce21b224e81
parent 348307 d6d563040da7366eba39b1c2ab67a8704466e8e4
child 348309 cfb3701b5edd3d377439ddda60aa3997b94d0183
push id14805
push userbmo:jacheng@mozilla.com
push dateThu, 07 Apr 2016 07:37:41 +0000
reviewersjsantell
bugs1221384
milestone48.0a1
Bug 1221384 - Scroll the Tree component when focusing new items with the arrow keys; r=jsantell
devtools/client/shared/components/test/mochitest/chrome.ini
devtools/client/shared/components/test/mochitest/test_tree_11.html
devtools/client/shared/components/tree.js
--- a/devtools/client/shared/components/test/mochitest/chrome.ini
+++ b/devtools/client/shared/components/test/mochitest/chrome.ini
@@ -9,8 +9,9 @@ support-files =
 [test_tree_03.html]
 [test_tree_04.html]
 [test_tree_05.html]
 [test_tree_06.html]
 [test_tree_07.html]
 [test_tree_08.html]
 [test_tree_09.html]
 [test_tree_10.html]
+[test_tree_11.html]
new file mode 100644
--- /dev/null
+++ b/devtools/client/shared/components/test/mochitest/test_tree_11.html
@@ -0,0 +1,89 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+Test that when an item in the Tree component is focused by arrow key, the view is scrolled.
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Tree component test</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <link rel="stylesheet" href="chrome://devtools/skin/light-theme.css" type="text/css">
+  <style>
+   * {
+       margin: 0;
+       padding: 0;
+       height: 30px;
+       max-height: 30px;
+       min-height: 30px;
+       font-size: 10px;
+       overflow: auto;
+   }
+  </style>
+</head>
+<body>
+<pre id="test">
+<script src="head.js" type="application/javascript;version=1.8"></script>
+<script type="application/javascript;version=1.8">
+window.onload = Task.async(function* () {
+  try {
+    const ReactDOM = browserRequire("devtools/client/shared/vendor/react-dom");
+    const React = browserRequire("devtools/client/shared/vendor/react");
+    const { Simulate } = React.addons.TestUtils;
+    const Tree = React.createFactory(browserRequire("devtools/client/shared/components/tree"));
+
+    TEST_TREE.expanded = new Set("ABCDEFGHIJKLMNO".split(""));
+
+    const tree = ReactDOM.render(Tree(TEST_TREE_INTERFACE), window.document.body);
+
+    yield setProps(tree, {
+      itemHeight: 10,
+      onFocus: item => setProps(tree, { focused: item }),
+      focused: "K",
+    });
+    yield setState(tree, {
+      scroll: 10,
+    });
+    yield forceRender(tree);
+
+    isRenderedTree(document.body.textContent, [
+      "A:false",
+      "-B:false",
+      "--E:false",
+      "---K:true",
+      "---L:false",
+    ], "Should render initial correctly");
+
+    yield new Promise(resolve => {
+      const treeElem = document.querySelector(".tree");
+      treeElem.addEventListener("scroll", function onScroll() {
+        dumpn("Got scroll event");
+        treeElem.removeEventListener("scroll", onScroll);
+        resolve();
+      });
+
+      dumpn("Sending ArrowDown key");
+      Simulate.keyDown(treeElem, { key: "ArrowDown" });
+    });
+
+    dumpn("Forcing re-render");
+    yield forceRender(tree);
+
+    isRenderedTree(document.body.textContent, [
+      "-B:false",
+      "--E:false",
+      "---K:false",
+      "---L:true",
+      "--F:false",
+    ], "Should have scrolled down one");
+
+  } catch(e) {
+    ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
+  } finally {
+    SimpleTest.finish();
+  }
+});
+</script>
+</pre>
+</body>
+</html>
--- a/devtools/client/shared/components/tree.js
+++ b/devtools/client/shared/components/tree.js
@@ -56,17 +56,17 @@ const TreeNode = createFactory(createCla
   },
 
   render() {
     const arrow = ArrowExpander({
       item: this.props.item,
       expanded: this.props.expanded,
       visible: this.props.hasChildren,
       onExpand: this.props.onExpand,
-      onCollapse: this.props.onCollapse
+      onCollapse: this.props.onCollapse,
     });
 
     let isOddRow = this.props.index % 2;
     return dom.div(
       {
         className: `tree-node div ${isOddRow ? "tree-node-odd" : ""}`,
         onFocus: this.props.onFocus,
         onClick: this.props.onFocus,
@@ -110,24 +110,27 @@ const TreeNode = createFactory(createCla
  * Create a function that calls the given function `fn` only once per animation
  * frame.
  *
  * @param {Function} fn
  * @returns {Function}
  */
 function oncePerAnimationFrame(fn) {
   let animationId = null;
+  let argsToPass = null;
   return function (...args) {
+    argsToPass = args;
     if (animationId !== null) {
       return;
     }
 
     animationId = requestAnimationFrame(() => {
+      fn.call(this, ...argsToPass);
       animationId = null;
-      fn.call(this, ...args);
+      argsToPass = null;
     });
   };
 }
 
 const NUMBER_OF_OFFSCREEN_ITEMS = 1;
 
 /**
  * A generic tree component. See propTypes for the public API.
@@ -230,19 +233,21 @@ const Tree = module.exports = createClas
     for (let i = 0; i < length; i++) {
       autoExpand(roots[i], 0);
     }
   },
 
   render() {
     const traversal = this._dfsFromRoots();
 
-    // Remove 1 from `begin` and add 2 to `end` so that the top and bottom of
-    // the page are filled with the previous and next items respectively,
-    // rather than whitespace if the item is not in full view.
+    // Remove `NUMBER_OF_OFFSCREEN_ITEMS` from `begin` and add `2 *
+    // NUMBER_OF_OFFSCREEN_ITEMS` to `end` so that the top and bottom of the
+    // page are filled with the `NUMBER_OF_OFFSCREEN_ITEMS` previous and next
+    // items respectively, rather than whitespace if the item is not in full
+    // view.
     const begin = Math.max(((this.state.scroll / this.props.itemHeight) | 0) - NUMBER_OF_OFFSCREEN_ITEMS, 0);
     const end = begin + (2 * NUMBER_OF_OFFSCREEN_ITEMS) + ((this.state.height / this.props.itemHeight) | 0);
     const toRender = traversal.slice(begin, end);
 
     const nodes = [
       dom.div({
         key: "top-spacer",
         style: {
@@ -261,17 +266,17 @@ const Tree = module.exports = createClas
         item: item,
         depth: depth,
         renderItem: this.props.renderItem,
         focused: this.props.focused === item,
         expanded: this.props.isExpanded(item),
         hasChildren: !!this.props.getChildren(item).length,
         onExpand: this._onExpand,
         onCollapse: this._onCollapse,
-        onFocus: () => this._focus(item)
+        onFocus: () => this._focus(begin + i, item),
       }));
     }
 
     nodes.push(dom.div({
       key: "bottom-spacer",
       style: {
         padding: 0,
         margin: 0,
@@ -279,26 +284,47 @@ const Tree = module.exports = createClas
       }
     }));
 
     return dom.div(
       {
         className: "tree",
         ref: "tree",
         onKeyDown: this._onKeyDown,
+        onKeyPress: this._preventArrowKeyScrolling,
+        onKeyUp: this._preventArrowKeyScrolling,
         onScroll: this._onScroll,
         style: {
           padding: 0,
           margin: 0
         }
       },
       nodes
     );
   },
 
+  _preventArrowKeyScrolling(e) {
+    switch (e.key) {
+      case "ArrowUp":
+      case "ArrowDown":
+      case "ArrowLeft":
+      case "ArrowRight":
+        e.preventDefault();
+        e.stopPropagation();
+        if (e.nativeEvent) {
+          if (e.nativeEvent.preventDefault) {
+            e.nativeEvent.preventDefault();
+          }
+          if (e.nativeEvent.stopPropagation) {
+            e.nativeEvent.stopPropagation();
+          }
+        }
+    }
+  },
+
   /**
    * Updates the state's height based on clientHeight.
    */
   _updateHeight() {
     this.setState({
       height: this.refs.tree.clientHeight
     });
   },
@@ -372,29 +398,51 @@ const Tree = module.exports = createClas
     if (this.props.onCollapse) {
       this.props.onCollapse(item);
     }
   }),
 
   /**
    * Sets the passed in item to be the focused item.
    *
-   * @param {Object} item
+   * @param {Number} index
+   *        The index of the item in a full DFS traversal (ignoring collapsed
+   *        nodes). Ignored if `item` is undefined.
+   *
+   * @param {Object|undefined} item
+   *        The item to be focused, or undefined to focus no item.
    */
-  _focus(item) {
+  _focus(index, item) {
+    if (item !== undefined) {
+      const itemStartPosition = index * this.props.itemHeight;
+      const itemEndPosition = (index + 1) * this.props.itemHeight;
+
+      // Note that if the height of the viewport (this.state.height) is less than
+      // `this.props.itemHeight`, we could accidentally try and scroll both up and
+      // down in a futile attempt to make both the item's start and end positions
+      // visible. Instead, give priority to the start of the item by checking its
+      // position first, and then using an "else if", rather than a separate "if",
+      // for the end position.
+      if (this.state.scroll > itemStartPosition) {
+        this.refs.tree.scrollTo(0, itemStartPosition);
+      } else if ((this.state.scroll + this.state.height) < itemEndPosition) {
+        this.refs.tree.scrollTo(0, itemEndPosition - this.state.height);
+      }
+    }
+
     if (this.props.onFocus) {
       this.props.onFocus(item);
     }
   },
 
   /**
    * Sets the state to have no focused item.
    */
   _onBlur() {
-    this._focus(undefined);
+    this._focus(0, undefined);
   },
 
   /**
    * Fired on a scroll within the tree's container, updates
    * the stored position of the view port to handle virtual view rendering.
    *
    * @param {Event} e
    */
@@ -415,75 +463,73 @@ const Tree = module.exports = createClas
       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);
-    }
+    this._preventArrowKeyScrolling(e);
 
     switch (e.key) {
       case "ArrowUp":
         this._focusPrevNode();
-        return false;
+        return;
 
       case "ArrowDown":
         this._focusNextNode();
-        return false;
+        return;
 
       case "ArrowLeft":
         if (this.props.isExpanded(this.props.focused)
             && this.props.getChildren(this.props.focused).length) {
           this._onCollapse(this.props.focused);
         } else {
           this._focusParentNode();
         }
-        return false;
+        return;
 
       case "ArrowRight":
         if (!this.props.isExpanded(this.props.focused)) {
           this._onExpand(this.props.focused);
         } else {
           this._focusNextNode();
         }
-        return false;
-      }
+        return;
+    }
   },
 
   /**
    * Sets the previous node relative to the currently focused item, to focused.
    */
   _focusPrevNode: oncePerAnimationFrame(function () {
     // Start a depth first search and keep going until we reach the currently
     // focused node. Focus the previous node in the DFS, if it exists. If it
     // doesn't exist, we're at the first node already.
 
     let prev;
+    let prevIndex;
 
     const traversal = this._dfsFromRoots();
     const length = traversal.length;
     for (let i = 0; i < length; i++) {
       const item = traversal[i].item;
       if (item === this.props.focused) {
         break;
       }
       prev = item;
+      prevIndex = i;
     }
 
     if (prev === undefined) {
       return;
     }
 
-    this._focus(prev);
+    this._focus(prevIndex, prev);
   }),
 
   /**
    * Handles the down arrow key which will focus either the next child
    * or sibling row.
    */
   _focusNextNode: oncePerAnimationFrame(function () {
     // Start a depth first search and keep going until we reach the currently
@@ -497,23 +543,34 @@ const Tree = module.exports = createClas
     while (i < length) {
       if (traversal[i].item === this.props.focused) {
         break;
       }
       i++;
     }
 
     if (i + 1 < traversal.length) {
-      this._focus(traversal[i + 1].item);
+      this._focus(i + 1, traversal[i + 1].item);
     }
   }),
 
   /**
    * Handles the left arrow key, going back up to the current rows'
    * parent row.
    */
   _focusParentNode: oncePerAnimationFrame(function () {
     const parent = this.props.getParent(this.props.focused);
-    if (parent) {
-      this._focus(parent);
+    if (!parent) {
+      return;
     }
+
+    const traversal = this._dfsFromRoots();
+    const length = traversal.length;
+    let parentIndex = 0;
+    for (; parentIndex < length; parentIndex++) {
+      if (traversal[parentIndex].item === parent) {
+        break;
+      }
+    }
+
+    this._focus(parentIndex, parent);
   }),
 });