Bug 1518487 - make sure VirtualizedTree keyboard focus scrolling works for trees that are themselves inside scrollable containers. Use the same approach as the tree component in debugger.html. r=nchevobbe

MozReview-Commit-ID: 4HO7WDbyPKA

Differential Revision: https://phabricator.services.mozilla.com/D19051

--HG--
extra : moz-landing-system : lando

devtools/client/shared/components/VirtualizedTree.js

@@ -7,6 +7,7 @@
 const { Component, createFactory } = require("devtools/client/shared/vendor/react");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
+const { scrollIntoView } = require("devtools/client/shared/scroll");
 
 const AUTO_EXPAND_DEPTH = 0;
 const NUMBER_OF_OFFSCREEN_ITEMS = 1;
@@ -429,22 +430,14 @@ class Tree extends Component {
    * @param {Object|undefined} item
    *        The item to be focused, or undefined to focus no item.
    */
-  _focus(index, item) {
+  _focus(index, item, options = {}) {
-    if (item !== undefined) {
+    if (item !== undefined && !options.preventAutoScroll) {
-      const itemStartPosition = index * this.props.itemHeight;
+      const treeElement = this.refs.tree;
-      const itemEndPosition = (index + 1) * this.props.itemHeight;
+      const element = document.getElementById(this.props.getKey(item));
-
+      scrollIntoView(element, {
-      // Note that if the height of the viewport (this.state.height) is less
+        ...options,
-      // than `this.props.itemHeight`, we could accidentally try and scroll both
+        container: treeElement,
-      // 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) {
@@ -553,13 +546,13 @@ class Tree extends Component {
 
   _focusFirstNode() {
     const traversal = this._dfsFromRoots();
-    this._focus(0, traversal[0].item);
+    this._focus(0, traversal[0].item, { alignTo: "top" });
   }
 
   _focusLastNode() {
     const traversal = this._dfsFromRoots();
     const lastIndex = traversal.length - 1;
-    this._focus(lastIndex, traversal[lastIndex].item);
+    this._focus(lastIndex, traversal[lastIndex].item, { alignTo: "bottom" });
   }
 
   /**
@@ -588,7 +581,7 @@ class Tree extends Component {
       return;
     }
 
-    this._focus(prevIndex, prev);
+    this._focus(prevIndex, prev, { alignTo: "top" });
   }
 
   /**
@@ -612,7 +605,7 @@ class Tree extends Component {
     }
 
     if (i + 1 < traversal.length) {
-      this._focus(i + 1, traversal[i + 1].item);
+      this._focus(i + 1, traversal[i + 1].item, { alignTo: "bottom" });
     }
   }
 
@@ -635,7 +628,7 @@ class Tree extends Component {
       }
     }
 
-    this._focus(parentIndex, parent);
+    this._focus(parentIndex, parent, { alignTo: "top" });
   }
 
   render() {
@@ -687,7 +680,9 @@ class Tree extends Component {
         hasChildren: !!this.props.getChildren(item).length,
         onExpand: this._onExpand,
         onCollapse: this._onCollapse,
-        onClick: () => this._focus(begin + i, item),
+        // Since the user just clicked the node, there's no need to check if
+        // it should be scrolled into view.
+        onClick: () => this._focus(begin + i, item, { preventAutoScroll: true }),
       }));
     }
 

devtools/client/shared/components/test/mochitest/test_tree_11.html

@@ -13,14 +13,15 @@ Test that when an item in the Tree component is focused by arrow key, the view i
   <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>
-   * {
+   .tree {
-       margin: 0;
+     height: 30px;
-       padding: 0;
+     overflow: auto;
-       height: 30px;
+     display: block;
-       max-height: 30px;
+   }
-       min-height: 30px;
+
-       font-size: 10px;
+   .tree-node {
-       overflow: auto;
+     font-size: 10px;
+     height: 10px;
    }
   </style>
 </head>

devtools/client/shared/components/test/mochitest/test_tree_13.html

@@ -13,9 +13,14 @@ Test trees have the correct scroll position when they are resized.
   <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
   <style>
    .tree {
-       height: 50px;
+     height: 50px;
-       overflow: auto;
+     overflow: auto;
-       display: block;
+     display: block;
+   }
+
+   .tree-node {
+     font-size: 10px;
+     height: 10px;
    }
   </style>
 </head>

devtools/client/shared/scroll.js

@@ -64,6 +64,57 @@ define(function(require, exports, module) {
       }
     }
   }
+
+  function closestScrolledParent(node) {
+    if (node == null) {
+      return null;
+    }
+
+    if (node.scrollHeight > node.clientHeight) {
+      return node;
+    }
+
+    return closestScrolledParent(node.parentNode);
+  }
+
+  /**
+   * Scrolls the element into view if it is not visible.
+   *
+   * @param {DOMNode|undefined} element
+   *        The item to be scrolled to.
+   *
+   * @param {Object|undefined} options
+   *        An options object which can contain:
+   *          - container: possible scrollable container. If it is not scrollable, we will
+   *                       look it up.
+   *          - alignTo:   "top" or "bottom" to indicate if we should scroll the element
+   *                       to the top or the bottom of the scrollable container when the
+   *                       element is off canvas.
+   */
+  function scrollIntoView(element, options = {}) {
+    if (!element) {
+      return;
+    }
+
+    const { alignTo, container } = options;
+
+    const { top, bottom } = element.getBoundingClientRect();
+    const scrolledParent = closestScrolledParent(container || element.parentNode);
+    const scrolledParentRect = scrolledParent ? scrolledParent.getBoundingClientRect() :
+                                                null;
+    const isVisible = !scrolledParent ||
+      (top >= scrolledParentRect.top && bottom <= scrolledParentRect.bottom);
+
+    if (isVisible) {
+      return;
+    }
+
+    const scrollToTop = alignTo ?
+      alignTo === "top" : !scrolledParentRect || top < scrolledParentRect.top;
+    element.scrollIntoView(scrollToTop);
+  }
+
   // Exports from this module
   module.exports.scrollIntoViewIfNeeded = scrollIntoViewIfNeeded;
+  module.exports.scrollIntoView = scrollIntoView;
 });