From f0d6f06be844aeef882cbda7657612521c096119 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 19 Jul 2019 20:31:16 +0200 Subject: [PATCH] fix(menu): keyboard controls not working if all items are disabled Currently we depend on `focusFirstItem` to bring focus into the menu, however if all the items are disabled, focus will stay on the trigger and the user won't get feedback that something has happened. Furthermore, the keyboard shortcuts that are implemented in the menu won't work either, because the keyboard event listener is on the menu. These changes work around the issue by setting focus on the `role="menu"` if none of the items were focusable. Fixes #16565. --- .../mdc-menu/menu.spec.ts | 56 +++++++++++++++++++ src/material/menu/menu.spec.ts | 19 ++++++- src/material/menu/menu.ts | 26 ++++++++- 3 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/material-experimental/mdc-menu/menu.spec.ts b/src/material-experimental/mdc-menu/menu.spec.ts index 7c9a6c6a019b..046c2d68ccdb 100644 --- a/src/material-experimental/mdc-menu/menu.spec.ts +++ b/src/material-experimental/mdc-menu/menu.spec.ts @@ -762,6 +762,44 @@ describe('MatMenu', () => { flush(); })); + it('should respect the DOM order, rather than insertion order, when moving focus using ' + + 'the arrow keys', fakeAsync(() => { + let fixture = createComponent(SimpleMenuWithRepeater); + + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + tick(500); + + let menuPanel = document.querySelector('.mat-mdc-menu-panel')!; + let items = menuPanel.querySelectorAll('.mat-mdc-menu-panel [mat-menu-item]'); + + expect(document.activeElement).toBe(items[0], 'Expected first item to be focused on open'); + + // Add a new item after the first one. + fixture.componentInstance.items.splice(1, 0, {label: 'Calzone', disabled: false}); + fixture.detectChanges(); + + items = menuPanel.querySelectorAll('.mat-mdc-menu-panel [mat-menu-item]'); + dispatchKeyboardEvent(menuPanel, 'keydown', DOWN_ARROW); + fixture.detectChanges(); + tick(); + + expect(document.activeElement).toBe(items[1], 'Expected second item to be focused'); + flush(); + })); + + it('should focus the menu panel if all items are disabled', fakeAsync(() => { + const fixture = createComponent(SimpleMenuWithRepeater, [], [FakeIcon]); + fixture.componentInstance.items.forEach(item => item.disabled = true); + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + + expect(document.activeElement) + .toBe(overlayContainerElement.querySelector('.mat-mdc-menu-panel')); + })); + describe('lazy rendering', () => { it('should be able to render the menu content lazily', fakeAsync(() => { const fixture = createComponent(SimpleLazyMenu); @@ -2361,3 +2399,21 @@ class DynamicPanelMenu { class MenuWithCheckboxItems { @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; } + + +@Component({ + template: ` + + + + + ` +}) +class SimpleMenuWithRepeater { + @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; + @ViewChild(MatMenu, {static: false}) menu: MatMenu; + items = [{label: 'Pizza', disabled: false}, {label: 'Pasta', disabled: false}]; +} diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index c03b4c7b6522..f9d002512a11 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -762,6 +762,16 @@ describe('MatMenu', () => { flush(); })); + it('should focus the menu panel if all items are disabled', fakeAsync(() => { + const fixture = createComponent(SimpleMenuWithRepeater, [], [FakeIcon]); + fixture.componentInstance.items.forEach(item => item.disabled = true); + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + + expect(document.activeElement).toBe(overlayContainerElement.querySelector('.mat-menu-panel')); + })); + describe('lazy rendering', () => { it('should be able to render the menu content lazily', fakeAsync(() => { const fixture = createComponent(SimpleLazyMenu); @@ -882,7 +892,7 @@ describe('MatMenu', () => { expect(document.activeElement).toBe(items[0], 'Expected first item to be focused on open'); // Add a new item after the first one. - fixture.componentInstance.items.splice(1, 0, 'Calzone'); + fixture.componentInstance.items.splice(1, 0, {label: 'Calzone', disabled: false}); fixture.detectChanges(); items = menuPanel.querySelectorAll('.mat-menu-panel [mat-menu-item]'); @@ -2383,14 +2393,17 @@ class MenuWithCheckboxItems { template: ` - + ` }) class SimpleMenuWithRepeater { @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; @ViewChild(MatMenu, {static: false}) menu: MatMenu; - items = ['Pizza', 'Pasta']; + items = [{label: 'Pizza', disabled: false}, {label: 'Pasta', disabled: false}]; } @Component({ diff --git a/src/material/menu/menu.ts b/src/material/menu/menu.ts index 9c9b6cf6cc4f..4bd01a54e54d 100644 --- a/src/material/menu/menu.ts +++ b/src/material/menu/menu.ts @@ -320,13 +320,35 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel * @param origin Action from which the focus originated. Used to set the correct styling. */ focusFirstItem(origin: FocusOrigin = 'program'): void { + const manager = this._keyManager; + // When the content is rendered lazily, it takes a bit before the items are inside the DOM. if (this.lazyContent) { this._ngZone.onStable.asObservable() .pipe(take(1)) - .subscribe(() => this._keyManager.setFocusOrigin(origin).setFirstItemActive()); + .subscribe(() => manager.setFocusOrigin(origin).setFirstItemActive()); } else { - this._keyManager.setFocusOrigin(origin).setFirstItemActive(); + manager.setFocusOrigin(origin).setFirstItemActive(); + } + + // If there's no active item at this point, it means that all the items are disabled. + // Move focus to the menu panel so keyboard events like Escape still work. Also this will + // give _some_ feedback to screen readers. + if (!manager.activeItem && this._directDescendantItems.length) { + let element = this._directDescendantItems.first._getHostElement().parentElement; + + // Because the `mat-menu` is at the DOM insertion point, not inside the overlay, we don't + // have a nice way of getting a hold of the menu panel. We can't use a `ViewChild` either + // because the panel is inside an `ng-template`. We work around it by starting from one of + // the items and walking up the DOM. + while (element) { + if (element.getAttribute('role') === 'menu') { + element.focus(); + break; + } else { + element = element.parentElement; + } + } } }