Skip to content

Commit

Permalink
fix(menu): escape not closing menus with submenus
Browse files Browse the repository at this point in the history
Ensure pressing escape while on a closed submenu anchor, closes the parent menu

Previously we were not letting a user close a menu when they were focused on any item with keepOpen = true. This included submenu anchors.

This change adds an escape (hehe) hatch to ensure that even on items with keepOpen = true, escape closes the menu. This is what most users expect and is what is the standard on menus around the web.

PiperOrigin-RevId: 587874713
  • Loading branch information
material-web-copybara authored and copybara-github committed Dec 5, 2023
1 parent aafea84 commit bd88880
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 2 deletions.
9 changes: 7 additions & 2 deletions menu/internal/controllers/menuItemController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,15 @@ export class MenuItemController implements ReactiveController {
}
}

if (this.host.keepOpen || event.defaultPrevented) return;
if (event.defaultPrevented) return;

// If the host has keepOpen = true we should ignore clicks & Space/Enter,
// however we always maintain the ability to close a menu with a explicit
// `escape` keypress.
const keyCode = event.code;
if (this.host.keepOpen && keyCode !== 'Escape') return;

if (!event.defaultPrevented && isClosableKey(keyCode)) {
if (isClosableKey(keyCode)) {
event.preventDefault();
this.host.dispatchEvent(
createDefaultCloseMenuEvent(this.host, {
Expand Down
49 changes: 49 additions & 0 deletions menu/menu_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@

// import 'jasmine'; (google3-only)
import './menu.js';
import './sub-menu.js';

import {html, render} from 'lit';

import {createTokenTests} from '../testing/tokens.js';

import {MenuItemHarness} from './harness.js';
import {MdMenu} from './menu.js';
import {MdMenuItem} from './menu-item.js';

Expand Down Expand Up @@ -60,6 +62,53 @@ describe('<md-menu>', () => {
expect(menu.open).toBeFalse();
expect(escapeKeydownEvent.defaultPrevented).toBeTrue();
});

// Regression test for b/314706578.
it('escape on submenu items closes menu', async () => {
render(
html`
<button>OpenMenu</button>
<md-menu quick>
<md-sub-menu hover-open-delay="0" hover-close-delay="0">
<md-menu-item id="submenu-item" slot="item">
<div slot="headline">Link Item 1</div>
</md-menu-item>
<md-menu quick slot="menu">
<md-menu-item slot="item">
<div slot="headline">Submenu Item 1</div>
</md-menu-item>
</md-menu>
</md-sub-menu>
</md-menu>
`,
root,
);

const button = root.querySelector('button')!;
const menu = root.querySelector('md-menu')!;
const submenuItemHarness = new MenuItemHarness(
menu.querySelector('#submenu-item')!,
);
menu.anchorElement = button;
menu.show();

expect(menu.open).toBeTrue();

const escapeKeydownEvent = new KeyboardEvent('keydown', {
key: 'Escape',
code: 'Escape',
bubbles: true,
composed: true,
cancelable: true,
});
const interactiveElement = await submenuItemHarness.getInteractiveElement();
interactiveElement.dispatchEvent(escapeKeydownEvent);

await menu.updateComplete;

expect(menu.open).toBeFalse();
expect(escapeKeydownEvent.defaultPrevented).toBeTrue();
});
});

describe('<md-menu-item>', () => {
Expand Down

0 comments on commit bd88880

Please sign in to comment.