From 8a344769f65863cf057ec44bf5f1a03fce8148cd Mon Sep 17 00:00:00 2001 From: Jack Lynch Date: Wed, 11 Dec 2019 19:17:38 -0800 Subject: [PATCH] Improve keyboard/screenreader experience in DOM breakpoints Currently, there is no intuitive way to navigate the DOM breakpoints pane by keyboard. This change refactors the DOM breakpoints pane to use UI.ListControl to manage keyboard navigation, in response to feedback here [3] about code duplication. Using ListControl also makes it easier to manage an accessible description on breakpoint elements so that screen reader users are informed about the checked state of breakpoints and whether the page is currently paused on them. Focusing the list items, before/after: https://gyazo.com/5edc75de0c0e9d7f968f49e114cec324 https://gyazo.com/c3efc3322783f3505bc4dde3edabbf13 This CL breaks a web test, so [1] must be merged first to disable it. [2] Fixes and reenables it. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1893960 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1644461 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1644461/14/third_party/blink/renderer/devtools/front_end/browser_debugger/DOMBreakpointsSidebarPane.js#141 Bug: 963183 Change-Id: I41e2e8b73baa7ae3e6169163785e245493ab4ba7 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1889352 Commit-Queue: Jack Lynch Reviewed-by: Robert Paveza --- .../DOMBreakpointsSidebarPane.js | 228 +++++++++++++----- .../browser_debugger_strings.grdp | 9 + .../domBreakpointsSidebarPane.css | 4 + front_end/langpacks/shared_strings.grdp | 6 + front_end/ui/ListControl.js | 1 - front_end/ui/ui_strings.grdp | 6 - ...om-breakpoints-pane-a11y-test-expected.txt | 37 +-- .../sources/dom-breakpoints-pane-a11y-test.js | 1 - 8 files changed, 184 insertions(+), 108 deletions(-) diff --git a/front_end/browser_debugger/DOMBreakpointsSidebarPane.js b/front_end/browser_debugger/DOMBreakpointsSidebarPane.js index e888440781..0a0d43f7bd 100644 --- a/front_end/browser_debugger/DOMBreakpointsSidebarPane.js +++ b/front_end/browser_debugger/DOMBreakpointsSidebarPane.js @@ -30,18 +30,25 @@ /** * @implements {UI.ContextFlavorListener} + * @implements {UI.ListDelegate} */ export class DOMBreakpointsSidebarPane extends UI.VBox { constructor() { super(true); this.registerRequiredCSS('browser_debugger/domBreakpointsSidebarPane.css'); - this._listElement = this.contentElement.createChild('div', 'breakpoint-list hidden'); this._emptyElement = this.contentElement.createChild('div', 'gray-info-message'); this._emptyElement.textContent = Common.UIString('No breakpoints'); + /** @type {!UI.ListModel.} */ + this._breakpoints = new UI.ListModel(); + /** @type {!UI.ListControl.} */ + this._list = new UI.ListControl(this._breakpoints, this, UI.ListMode.NonViewport); + this.contentElement.appendChild(this._list.element); + this._list.element.classList.add('breakpoint-list', 'hidden'); + UI.ARIAUtils.markAsList(this._list.element); + UI.ARIAUtils.setAccessibleName(this._list.element, ls`DOM Breakpoints list`); + this._emptyElement.tabIndex = -1; - /** @type {!Map} */ - this._items = new Map(); SDK.targetManager.addModelListener( SDK.DOMDebuggerModel, SDK.DOMDebuggerModel.Events.DOMBreakpointAdded, this._breakpointAdded, this); SDK.targetManager.addModelListener( @@ -56,10 +63,114 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { } } - this._highlightedElement = null; + this._highlightedBreakpoint = null; this._update(); } + /** + * @override + * @param {!SDK.DOMDebuggerModel.DOMBreakpoint} item + * @return {!Element} + */ + createElementForItem(item) { + const element = createElementWithClass('div', 'breakpoint-entry'); + element.addEventListener('contextmenu', this._contextMenu.bind(this, item), true); + UI.ARIAUtils.markAsListitem(element); + element.tabIndex = this._list.selectedItem() === item ? 0 : -1; + + const checkboxLabel = UI.CheckboxLabel.create(/* title */ '', item.enabled); + const checkboxElement = checkboxLabel.checkboxElement; + checkboxElement.addEventListener('click', this._checkboxClicked.bind(this, item), false); + checkboxElement.tabIndex = -1; + UI.ARIAUtils.markAsHidden(checkboxLabel); + element.appendChild(checkboxLabel); + + const labelElement = createElementWithClass('div', 'dom-breakpoint'); + element.appendChild(labelElement); + element.addEventListener('keydown', event => { + if (event.key === ' ') { + checkboxElement.click(); + event.consume(true); + } + }); + + const description = createElement('div'); + const breakpointTypeLabel = BrowserDebugger.DOMBreakpointsSidebarPane.BreakpointTypeLabels.get(item.type); + description.textContent = breakpointTypeLabel; + const linkifiedNode = createElementWithClass('monospace'); + linkifiedNode.style.display = 'block'; + labelElement.appendChild(linkifiedNode); + Common.Linkifier.linkify(item.node, {preventKeyboardFocus: true}).then(linkified => { + linkifiedNode.appendChild(linkified); + UI.ARIAUtils.setAccessibleName(checkboxElement, ls`${breakpointTypeLabel}: ${linkified.deepTextContent()}`); + }); + + labelElement.appendChild(description); + + const checkedStateText = item.enabled ? ls`checked` : ls`unchecked`; + if (item === this._highlightedBreakpoint) { + element.classList.add('breakpoint-hit'); + UI.ARIAUtils.setDescription(element, ls`${checkedStateText} breakpoint hit`); + } else { + UI.ARIAUtils.setDescription(element, checkedStateText); + } + + + this._emptyElement.classList.add('hidden'); + this._list.element.classList.remove('hidden'); + + return element; + } + + /** + * @override + * @param {!SDK.DOMDebuggerModel.DOMBreakpoint} item + * @return {number} + */ + heightForItem(item) { + return 0; + } + + /** + * @override + * @param {!SDK.DOMDebuggerModel.DOMBreakpoint} item + * @return {boolean} + */ + isItemSelectable(item) { + return true; + } + + /** + * @override + * @param {?Element} fromElement + * @param {?Element} toElement + * @return {boolean} + */ + updateSelectedItemARIA(fromElement, toElement) { + return true; + } + + /** + * @override + * @param {?SDK.DOMDebuggerModel.DOMBreakpoint} from + * @param {?SDK.DOMDebuggerModel.DOMBreakpoint} to + * @param {?Element} fromElement + * @param {?Element} toElement + */ + selectedItemChanged(from, to, fromElement, toElement) { + if (fromElement) { + fromElement.tabIndex = -1; + } + + if (toElement) { + this.setDefaultFocusedElement(toElement); + toElement.tabIndex = 0; + if (this.hasFocus()) { + toElement.focus(); + } + } + } + /** * @param {!Common.Event} event */ @@ -71,10 +182,11 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { * @param {!Common.Event} event */ _breakpointToggled(event) { + const hadFocus = this.hasFocus(); const breakpoint = /** @type {!SDK.DOMDebuggerModel.DOMBreakpoint} */ (event.data); - const item = this._items.get(breakpoint); - if (item) { - item.checkbox.checked = breakpoint.enabled; + this._list.refreshItem(breakpoint); + if (hadFocus) { + this.focus(); } } @@ -82,17 +194,28 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { * @param {!Common.Event} event */ _breakpointsRemoved(event) { + const hadFocus = this.hasFocus(); const breakpoints = /** @type {!Array} */ (event.data); + let lastIndex = -1; for (const breakpoint of breakpoints) { - const item = this._items.get(breakpoint); - if (item) { - this._items.delete(breakpoint); - this._listElement.removeChild(item.element); + const index = this._breakpoints.indexOf(breakpoint); + if (index >= 0) { + this._breakpoints.remove(index); + lastIndex = index; } } - if (!this._listElement.firstChild) { + if (this._breakpoints.length === 0) { this._emptyElement.classList.remove('hidden'); - this._listElement.classList.add('hidden'); + this.setDefaultFocusedElement(this._emptyElement); + this._list.element.classList.add('hidden'); + } else if (lastIndex >= 0) { + const breakpointToSelect = this._breakpoints.at(lastIndex); + if (breakpointToSelect) { + this._list.selectItem(breakpointToSelect); + } + } + if (hadFocus) { + this.focus(); } } @@ -100,43 +223,18 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { * @param {!SDK.DOMDebuggerModel.DOMBreakpoint} breakpoint */ _addBreakpoint(breakpoint) { - const element = createElementWithClass('div', 'breakpoint-entry'); - element.addEventListener('contextmenu', this._contextMenu.bind(this, breakpoint), true); - - const checkboxLabel = UI.CheckboxLabel.create('', breakpoint.enabled); - const checkboxElement = checkboxLabel.checkboxElement; - checkboxElement.addEventListener('click', this._checkboxClicked.bind(this, breakpoint), false); - element.appendChild(checkboxLabel); - - const labelElement = createElementWithClass('div', 'dom-breakpoint'); - element.appendChild(labelElement); - - const description = createElement('div'); - const breakpointTypeLabel = BreakpointTypeLabels.get(breakpoint.type); - description.textContent = breakpointTypeLabel; - const linkifiedNode = createElementWithClass('monospace'); - linkifiedNode.style.display = 'block'; - labelElement.appendChild(linkifiedNode); - Common.Linkifier.linkify(breakpoint.node).then(linkified => { - linkifiedNode.appendChild(linkified); - UI.ARIAUtils.setAccessibleName(checkboxElement, ls`${breakpointTypeLabel}: ${linkified.deepTextContent()}`); - }); - labelElement.appendChild(description); - - const item = {breakpoint: breakpoint, element: element, checkbox: checkboxElement}; - element._item = item; - this._items.set(breakpoint, item); - - let currentElement = this._listElement.firstChild; - while (currentElement) { - if (currentElement._item && currentElement._item.breakpoint.type < breakpoint.type) { - break; + this._breakpoints.insertWithComparator(breakpoint, (breakpointA, breakpointB) => { + if (breakpointA.type > breakpointB.type) { + return -1; } - currentElement = currentElement.nextSibling; + if (breakpointA.type < breakpointB.type) { + return 1; + } + return 0; + }); + if (!this.hasFocus()) { + this._list.selectItem(this._breakpoints.at(0)); } - this._listElement.insertBefore(element, currentElement); - this._emptyElement.classList.add('hidden'); - this._listElement.classList.remove('hidden'); } /** @@ -145,6 +243,8 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { */ _contextMenu(breakpoint, event) { const contextMenu = new UI.ContextMenu(event); + contextMenu.defaultSection().appendItem( + ls`Reveal DOM node in Elements panel`, Common.Revealer.reveal.bind(null, breakpoint.node)); contextMenu.defaultSection().appendItem(Common.UIString('Remove breakpoint'), () => { breakpoint.domDebuggerModel.removeDOMBreakpoint(breakpoint.node, breakpoint.type); }); @@ -156,13 +256,10 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { /** * @param {!SDK.DOMDebuggerModel.DOMBreakpoint} breakpoint + * @param {!Event} event */ - _checkboxClicked(breakpoint) { - const item = this._items.get(breakpoint); - if (!item) { - return; - } - breakpoint.domDebuggerModel.toggleDOMBreakpoint(breakpoint, item.checkbox.checked); + _checkboxClicked(breakpoint, event) { + breakpoint.domDebuggerModel.toggleDOMBreakpoint(breakpoint, event.target.checked); } /** @@ -175,13 +272,15 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { _update() { const details = UI.context.flavor(SDK.DebuggerPausedDetails); + if (this._highlightedBreakpoint) { + const oldHighlightedBreakpoint = this._highlightedBreakpoint; + delete this._highlightedBreakpoint; + this._list.refreshItem(oldHighlightedBreakpoint); + } if (!details || !details.auxData || details.reason !== SDK.DebuggerModel.BreakReason.DOM) { - if (this._highlightedElement) { - this._highlightedElement.classList.remove('breakpoint-hit'); - delete this._highlightedElement; - } return; } + const domDebuggerModel = details.debuggerModel.target().model(SDK.DOMDebuggerModel); if (!domDebuggerModel) { return; @@ -191,18 +290,15 @@ export class DOMBreakpointsSidebarPane extends UI.VBox { return; } - let element = null; - for (const item of this._items.values()) { - if (item.breakpoint.node === data.node && item.breakpoint.type === data.type) { - element = item.element; + for (const breakpoint of this._breakpoints) { + if (breakpoint.node === data.node && breakpoint.type === data.type) { + this._highlightedBreakpoint = breakpoint; } } - if (!element) { - return; + if (this._highlightedBreakpoint) { + this._list.refreshItem(this._highlightedBreakpoint); } UI.viewManager.showView('sources.domBreakpoints'); - element.classList.add('breakpoint-hit'); - this._highlightedElement = element; } } diff --git a/front_end/browser_debugger/browser_debugger_strings.grdp b/front_end/browser_debugger/browser_debugger_strings.grdp index 7cd1d3edd8..c4cf304f0a 100644 --- a/front_end/browser_debugger/browser_debugger_strings.grdp +++ b/front_end/browser_debugger/browser_debugger_strings.grdp @@ -15,6 +15,9 @@ breakpoint hit + + Reveal DOM node in Elements panel + Break on @@ -27,6 +30,9 @@ Refresh global listeners + + $1schecked breakpoint hit + Any XHR or fetch @@ -39,6 +45,9 @@ Global Listeners + + DOM Breakpoints list + Event Listener Breakpoints diff --git a/front_end/browser_debugger/domBreakpointsSidebarPane.css b/front_end/browser_debugger/domBreakpointsSidebarPane.css index d5622c2f12..f9a34b21e8 100644 --- a/front_end/browser_debugger/domBreakpointsSidebarPane.css +++ b/front_end/browser_debugger/domBreakpointsSidebarPane.css @@ -21,6 +21,10 @@ padding: 2px 0; } +.breakpoint-entry[data-keyboard-focus="true"] { + background-color: var(--focus-bg-color); +} + .breakpoint-list .breakpoint-entry:hover { background-color: #eee; } diff --git a/front_end/langpacks/shared_strings.grdp b/front_end/langpacks/shared_strings.grdp index 1c90a1d469..3b2e65912d 100644 --- a/front_end/langpacks/shared_strings.grdp +++ b/front_end/langpacks/shared_strings.grdp @@ -124,6 +124,9 @@ JS Heap + + checked + N/A @@ -427,6 +430,9 @@ Collapse children + + unchecked + Closure diff --git a/front_end/ui/ListControl.js b/front_end/ui/ListControl.js index b54ff8d4ec..19b3603e48 100644 --- a/front_end/ui/ListControl.js +++ b/front_end/ui/ListControl.js @@ -489,7 +489,6 @@ export default class ListControl { const newItem = this._selectedItem; const newElement = this._selectedIndex !== -1 ? this._elementAtIndex(index) : null; - UI.ARIAUtils.setActiveDescendant(this.element, newElement); this._delegate.selectedItemChanged(oldItem, newItem, /** @type {?Element} */ (oldElement), newElement); if (!this._delegate.updateSelectedItemARIA(/** @type {?Element} */ (oldElement), newElement)) { if (oldElement) { diff --git a/front_end/ui/ui_strings.grdp b/front_end/ui/ui_strings.grdp index 8c0421e48e..d5937847f8 100644 --- a/front_end/ui/ui_strings.grdp +++ b/front_end/ui/ui_strings.grdp @@ -63,9 +63,6 @@ Async Call - - checked - Open in new tab @@ -240,9 +237,6 @@ Once page is reloaded, DevTools will automatically reconnect. - - unchecked - more diff --git a/test/webtests/http/tests/devtools/a11y-axe-core/sources/dom-breakpoints-pane-a11y-test-expected.txt b/test/webtests/http/tests/devtools/a11y-axe-core/sources/dom-breakpoints-pane-a11y-test-expected.txt index 65648a94fc..cc84d71b0f 100644 --- a/test/webtests/http/tests/devtools/a11y-axe-core/sources/dom-breakpoints-pane-a11y-test-expected.txt +++ b/test/webtests/http/tests/devtools/a11y-axe-core/sources/dom-breakpoints-pane-a11y-test-expected.txt @@ -1,39 +1,8 @@ Testing accessibility in the DOM breakpoints pane. Setting DOM breakpoints. -DOM breakpoints container text content: DOM BreakpointsSubtree modifiedNode removedNo breakpoints -DOM breakpoints pane text content: Subtree modifiedNode removedNo breakpoints +DOM breakpoints container text content: DOM BreakpointsNo breakpointsSubtree modifiedcheckedNode removedunchecked +DOM breakpoints pane text content: No breakpointsSubtree modifiedcheckedNode removedunchecked Running the axe-core linter on the DOM breakpoints pane. -aXe violations: [ - { - "ruleDescription": "Ensures every form element has a label", - "helpUrl": "https://dequeuniversity.com/rules/axe/3.3/label?application=axeAPI", - "ruleId": "label", - "impact": "critical", - "failedNodes": [ - { - "target": [ - [ - ".flex-none.flex-auto.vbox:nth-child(7) > .flex-auto.vbox", - ".breakpoint-entry:nth-child(1) > span[is=\"dt-checkbox\"]", - "#ui-checkbox-label9" - ] - ], - "html": "", - "failureSummary": "Fix any of the following:\n aria-label attribute does not exist or is empty\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty\n Form element does not have an implicit (wrapped)