Skip to content

Commit

Permalink
fix(menu): Fix focus management bugs (TAB on menu item closes menu wi…
Browse files Browse the repository at this point in the history 
…thout restoring focus to anchor element, on menu open, menu respects focusState option (first item, last item, or list root).

PiperOrigin-RevId: 466956413
  • Loading branch information
@joyzhong @copybara-github
joyzhong authored and copybara-github committed Aug 11, 2022
1 parent 003579a commit 305b790
Show file tree
Hide file tree
Showing 7 changed files with 121 additions and 111 deletions.
4 changes: 4 additions & 0 deletions list/lib/list.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ export class List extends LitElement {
this.activeListItem.activate();
}

resetActiveListItem() {
this.activeListItem = null;
}

focusListRoot() {
this.listRoot.focus();
}
Expand Down
9 changes: 0 additions & 9 deletions menu/lib/adapter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
8 changes: 4 additions & 4 deletions menu/lib/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
31 changes: 1 addition & 30 deletions menu/lib/foundation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -26,7 +26,6 @@ export class MDCMenuFoundation {

private readonly adapter: MDCMenuAdapter;
private closeAnimationEndTimerId = 0;
private defaultFocusState = DefaultFocusState.LIST_ROOT;
private selectedIndex = -1;

/**
Expand All @@ -45,8 +44,6 @@ export class MDCMenuFoundation {
getElementIndex: () => -1,
notifySelected: () => undefined,
getMenuItemCount: () => 0,
focusItemAtIndex: () => undefined,
focusListRoot: () => undefined,
getSelectedSiblingOfItemAtIndex: () => -1,
isSelectableItemAtIndex: () => false,
};
Expand Down Expand Up @@ -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;
Expand Down
88 changes: 27 additions & 61 deletions menu/lib/menu.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<unknown> = null;

Expand All @@ -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;

Expand All @@ -130,16 +113,18 @@ 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}
@opened=${this.onOpened}
@keydown=${this.onKeydown}>
<md-list
aria-label="${ifDefined(this.ariaLabel)}"
.listTabIndex=${-1}
.listItemTagName=${this.getMenuItemTagName()}
role=${'menu'}
.listTabIndex=${
- 1}
.listItemTagName=${this.getMenuItemTagName()}
@action=${this.onAction}>
<slot></slot>
</md-list>
Expand Down Expand Up @@ -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) => {
Expand All @@ -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;

Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
// }
}
}
86 changes: 81 additions & 5 deletions menu/menu_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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<MdMenu> = {}) {
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<MdMenu> = {}) {
return html`
<div class="root" style="position: relative;">
<button @click=${setAnchorAndOpen}>
Open Menu
</button>
<md-menu .quick="${propsInit.quick ?? true}">
<md-menu .quick="${propsInit.quick ?? true}"
.defaultFocus="${propsInit.defaultFocus ?? 'LIST_ROOT'}">
<md-menu-item .headline=${'One'}></md-menu-item>
<md-menu-item .headline=${'Two'}></md-menu-item>
<md-menu-item .headline=${'Three'}></md-menu-item>
Expand Down
6 changes: 4 additions & 2 deletions menusurface/lib/menu-surface.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 305b790

Please sign in to comment.