diff --git a/devtools/client/accessibility/accessibility.css b/devtools/client/accessibility/accessibility.css index 565430388695..a27068c56431 100644 --- a/devtools/client/accessibility/accessibility.css +++ b/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 { - height: calc(100vh - var(--accessibility-toolbar-height)); +.treeTable thead, .treeTable tbody { + display: block; } -.treeTable > thead { - position: sticky; - top: 0; - /* Bug 1466806 - fix expander arrow for expanding treeview rows rendering over the - thead */ - z-index: 1; +.treeTable tr { + width: 100%; + display: table; } -.split-box:not(.horz) .treeTable { - /* To compenstate for 1px splitter between the tree and sidebar. */ - width: var(--accessibility-full-length-minus-splitter); +.treeTable tbody { + overflow-y: auto; } -.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; diff --git a/devtools/client/accessibility/components/AccessibilityRow.js b/devtools/client/accessibility/components/AccessibilityRow.js index f945dee185f5..2f180ba492e5 100644 --- a/devtools/client/accessibility/components/AccessibilityRow.js +++ b/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); } diff --git a/devtools/client/accessibility/components/AccessibilityTree.js b/devtools/client/accessibility/components/AccessibilityTree.js index 6f04f2fc3d2d..ee164d2de38c 100644 --- a/devtools/client/accessibility/components/AccessibilityTree.js +++ b/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 diff --git a/devtools/client/accessibility/test/browser/browser.ini b/devtools/client/accessibility/test/browser/browser.ini index 191fc0b42ca7..c50e237c6238 100644 --- a/devtools/client/accessibility/test/browser/browser.ini +++ b/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] diff --git a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit.js b/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit.js index 72ec710b5fe0..6124f7aeeb8e 100644 --- a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit.js +++ b/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, }], }, }]; diff --git a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_long.js b/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_long.js new file mode 100644 index 000000000000..b3f8a743e548 --- /dev/null +++ b/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 = "

header

"; + +const TEST_URI = ` + + + Accessibility Panel Test + + + ${header.repeat(20)} + +`; + +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."); diff --git a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_reset.js b/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_reset.js index 95749c06ad02..8eb8db59c4d4 100644 --- a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_reset.js +++ b/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"`, diff --git a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_toolbar.js b/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_toolbar.js index c8d37dc95408..7e4b7961f430 100644 --- a/devtools/client/accessibility/test/browser/browser_accessibility_tree_audit_toolbar.js +++ b/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"`, diff --git a/devtools/client/accessibility/test/browser/head.js b/devtools/client/accessibility/test/browser/head.js index 9db8325db9fc..74bceefbe9b1 100644 --- a/devtools/client/accessibility/test/browser/head.js +++ b/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) => - row.querySelector(".treeLabelCell").textContent === expected[i].role && - row.querySelector(".treeValueCell").textContent === expected[i].name && - compareBadges(row.querySelector(".badges"), expected[i].badges)), - "Wait for the right tree update."); + [...doc.querySelectorAll(".treeRow")].every((row, i) => { + const { role, name, badges, selected } = expected[i]; + return row.querySelector(".treeLabelCell").textContent === role && + row.querySelector(".treeValueCell").textContent === name && + compareBadges(row.querySelector(".badges"), badges) && + checkSelected(row, selected); + }), "Wait for the right tree update."); ok(hasExpectedStructure, "Tree structure is correct."); } diff --git a/devtools/client/shared/components/splitter/SplitBox.js b/devtools/client/shared/components/splitter/SplitBox.js index 69ac3ecf7604..0a177f3ba62f 100644 --- a/devtools/client/shared/components/splitter/SplitBox.js +++ b/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"]; diff --git a/devtools/client/shared/components/tree/TreeRow.js b/devtools/client/shared/components/tree/TreeRow.js index 9a2497d37171..38b2d494e134 100644 --- a/devtools/client/shared/components/tree/TreeRow.js +++ b/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; diff --git a/devtools/client/shared/components/tree/TreeView.js b/devtools/client/shared/components/tree/TreeView.js index 8f9b2ce5ac71..1c9f75797150 100644 --- a/devtools/client/shared/components/tree/TreeView.js +++ b/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 - // the table head. So we want to force the tree to scroll to the very top. - this.selectRow(firstRow, { - block: "end", - inline: "nearest", - }); + this.selectRow(firstRow, { alignTo: "top" }); } 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"}) { - row = findDOMNode(row); + _scrollIntoView(row, options = {}) { + if (!this.treeRef.current || !row) { + return; + } - if (this.state.selected === row.id) { - row.scrollIntoView(scrollOptions); + const { props: { member: { path } = {} } = {} } = row; + 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) {