Skip to content

Commit

Permalink
fix(menu): keyboard controls not working if all items are disabled (#…
Browse files Browse the repository at this point in the history
…16572)

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.
  • Loading branch information
crisbeto authored and jelbourn committed Aug 27, 2019
1 parent 399ed29 commit d3f63a3
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 5 deletions.
56 changes: 56 additions & 0 deletions src/material-experimental/mdc-menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -2361,3 +2399,21 @@ class DynamicPanelMenu {
class MenuWithCheckboxItems {
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
}


@Component({
template: `
<button [matMenuTriggerFor]="menu">Toggle menu</button>
<mat-menu #menu="matMenu">
<button
*ngFor="let item of items"
[disabled]="item.disabled"
mat-menu-item>{{item.label}}</button>
</mat-menu>
`
})
class SimpleMenuWithRepeater {
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
@ViewChild(MatMenu, {static: false}) menu: MatMenu;
items = [{label: 'Pizza', disabled: false}, {label: 'Pasta', disabled: false}];
}
19 changes: 16 additions & 3 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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]');
Expand Down Expand Up @@ -2383,14 +2393,17 @@ class MenuWithCheckboxItems {
template: `
<button [matMenuTriggerFor]="menu">Toggle menu</button>
<mat-menu #menu="matMenu">
<button *ngFor="let item of items" mat-menu-item>{{item}}</button>
<button
*ngFor="let item of items"
[disabled]="item.disabled"
mat-menu-item>{{item.label}}</button>
</mat-menu>
`
})
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({
Expand Down
26 changes: 24 additions & 2 deletions src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,35 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
* @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;
}
}
}
}

Expand Down

0 comments on commit d3f63a3

Please sign in to comment.