From 305b790faa91e1cb7f1d80e5b7f3761acd38c302 Mon Sep 17 00:00:00 2001 From: Joy Zhong Date: Thu, 11 Aug 2022 08:11:54 -0700 Subject: [PATCH] fix(menu): Fix focus management bugs (TAB on menu item closes menu without restoring focus to anchor element, on menu open, menu respects focusState option (first item, last item, or list root). PiperOrigin-RevId: 466956413 --- list/lib/list.ts | 4 ++ menu/lib/adapter.ts | 9 ---- menu/lib/constants.ts | 8 +-- menu/lib/foundation.ts | 31 +----------- menu/lib/menu.ts | 88 ++++++++++----------------------- menu/menu_test.ts | 86 ++++++++++++++++++++++++++++++-- menusurface/lib/menu-surface.ts | 6 ++- 7 files changed, 121 insertions(+), 111 deletions(-) diff --git a/list/lib/list.ts b/list/lib/list.ts index 8395f39454..032aad5612 100644 --- a/list/lib/list.ts +++ b/list/lib/list.ts @@ -122,6 +122,10 @@ export class List extends LitElement { this.activeListItem.activate(); } + resetActiveListItem() { + this.activeListItem = null; + } + focusListRoot() { this.listRoot.focus(); } diff --git a/menu/lib/adapter.ts b/menu/lib/adapter.ts index 4b0f642785..92c407b6ad 100644 --- a/menu/lib/adapter.ts +++ b/menu/lib/adapter.ts @@ -64,15 +64,6 @@ export interface MDCMenuAdapter { /** @return Returns the menu item count. */ getMenuItemCount(): number; - /** - * Focuses the menu item at given index. - * @param index Index of the menu item that will be focused every time the menu opens. - */ - focusItemAtIndex(index: number): void; - - /** Focuses the list root element. */ - focusListRoot(): void; - /** * @return Returns selected list item index within the same selection group which is * a sibling of item at given `index`. diff --git a/menu/lib/constants.ts b/menu/lib/constants.ts index 570a32fde2..d7fef717a7 100644 --- a/menu/lib/constants.ts +++ b/menu/lib/constants.ts @@ -24,10 +24,10 @@ const numbers = { }; enum DefaultFocusState { - NONE = 0, - LIST_ROOT = 1, - FIRST_ITEM = 2, - LAST_ITEM = 3, + NONE = 'NONE', + LIST_ROOT = 'LIST_ROOT', + FIRST_ITEM = 'FIRST_ITEM', + LAST_ITEM = 'LAST_ITEM', } export {cssClasses, strings, numbers, DefaultFocusState}; diff --git a/menu/lib/foundation.ts b/menu/lib/foundation.ts index 2165b270ba..13b8804e81 100644 --- a/menu/lib/foundation.ts +++ b/menu/lib/foundation.ts @@ -7,7 +7,7 @@ import {MDCMenuSurfaceFoundation} from '../../menusurface/lib/foundation'; import {MDCMenuAdapter} from './adapter'; -import {cssClasses, DefaultFocusState, numbers, strings} from './constants'; +import {cssClasses, numbers, strings} from './constants'; const LIST_ITEM_DISABLED_CLASS = 'md3-list-item--disabled'; @@ -26,7 +26,6 @@ export class MDCMenuFoundation { private readonly adapter: MDCMenuAdapter; private closeAnimationEndTimerId = 0; - private defaultFocusState = DefaultFocusState.LIST_ROOT; private selectedIndex = -1; /** @@ -45,8 +44,6 @@ export class MDCMenuFoundation { getElementIndex: () => -1, notifySelected: () => undefined, getMenuItemCount: () => 0, - focusItemAtIndex: () => undefined, - focusListRoot: () => undefined, getSelectedSiblingOfItemAtIndex: () => -1, isSelectableItemAtIndex: () => false, }; @@ -97,32 +94,6 @@ export class MDCMenuFoundation { }, MDCMenuSurfaceFoundation.numbers.TRANSITION_CLOSE_DURATION); } - handleMenuSurfaceOpened() { - switch (this.defaultFocusState) { - case DefaultFocusState.FIRST_ITEM: - this.adapter.focusItemAtIndex(0); - break; - case DefaultFocusState.LAST_ITEM: - this.adapter.focusItemAtIndex(this.adapter.getMenuItemCount() - 1); - break; - case DefaultFocusState.NONE: - // Do nothing. - break; - default: - this.adapter.focusListRoot(); - break; - } - } - - /** - * Sets default focus state where the menu should focus every time when menu - * is opened. Focuses the list root (`DefaultFocusState.LIST_ROOT`) element by - * default. - */ - setDefaultFocusState(focusState: DefaultFocusState) { - this.defaultFocusState = focusState; - } - /** @return Index of the currently selected list item within the menu. */ getSelectedIndex() { return this.selectedIndex; diff --git a/menu/lib/menu.ts b/menu/lib/menu.ts index 9d710fd198..b570afe1ce 100644 --- a/menu/lib/menu.ts +++ b/menu/lib/menu.ts @@ -10,8 +10,6 @@ import '../../list/list'; import '../../menusurface/menu-surface'; -// TODO(b/239222919): remove compat dependencies -import {observer} from '@material/web/compat/base/observer'; import {ariaProperty} from '@material/web/decorators/aria-property'; import {html, LitElement} from 'lit'; import {property, query} from 'lit/decorators'; @@ -22,11 +20,9 @@ import {ListItem} from '../../list/lib/listitem/list-item'; import {Corner, MenuSurface} from '../../menusurface/lib/menu-surface'; import {MDCMenuAdapter} from './adapter'; -import {DefaultFocusState as DefaultFocusStateEnum} from './constants'; +import {DefaultFocusState} from './constants'; import {MDCMenuFoundation} from './foundation'; -export type DefaultFocusState = keyof typeof DefaultFocusStateEnum; - interface ActionDetail { item: ListItem; } @@ -75,15 +71,12 @@ export abstract class Menu extends LitElement { @property({type: Boolean}) flipMenuHorizontally = false; - @property({type: Boolean}) stayOpenOnBodyClick: boolean = false; + @property({type: Boolean}) stayOpenOnBodyClick = false; + + @property({type: Boolean}) skipRestoreFocus = false; @property({type: String}) - @observer(function(this: Menu, value: DefaultFocusState) { - if (this.mdcFoundation) { - this.mdcFoundation.setDefaultFocusState(DefaultFocusStateEnum[value]); - } - }) - defaultFocus: DefaultFocusState = 'LIST_ROOT'; + defaultFocus: DefaultFocusState = DefaultFocusState.LIST_ROOT; protected listUpdateComplete: null|Promise = null; @@ -96,16 +89,6 @@ export abstract class Menu extends LitElement { return this.listElementInternal; } - override click() { - if (this.mdcRoot) { - this.mdcRoot.focus(); - this.mdcRoot.click(); - return; - } - - super.click(); - } - get items(): ListItem[] { const listElement = this.listElement; @@ -130,6 +113,7 @@ export abstract class Menu extends LitElement { .fixed=${this.fixed} .fullwidth=${this.fullwidth} .flipMenuHorizontally=${this.flipMenuHorizontally} + .skipRestoreFocus=${this.skipRestoreFocus} ?stayOpenOnBodyClick=${this.stayOpenOnBodyClick} class="md3-menu md3-menu-surface" @closed=${this.onClosed} @@ -137,9 +121,10 @@ export abstract class Menu extends LitElement { @keydown=${this.onKeydown}> @@ -216,7 +201,8 @@ export abstract class Menu extends LitElement { }, elementContainsClass: (element, className) => element.classList.contains(className), - closeSurface: () => { + closeSurface: (skipRestoreFocus) => { + this.skipRestoreFocus = Boolean(skipRestoreFocus); this.open = false; }, getElementIndex: (element) => { @@ -236,22 +222,6 @@ export abstract class Menu extends LitElement { return listElement.items.length; }, - focusItemAtIndex: (index) => { - const listElement = this.listElement; - if (!listElement) { - return; - } - const element = listElement.items[index]; - - if (element) { - (element as HTMLElement).focus(); - } - }, - focusListRoot: () => { - if (this.listElement) { - this.listElement.focus(); - } - }, getSelectedSiblingOfItemAtIndex: (index) => { const listElement = this.listElement; @@ -313,10 +283,24 @@ export abstract class Menu extends LitElement { } protected onOpened() { + this.skipRestoreFocus = false; this.open = true; - if (this.mdcFoundation) { - this.mdcFoundation.handleMenuSurfaceOpened(); + this.listElement?.resetActiveListItem(); + switch (this.defaultFocus) { + case DefaultFocusState.FIRST_ITEM: + this.listElement?.activateFirstItem(); + break; + case DefaultFocusState.LAST_ITEM: + this.listElement?.activateLastItem(); + break; + case DefaultFocusState.NONE: + // Do nothing. + break; + case DefaultFocusState.LIST_ROOT: + default: + this.listElement?.focus(); + break; } } @@ -354,22 +338,4 @@ export abstract class Menu extends LitElement { show() { this.open = true; } - - getFocusedItemIndex() { - // TODO(b/240177152): Implement keyboard navigation support. - // const listElement = this.listElement; - // if (listElement) { - // return listElement.getFocusedItemIndex(); - // } - - return -1; - } - - focusItemAtIndex(index: number) { - // TODO(b/240177152): Implement keyboard navigation support. - // const listElement = this.listElement; - // if (listElement) { - // listElement.focusItemAtIndex(index); - // } - } } diff --git a/menu/menu_test.ts b/menu/menu_test.ts index 8074da91de..0ffee56e11 100644 --- a/menu/menu_test.ts +++ b/menu/menu_test.ts @@ -11,18 +11,17 @@ import {Environment} from '@material/web/testing/environment'; import {html} from 'lit'; import {MenuHarness} from './harness'; +import {DefaultFocusState} from './lib/constants'; import {MdMenu} from './menu'; describe('menu tests', () => { let menu: MdMenu; let harness: MenuHarness; + let anchor: HTMLButtonElement; const env = new Environment(); beforeEach(async () => { - const el = env.render(getMenuTemplate()); - menu = el.querySelector('md-menu')!; - harness = await new MenuHarness(menu); - await env.waitForStability(); + ({menu, harness, anchor} = await setUp(env)); }); describe('open/close', () => { @@ -78,15 +77,92 @@ describe('menu tests', () => { expect(menu.open).toBe(false); }); }); + + describe('focus management', () => { + it('with `defaultFocus=FIRST_ITEM`, focuses on first menu item on open', + async () => { + ({menu, harness} = + await setUp(env, {defaultFocus: DefaultFocusState.FIRST_ITEM})); + menu.show(); + await menu.updateComplete; + + const firstItem = harness.getItems()[0].element; + expect(document.activeElement).toBe(firstItem); + }); + + it('with `defaultFocus=LAST_ITEM`, focuses on last menu item on open', + async () => { + ({menu, harness} = + await setUp(env, {defaultFocus: DefaultFocusState.LAST_ITEM})); + menu.show(); + await menu.updateComplete; + + const items = harness.getItems(); + const lastItem = items[items.length - 1].element; + expect(document.activeElement).toBe(lastItem); + }); + + it('with `defaultFocus=LIST_ROOT`, focuses on menu root on open', + async () => { + ({menu} = + await setUp(env, {defaultFocus: DefaultFocusState.LIST_ROOT})); + menu.show(); + await menu.updateComplete; + + expect(document.activeElement).toBe(menu); + }); + + it('on TAB, closes the menu without restoring focus to the anchor', + async () => { + anchor.focus(); + expect(document.activeElement).toBe(anchor); + + menu.show(); + await menu.updateComplete; + expect(document.activeElement).not.toBe(anchor); + + const menuSurface = menu.renderRoot.querySelector('md-menu-surface')!; + menuSurface.dispatchEvent(new KeyboardEvent('keydown', {key: 'Tab'})); + expect(menu.open).toBe(false); + expect(document.activeElement).not.toBe(anchor); + }); + + it('on ESC, closes the menu and restores focus to the anchor', + async () => { + anchor.focus(); + expect(document.activeElement).toBe(anchor); + + menu.show(); + await menu.updateComplete; + expect(document.activeElement).not.toBe(anchor); + + const menuSurface = menu.renderRoot.querySelector('md-menu-surface')!; + menuSurface.mdcRoot.dispatchEvent( + new KeyboardEvent('keydown', {key: 'Escape'})); + expect(menu.open).toBe(false); + expect(document.activeElement).not.toBe(anchor); + }); + }); }); +async function setUp(env: Environment, propsInit: Partial = {}) { + const el = env.render(getMenuTemplate(propsInit)); + const menu = el.querySelector('md-menu')!; + const harness = await new MenuHarness(menu); + const anchor = el.querySelector('button')!; + await env.waitForStability(); + + return {menu, harness, anchor}; +} + function getMenuTemplate(propsInit: Partial = {}) { return html`
- + diff --git a/menusurface/lib/menu-surface.ts b/menusurface/lib/menu-surface.ts index 04887ea0cb..a0e7391cb3 100644 --- a/menusurface/lib/menu-surface.ts +++ b/menusurface/lib/menu-surface.ts @@ -101,13 +101,15 @@ export abstract class MenuSurface extends LitElement { this.mdcFoundation.open(); // wasOpen helps with first render (when it is `undefined`) perf } else if (wasOpen !== undefined) { - this.mdcFoundation.close(); + this.mdcFoundation.close(this.skipRestoreFocus); } } }) open = false; - @property({type: Boolean}) stayOpenOnBodyClick: boolean = false; + @property({type: Boolean}) stayOpenOnBodyClick = false; + + @property({type: Boolean}) skipRestoreFocus = false; @state() @observer(function(this: MenuSurface, value: CornerEnum) {