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
authorYura Zenevich <yura.zenevich@gmail.com>
Tue, 12 Feb 2019 19:39:46 +0000
changeset 458786 4923cfe749d5
parent 458785 bcda0131d8f5
child 458787 8de6b5e7abb3
push id35548
push useropoprus@mozilla.com
push dateWed, 13 Feb 2019 09:48:26 +0000
treeherdermozilla-central@93e37c529818 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnchevobbe
bugs1518487
milestone67.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 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
devtools/client/shared/components/VirtualizedTree.js
devtools/client/shared/components/test/mochitest/test_tree_11.html
devtools/client/shared/components/test/mochitest/test_tree_13.html
devtools/client/shared/scroll.js
--- a/devtools/client/shared/components/VirtualizedTree.js
+++ b/devtools/client/shared/components/VirtualizedTree.js
@@ -2,16 +2,17 @@
  * 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/. */
 /* eslint-env browser */
 "use strict";
 
 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;
 
 /**
  * A fast, generic, expandable and collapsible tree component.
  *
  * This tree component is fast: it can handle trees with *many* items. It only
@@ -424,32 +425,24 @@ class Tree extends Component {
    *
    * @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(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);
-      }
+  _focus(index, item, options = {}) {
+    if (item !== undefined && !options.preventAutoScroll) {
+      const treeElement = this.refs.tree;
+      const element = document.getElementById(this.props.getKey(item));
+      scrollIntoView(element, {
+        ...options,
+        container: treeElement,
+      });
     }
 
     if (this.props.onFocus) {
       this.props.onFocus(item);
     }
   }
 
   /**
@@ -548,23 +541,23 @@ class Tree extends Component {
   _activateNode() {
     if (this.props.onActivate) {
       this.props.onActivate(this.props.focused);
     }
   }
 
   _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" });
   }
 
   /**
    * Sets the previous node relative to the currently focused item, to focused.
    */
   _focusPrevNode() {
     // 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
@@ -583,17 +576,17 @@ class Tree extends Component {
       prev = item;
       prevIndex = i;
     }
 
     if (prev === undefined) {
       return;
     }
 
-    this._focus(prevIndex, prev);
+    this._focus(prevIndex, prev, { alignTo: "top" });
   }
 
   /**
    * Handles the down arrow key which will focus either the next child
    * or sibling row.
    */
   _focusNextNode() {
     // Start a depth first search and keep going until we reach the currently
@@ -607,17 +600,17 @@ class Tree extends Component {
     while (i < length) {
       if (traversal[i].item === this.props.focused) {
         break;
       }
       i++;
     }
 
     if (i + 1 < traversal.length) {
-      this._focus(i + 1, traversal[i + 1].item);
+      this._focus(i + 1, traversal[i + 1].item, { alignTo: "bottom" });
     }
   }
 
   /**
    * Handles the left arrow key, going back up to the current rows'
    * parent row.
    */
   _focusParentNode() {
@@ -630,17 +623,17 @@ class Tree extends Component {
     const length = traversal.length;
     let parentIndex = 0;
     for (; parentIndex < length; parentIndex++) {
       if (traversal[parentIndex].item === parent) {
         break;
       }
     }
 
-    this._focus(parentIndex, parent);
+    this._focus(parentIndex, parent, { alignTo: "top" });
   }
 
   render() {
     const traversal = this._dfsFromRoots();
 
     // 'begin' and 'end' are the index of the first (at least partially) visible item
     // and the index after the last (at least partially) visible item, respectively.
     // `NUMBER_OF_OFFSCREEN_ITEMS` is removed from `begin` and added to `end` so that
@@ -682,17 +675,19 @@ class Tree extends Component {
         depth,
         id: key,
         renderItem: this.props.renderItem,
         focused: focused === item,
         expanded: this.props.isExpanded(item),
         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 }),
       }));
     }
 
     nodes.push(dom.div({
       key: "bottom-spacer",
       role: "presentation",
       style: {
         padding: 0,
--- a/devtools/client/shared/components/test/mochitest/test_tree_11.html
+++ b/devtools/client/shared/components/test/mochitest/test_tree_11.html
@@ -8,24 +8,25 @@ Test that when an item in the Tree compo
 -->
 <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;
+   .tree {
+     height: 30px;
+     overflow: auto;
+     display: block;
+   }
+
+   .tree-node {
+     font-size: 10px;
+     height: 10px;
    }
   </style>
 </head>
 <body>
 <pre id="test">
 <script src="head.js" type="application/javascript"></script>
 <script type="application/javascript">
 window.onload = async function () {
--- a/devtools/client/shared/components/test/mochitest/test_tree_13.html
+++ b/devtools/client/shared/components/test/mochitest/test_tree_13.html
@@ -8,19 +8,24 @@ Test trees have the correct scroll posit
 -->
 <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">
   <style>
    .tree {
-       height: 50px;
-       overflow: auto;
-       display: block;
+     height: 50px;
+     overflow: auto;
+     display: block;
+   }
+
+   .tree-node {
+     font-size: 10px;
+     height: 10px;
    }
   </style>
 </head>
 <body>
 <pre id="test">
 <script src="head.js" type="application/javascript"></script>
 <script type="application/javascript">
 
--- a/devtools/client/shared/scroll.js
+++ b/devtools/client/shared/scroll.js
@@ -59,11 +59,62 @@ define(function(require, exports, module
       if (yAllowed && (topToBottom <= 0 || bottomToTop >= 0)) {
         const x = win.scrollX;
         const y = win.scrollY + clientRect.top -
           (win.innerHeight - elem.offsetHeight) / 2;
         win.scroll(Object.assign({left: x, top: y}, options));
       }
     }
   }
+
+  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;
 });