Bug 1544710 - ensure that selected row is always visible within TreeView after update. Clean up scroll into view operations across all uses of TreeView. r=mtigley

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

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

devtools/client/accessibility/accessibility.css

@@ -12,6 +12,7 @@
   --accessibility-horizontal-padding: 5px;
   --accessibility-horizontal-indent: 20px;
   --accessibility-properties-item-width: calc(100% - var(--accessibility-horizontal-indent));
+  --accessibility-tree-height: calc(100vh - var(--accessibility-toolbar-height) * 2 - 1px);
   --accessibility-arrow-horizontal-padding: 4px;
   --accessibility-tree-row-height: 21px;
   --accessibility-unfocused-tree-focused-node-background: var(--grey-20);
@@ -71,16 +72,16 @@ body {
   fill: var(--theme-selection-color);
 }
 
-.mainFrame .main-panel {
-  flex: 1 1 auto;
-  overflow: auto;
-}
-
 .mainFrame {
   height: 100%;
   color: var(--theme-toolbar-color);
 }
 
+.main-panel {
+  /* To compenstate for 1px splitter between the tree and sidebar. */
+  width: var(--accessibility-full-length-minus-splitter);
+}
+
 .devtools-button,
 .toggle-button {
   cursor: pointer;
@@ -225,24 +226,30 @@ body {
 }
 
 /* TreeView Customization */
-.split-box:not(.horz) .main-panel {
+.treeTable thead, .treeTable tbody {
-  height: calc(100vh - var(--accessibility-toolbar-height));
+  display: block;
 }
 
-.treeTable > thead {
+.treeTable tr {
-  position: sticky;
+  width: 100%;
-  top: 0;
+  display: table;
-  /* Bug 1466806 - fix expander arrow for expanding treeview rows rendering over the
-     thead */
-  z-index: 1;
 }
 
-.split-box:not(.horz) .treeTable {
+.treeTable tbody {
-  /* To compenstate for 1px splitter between the tree and sidebar. */
+  overflow-y: auto;
-  width: var(--accessibility-full-length-minus-splitter);
 }
 
-.split-box.horz .treeTable {
+.split-box:not(.horz) .treeTable tbody {
+  height: var(--accessibility-tree-height);
+}
+
+.split-box.horz .treeTable tbody {
+  /* Accessibility tree height depends on the height of the controlled panel
+     (sidebar) when in horz mode and also has an additional separator. */
+  height: calc(var(--accessibility-tree-height) - var(--split-box-controlled-panel-size) - 1px);
+}
+
+.treeTable {
   width: 100%;
 }
 
@@ -318,6 +325,7 @@ body {
 }
 
 .mainFrame .treeTable .treeHeaderCell {
+  width: 50%;
   border-bottom: 1px solid var(--theme-splitter-color);
   background: var(--theme-toolbar-background);
   font: message-box;

devtools/client/accessibility/components/AccessibilityRow.js

@@ -30,6 +30,8 @@ const { L10N } = require("../utils/l10n");
 loader.lazyRequireGetter(this, "Menu", "devtools/client/framework/menu");
 loader.lazyRequireGetter(this, "MenuItem", "devtools/client/framework/menu-item");
 
+const { scrollIntoView } = require("devtools/client/shared/scroll");
+
 const JSON_URL_PREFIX = "data:application/json;charset=UTF-8,";
 
 const TELEMETRY_ACCESSIBLE_CONTEXT_MENU_OPENED =
@@ -71,7 +73,7 @@ class AccessibilityRow extends Component {
     const { selected, object } = this.props.member;
     if (selected) {
       this.unhighlight();
-      this.updateAndScrollIntoViewIfNeeded();
+      this.update();
       this.highlight(object, { duration: VALUE_HIGHLIGHT_DURATION });
     }
 
@@ -89,7 +91,7 @@ class AccessibilityRow extends Component {
     // If row is selected, update corresponding accessible details.
     if (!prevProps.member.selected && selected) {
       this.unhighlight();
-      this.updateAndScrollIntoViewIfNeeded();
+      this.update();
       this.highlight(object, { duration: VALUE_HIGHLIGHT_DURATION });
     }
 
@@ -110,17 +112,16 @@ class AccessibilityRow extends Component {
       return;
     }
 
-    row.scrollIntoView({ block: "center" });
+    scrollIntoView(row, { container: document.querySelector(".treeTable > tbody") });
   }
 
-  updateAndScrollIntoViewIfNeeded() {
+  update() {
     const { dispatch, member: { object }, supports } = this.props;
     if (!gToolbox || !object.actorID) {
       return;
     }
 
     dispatch(updateDetails(gToolbox.walker, object, supports));
-    this.scrollIntoView();
     window.emit(EVENTS.NEW_ACCESSIBLE_FRONT_SELECTED, object);
   }
 

devtools/client/accessibility/components/AccessibilityTree.js

@@ -191,7 +191,10 @@ class AccessibilityTree extends Component {
           if (event.target.classList.contains("theme-twisty")) {
             this.toggle(nodePath);
           }
-          this.selectRow(event.currentTarget);
+
+          this.selectRow(
+            this.rows.find(row => row.props.member.path === nodePath),
+            { preventAutoScroll: true });
         },
         onContextMenuTree: hasContextMenu && function(e) {
           // If context menu event is triggered on (or bubbled to) the TreeView, it was

devtools/client/accessibility/test/browser/browser.ini

@@ -24,6 +24,7 @@ skip-if = (os == 'linux' && debug && bits == 64) # Bug 1511247
 [browser_accessibility_reload.js]
 [browser_accessibility_sidebar_checks.js]
 [browser_accessibility_sidebar.js]
+[browser_accessibility_tree_audit_long.js]
 [browser_accessibility_tree_audit_reset.js]
 [browser_accessibility_tree_audit_toolbar.js]
 [browser_accessibility_tree_audit.js]

devtools/client/accessibility/test/browser/browser_accessibility_tree_audit.js

@@ -65,6 +65,7 @@ const tests = [{
       role: "text leaf",
       name: `"Second level header "contrast`,
       badges: [ "contrast" ],
+      selected: true,
     }],
   },
 }, {
@@ -90,6 +91,7 @@ const tests = [{
       role: "text leaf",
       name: `"Second level header "contrast`,
       badges: [ "contrast" ],
+      selected: true,
     }],
   },
 }];

devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_long.js

@@ -0,0 +1,94 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/* global toggleFilter */
+
+const header = "<h1 style=\"color:rgba(255,0,0,0.1); " +
+               "background-color:rgba(255,255,255,1);\">header</h1>";
+
+const TEST_URI = `<html>
+  <head>
+    <meta charset="utf-8"/>
+    <title>Accessibility Panel Test</title>
+  </head>
+  <body>
+    ${header.repeat(20)}
+  </body>
+</html>`;
+
+const docRow = {
+  role: "document",
+  name: `"Accessibility Panel Test"`,
+};
+const headingRow = {
+  role: "heading",
+  name: `"header"`,
+};
+const textLeafRow = {
+  role: "text leaf",
+  name: `"header"contrast`,
+  badges: [ "contrast" ],
+};
+const audit = new Array(20).fill(textLeafRow);
+
+const auditInitial = audit.map(check => ({ ...check }));
+auditInitial[0].selected = true;
+
+const auditSecondLastSelected = audit.map(check => ({ ...check }));
+auditSecondLastSelected[18].selected = true;
+
+const resetAfterAudit = [docRow];
+for (let i = 0; i < 20; i++) {
+  resetAfterAudit.push(headingRow);
+  resetAfterAudit.push({ ...textLeafRow, selected: i === 18 });
+}
+
+/**
+* Test data has the format of:
+* {
+*   desc     {String}    description for better logging
+*   setup    {Function}  An optional setup that needs to be performed before
+*                        the state of the tree and the sidebar can be checked.
+*   expected {JSON}      An expected states for the tree and the sidebar.
+* }
+*/
+const tests = [{
+  desc: "Check initial state.",
+  expected: {
+    tree: [{ ...docRow, selected: true }],
+  },
+}, {
+  desc: "Run an audit from a11y panel toolbar by activating a filter.",
+  setup: async ({ doc }) => {
+    await toggleFilter(doc, 0);
+  },
+  expected: {
+    tree: auditInitial,
+  },
+}, {
+  desc: "Select a row that is guaranteed to have to be scrolled into view.",
+  setup: async ({ doc }) => {
+    await selectRow(doc, 18);
+  },
+  expected: {
+    tree: auditSecondLastSelected,
+  },
+}, {
+  desc: "Click on the filter again.",
+  setup: async ({ doc }) => {
+    await toggleFilter(doc, 0);
+  },
+  expected: {
+    tree: resetAfterAudit,
+  },
+}];
+
+/**
+* Simple test that checks content of the Accessibility panel tree when the
+* audit is activated via the panel's toolbar and the selection persists when
+* the filter is toggled off.
+*/
+addA11yPanelTestsTask(tests, TEST_URI,
+  "Test Accessibility panel tree with persistent selected row.");

devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_reset.js

@@ -35,6 +35,7 @@ const tests = [{
     tree: [{
       role: "document",
       name: `"Accessibility Panel Test"`,
+      selected: true,
     }],
   },
 }, {
@@ -47,6 +48,7 @@ const tests = [{
       role: "text leaf",
       name: `"Top level header "contrast`,
       badges: [ "contrast" ],
+      selected: true,
     }, {
       role: "text leaf",
       name: `"Second level header "contrast`,
@@ -62,6 +64,7 @@ const tests = [{
     tree: [{
       role: "document",
       name: `"Accessibility Panel Test"`,
+      selected: true,
     }, {
       role: "heading",
       name: `"Top level header"`,

devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_toolbar.js

@@ -35,6 +35,7 @@ const tests = [{
     tree: [{
       role: "document",
       name: `"Accessibility Panel Test"`,
+      selected: true,
     }],
   },
 }, {
@@ -47,6 +48,7 @@ const tests = [{
       role: "text leaf",
       name: `"Top level header "contrast`,
       badges: [ "contrast" ],
+      selected: true,
     }, {
       role: "text leaf",
       name: `"Second level header "contrast`,
@@ -69,6 +71,7 @@ const tests = [{
       role: "text leaf",
       name: `"Top level header "contrast`,
       badges: [ "contrast" ],
+      selected: true,
     }, {
       role: "heading",
       name: `"Second level header"`,

devtools/client/accessibility/test/browser/head.js

@@ -176,6 +176,66 @@ function compareBadges(badges, expected = []) {
          badgeEls.every((badge, i) => badge.textContent === expected[i]);
 }
 
+/**
+ * Find an ancestor that is scrolled for a given DOMNode.
+ *
+ * @param {DOMNode} node
+ *        DOMNode that to find an ancestor for that is scrolled.
+ */
+function closestScrolledParent(node) {
+  if (node == null) {
+    return null;
+  }
+
+  if (node.scrollHeight > node.clientHeight) {
+    return node;
+  }
+
+  return closestScrolledParent(node.parentNode);
+}
+
+/**
+ * Check if a given element is visible to the user and is not scrolled off
+ * because of the overflow.
+ *
+ * @param   {Element} element
+ *          Element to be checked whether it is visible and is not scrolled off.
+ *
+ * @returns {Boolean}
+ *          True if the element is visible.
+ */
+function isVisible(element) {
+  const { top, bottom } = element.getBoundingClientRect();
+  const scrolledParent = closestScrolledParent(element.parentNode);
+  const scrolledParentRect = scrolledParent ? scrolledParent.getBoundingClientRect() :
+                                              null;
+  return !scrolledParent ||
+         (top >= scrolledParentRect.top && bottom <= scrolledParentRect.bottom);
+}
+
+/**
+ * Check selected styling and visibility for a given row in the accessibility
+ * tree.
+ * @param   {DOMNode} row
+ *          DOMNode for a given accessibility row.
+ * @param   {Boolean} expected
+ *          Expected selected state.
+ *
+ * @returns {Boolean}
+ *          True if visibility and styling matches expected selected state.
+ */
+function checkSelected(row, expected) {
+  if (!expected) {
+    return true;
+  }
+
+  if (row.classList.contains("selected") !== expected) {
+    return false;
+  }
+
+  return isVisible(row);
+}
+
 /**
  * Check the state of the accessibility tree.
  * @param  {document} doc       panel documnent.
@@ -184,11 +244,13 @@ function compareBadges(badges, expected = []) {
 async function checkTreeState(doc, expected) {
   info("Checking tree state.");
   const hasExpectedStructure = await BrowserTestUtils.waitForCondition(() =>
-    [...doc.querySelectorAll(".treeRow")].every((row, i) =>
+    [...doc.querySelectorAll(".treeRow")].every((row, i) => {
-      row.querySelector(".treeLabelCell").textContent === expected[i].role &&
+      const { role, name, badges, selected } = expected[i];
-      row.querySelector(".treeValueCell").textContent === expected[i].name &&
+      return row.querySelector(".treeLabelCell").textContent === role &&
-      compareBadges(row.querySelector(".badges"), expected[i].badges)),
+        row.querySelector(".treeValueCell").textContent === name &&
-    "Wait for the right tree update.");
+        compareBadges(row.querySelector(".badges"), badges) &&
+        checkSelected(row, selected);
+    }), "Wait for the right tree update.");
 
   ok(hasExpectedStructure, "Tree structure is correct.");
 }

devtools/client/shared/components/splitter/SplitBox.js

@@ -203,7 +203,13 @@ class SplitBox extends Component {
     const { endPanelControl, splitterSize, vert } = this.state;
     const { startPanel, endPanel, minSize, maxSize } = this.props;
 
-    const style = Object.assign({}, this.props.style);
+    const style = Object.assign({
+      // Set the size of the controlled panel (height or width depending on the
+      // current state). This can be used to help with styling of dependent
+      // panels.
+      "--split-box-controlled-panel-size":
+        `${vert ? this.state.width : this.state.height}`,
+    }, this.props.style);
 
     // Calculate class names list.
     let classNames = ["split-box"];

devtools/client/shared/components/tree/TreeRow.js

@@ -21,8 +21,6 @@ define(function(require, exports, module) {
   const TreeCell = createFactory(require("./TreeCell"));
   const LabelCell = createFactory(require("./LabelCell"));
 
-  // Scroll
-  const { scrollIntoViewIfNeeded } = require("devtools/client/shared/scroll");
   const { focusableSelector } = require("devtools/client/shared/focus");
 
   const UPDATE_ON_PROPS = [
@@ -123,17 +121,6 @@ define(function(require, exports, module) {
       return false;
     }
 
-    componentDidUpdate() {
-      if (this.props.member.selected) {
-        const row = findDOMNode(this);
-        // Because this is called asynchronously, context window might be
-        // already gone.
-        if (row.ownerDocument.defaultView) {
-          scrollIntoViewIfNeeded(row);
-        }
-      }
-    }
-
     componentWillUnmount() {
       this.observer.disconnect();
       this.observer = null;

devtools/client/shared/components/tree/TreeView.js

@@ -22,6 +22,8 @@ define(function(require, exports, module) {
   const TreeRow = createFactory(require("./TreeRow"));
   const TreeHeader = createFactory(require("./TreeHeader"));
 
+  const { scrollIntoView } = require("devtools/client/shared/scroll");
+
   const SUPPORTED_KEYS = [
     "ArrowUp",
     "ArrowDown",
@@ -240,7 +242,10 @@ define(function(require, exports, module) {
       const selected = this.getSelectedRow();
       if (!selected && this.rows.length > 0) {
         this.selectRow(this.rows[
-          Math.min(this.state.lastSelectedIndex, this.rows.length - 1)]);
+          Math.min(this.state.lastSelectedIndex, this.rows.length - 1)
+        ], { alignTo: "top" });
+      } else {
+        this._scrollIntoView(this.getSelectedRow());
       }
     }
 
@@ -291,39 +296,34 @@ define(function(require, exports, module) {
             const parentRow = this.rows.slice(0, index).reverse().find(
               r => r.props.member.level < row.props.member.level);
             if (parentRow) {
-              this.selectRow(parentRow);
+              this.selectRow(parentRow, { alignTo: "top" });
             }
           }
           break;
         case "ArrowDown":
           const nextRow = this.rows[index + 1];
           if (nextRow) {
-            this.selectRow(nextRow);
+            this.selectRow(nextRow, { alignTo: "bottom" });
           }
           break;
         case "ArrowUp":
           const previousRow = this.rows[index - 1];
           if (previousRow) {
-            this.selectRow(previousRow);
+            this.selectRow(previousRow, { alignTo: "top" });
           }
           break;
         case "Home":
           const firstRow = this.rows[0];
 
           if (firstRow) {
-            // Due to the styling, the first row is sometimes overlapped by
+            this.selectRow(firstRow, { alignTo: "top" });
-            // the table head. So we want to force the tree to scroll to the very top.
-            this.selectRow(firstRow, {
-              block: "end",
-              inline: "nearest",
-            });
           }
           break;
 
         case "End":
           const lastRow = this.rows[this.rows.length - 1];
           if (lastRow) {
-            this.selectRow(lastRow);
+            this.selectRow(lastRow, { alignTo: "bottom" });
           }
           break;
 
@@ -367,7 +367,10 @@ define(function(require, exports, module) {
       if (cell && cell.classList.contains("treeLabelCell")) {
         this.toggle(nodePath);
       }
-      this.selectRow(event.currentTarget);
+
+      this.selectRow(
+        this.rows.find(row => row.props.member.path === nodePath),
+        { preventAutoScroll: true });
     }
 
     onContextMenu(member, event) {
@@ -394,11 +397,30 @@ define(function(require, exports, module) {
       return this.rows.indexOf(row);
     }
 
-    selectRow(row, scrollOptions = {block: "nearest"}) {
+    _scrollIntoView(row, options = {}) {
-      row = findDOMNode(row);
+      if (!this.treeRef.current || !row) {
+        return;
+      }
 
-      if (this.state.selected === row.id) {
+      const { props: { member: { path } = {} } = {} } = row;
-        row.scrollIntoView(scrollOptions);
+      if (!path) {
+        return;
+      }
+
+      const element = document.getElementById(path);
+      if (!element) {
+        return;
+      }
+
+      // For usability/accessibility purposes we do not want to scroll the
+      // thead. TreeView will scroll relative to the tbody.
+      const container = this.treeRef.current.tBodies[0];
+      scrollIntoView(element, { ...options, container });
+    }
+
+    selectRow(row, options) {
+      const { props: { member: { path } = {} } = {} } = row;
+      if (this.isSelected(path)) {
         return;
       }
 
@@ -408,13 +430,15 @@ define(function(require, exports, module) {
         }
       }
 
+      if (!options.preventAutoScroll) {
+        this._scrollIntoView(row, options);
+      }
+
       this.setState({
         ...this.state,
-        selected: row.id,
+        selected: path,
         active: null,
       });
-
-      row.scrollIntoView(scrollOptions);
     }
 
     activateRow(active) {