Commit 6c3713e5 authored by John Lee's avatar John Lee Committed by Commit Bot

WebUI: Only focus first item in menus on keydown

Previously, CrMenuSelector relied on :focus-visible to determine if
focus was moved into it using a keyboard. This isn't a reliable way
because users can turn on an accessibility mode that forces
:focus-visible pseudoclasses on elements regardless of mouse or
keyboard input.

Bug: 1143200
Change-Id: I9b12138546ec4eaf31296147b808ad1a9f2faa4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505680Reviewed-by: default avatardpapad <dpapad@chromium.org>
Commit-Queue: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823246}
parent f80d4e4e
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
// found in the LICENSE file. // found in the LICENSE file.
import {CrMenuSelector} from 'chrome://resources/cr_elements/cr_menu_selector/cr_menu_selector.js'; import {CrMenuSelector} from 'chrome://resources/cr_elements/cr_menu_selector/cr_menu_selector.js';
import {FocusOutlineManager} from 'chrome://resources/js/cr/ui/focus_outline_manager.m.js';
import {getDeepActiveElement} from 'chrome://resources/js/util.m.js'; import {getDeepActiveElement} from 'chrome://resources/js/util.m.js';
import {keyDownOn} from 'chrome://resources/polymer/v3_0/iron-test-helpers/mock-interactions.js'; import {keyDownOn} from 'chrome://resources/polymer/v3_0/iron-test-helpers/mock-interactions.js';
...@@ -89,8 +90,8 @@ suite('CrMenuSelectorFocusTest', () => { ...@@ -89,8 +90,8 @@ suite('CrMenuSelectorFocusTest', () => {
document.body.appendChild(outsideElement); document.body.appendChild(outsideElement);
outsideElement.focus(); outsideElement.focus();
// Mock item as having been focused by keyboard. // Mock document as having been focused by keyboard.
element.children[2].matches = selector => selector === ':focus-visible'; FocusOutlineManager.forDocument(document).visible = true;
element.children[2].focus(); element.children[2].focus();
assertEquals(element.children[0], getDeepActiveElement()); assertEquals(element.children[0], getDeepActiveElement());
...@@ -101,8 +102,8 @@ suite('CrMenuSelectorFocusTest', () => { ...@@ -101,8 +102,8 @@ suite('CrMenuSelectorFocusTest', () => {
document.body.appendChild(outsideElement); document.body.appendChild(outsideElement);
outsideElement.focus(); outsideElement.focus();
// Mock item as not having been focused by keyboard. // Mock document as not having been focused by keyboard.
element.children[2].matches = selector => selector !== ':focus-visible'; FocusOutlineManager.forDocument(document).visible = false;
element.children[2].focus(); element.children[2].focus();
assertEquals(element.children[2], getDeepActiveElement()); assertEquals(element.children[2], getDeepActiveElement());
......
...@@ -15,5 +15,8 @@ js_type_check("closure_compile") { ...@@ -15,5 +15,8 @@ js_type_check("closure_compile") {
} }
js_library("cr_menu_selector") { js_library("cr_menu_selector") {
deps = [ "//ui/webui/resources/js:assert.m" ] deps = [
"//ui/webui/resources/js:assert.m",
"//ui/webui/resources/js/cr/ui:focus_outline_manager.m",
]
} }
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
// found in the LICENSE file. // found in the LICENSE file.
import {assert} from 'chrome://resources/js/assert.m.js'; import {assert} from 'chrome://resources/js/assert.m.js';
import {FocusOutlineManager} from 'chrome://resources/js/cr/ui/focus_outline_manager.m.js';
/** @extends {HTMLElement} */ /** @extends {HTMLElement} */
export class CrMenuSelector extends HTMLElement { export class CrMenuSelector extends HTMLElement {
...@@ -12,6 +13,10 @@ export class CrMenuSelector extends HTMLElement { ...@@ -12,6 +13,10 @@ export class CrMenuSelector extends HTMLElement {
constructor() { constructor() {
super(); super();
/** @private {!FocusOutlineManager} */
this.focusOutlineManager_;
this.addEventListener( this.addEventListener(
'focusin', e => this.onFocusin_(/** @type {!FocusEvent} */ (e))); 'focusin', e => this.onFocusin_(/** @type {!FocusEvent} */ (e)));
this.addEventListener( this.addEventListener(
...@@ -19,6 +24,7 @@ export class CrMenuSelector extends HTMLElement { ...@@ -19,6 +24,7 @@ export class CrMenuSelector extends HTMLElement {
} }
connectedCallback() { connectedCallback() {
this.focusOutlineManager_ = FocusOutlineManager.forDocument(document);
this.setAttribute('role', 'menu'); this.setAttribute('role', 'menu');
} }
...@@ -41,8 +47,7 @@ export class CrMenuSelector extends HTMLElement { ...@@ -41,8 +47,7 @@ export class CrMenuSelector extends HTMLElement {
// that is not within this menu, move the focus to the first menu item. This // that is not within this menu, move the focus to the first menu item. This
// ensures that the first menu item is always the first focused item when // ensures that the first menu item is always the first focused item when
// focusing into the menu. // focusing into the menu.
const focusMovedWithKeyboard = const focusMovedWithKeyboard = this.focusOutlineManager_.visible;
e.target && e.target.matches(':focus-visible');
const focusMovedFromOutside = e.relatedTarget && const focusMovedFromOutside = e.relatedTarget &&
!this.contains(/** @type {!HTMLElement} */ (e.relatedTarget)); !this.contains(/** @type {!HTMLElement} */ (e.relatedTarget));
if (focusMovedWithKeyboard && focusMovedFromOutside) { if (focusMovedWithKeyboard && focusMovedFromOutside) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment