Skip to content

Commit

Permalink
fix(material/menu): focus lost if active item is removed
Browse files Browse the repository at this point in the history
Fixes the user's focus being lost if the active item is destroyed while a menu is open.

Fixes angular#14028.
  • Loading branch information
crisbeto committed Feb 27, 2022
1 parent d011cae commit 1ea75b9
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 1 deletion.
37 changes: 37 additions & 0 deletions src/material-experimental/mdc-menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,28 @@ describe('MDC-based MatMenu', () => {
tick(500);
}));

it('should move focus to another item if the active item is destroyed', fakeAsync(() => {
const fixture = createComponent(MenuWithRepeatedItems, [], [FakeIcon]);
fixture.detectChanges();
const triggerEl = fixture.componentInstance.triggerEl.nativeElement;

triggerEl.click();
fixture.detectChanges();
tick(500);

const items = overlayContainerElement.querySelectorAll(
'.mat-mdc-menu-panel .mat-mdc-menu-item',
);

expect(document.activeElement).toBe(items[0]);

fixture.componentInstance.items.shift();
fixture.detectChanges();
tick(500);

expect(document.activeElement).toBe(items[1]);
}));

it('should be able to set a custom class on the backdrop', fakeAsync(() => {
const fixture = createComponent(SimpleMenu, [], [FakeIcon]);

Expand Down Expand Up @@ -3071,3 +3093,18 @@ class StaticAriaLabelledByMenu {}
template: '<mat-menu aria-describedby="some-element"></mat-menu>',
})
class StaticAriaDescribedbyMenu {}

@Component({
template: `
<button [matMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
<mat-menu #menu="matMenu">
<button *ngFor="let item of items" mat-menu-item>{{item}}</button>
</mat-menu>
`,
})
class MenuWithRepeatedItems {
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
@ViewChild('triggerEl', {static: false}) triggerEl: ElementRef<HTMLElement>;
@ViewChild(MatMenu, {static: false}) menu: MatMenu;
items = ['One', 'Two', 'Three'];
}
36 changes: 36 additions & 0 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,26 @@ describe('MatMenu', () => {
tick(500);
}));

it('should move focus to another item if the active item is destroyed', fakeAsync(() => {
const fixture = createComponent(MenuWithRepeatedItems, [], [FakeIcon]);
fixture.detectChanges();
const triggerEl = fixture.componentInstance.triggerEl.nativeElement;

triggerEl.click();
fixture.detectChanges();
tick(500);

const items = overlayContainerElement.querySelectorAll('.mat-menu-panel .mat-menu-item');

expect(document.activeElement).toBe(items[0]);

fixture.componentInstance.items.shift();
fixture.detectChanges();
tick(500);

expect(document.activeElement).toBe(items[1]);
}));

it('should be able to set a custom class on the backdrop', fakeAsync(() => {
const fixture = createComponent(SimpleMenu, [], [FakeIcon]);

Expand Down Expand Up @@ -351,6 +371,7 @@ describe('MatMenu', () => {
// Add 50 items to make the menu scrollable
fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
fixture.detectChanges();
tick(50);

const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
dispatchFakeEvent(triggerEl, 'mousedown');
Expand Down Expand Up @@ -3053,3 +3074,18 @@ class StaticAriaLabelledByMenu {}
template: '<mat-menu aria-describedby="some-element"></mat-menu>',
})
class StaticAriaDescribedbyMenu {}

@Component({
template: `
<button [matMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
<mat-menu #menu="matMenu">
<button *ngFor="let item of items" mat-menu-item>{{item}}</button>
</mat-menu>
`,
})
class MenuWithRepeatedItems {
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
@ViewChild('triggerEl', {static: false}) triggerEl: ElementRef<HTMLElement>;
@ViewChild(MatMenu, {static: false}) menu: MatMenu;
items = ['One', 'Two', 'Three'];
}
24 changes: 23 additions & 1 deletion src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {
ChangeDetectorRef,
} from '@angular/core';
import {merge, Observable, Subject, Subscription} from 'rxjs';
import {startWith, switchMap, take} from 'rxjs/operators';
import {startWith, switchMap, take, tap} from 'rxjs/operators';
import {matMenuAnimations} from './menu-animations';
import {MAT_MENU_CONTENT, MatMenuContent} from './menu-content';
import {MenuPositionX, MenuPositionY} from './menu-positions';
Expand Down Expand Up @@ -298,6 +298,28 @@ export class _MatMenuBase
switchMap(items => merge(...items.map((item: MatMenuItem) => item._focused))),
)
.subscribe(focusedItem => this._keyManager.updateActiveItem(focusedItem as MatMenuItem));

this._directDescendantItems.changes.subscribe((itemsList: QueryList<MatMenuItem>) => {
// Move focus to another item, if the active item is removed from the list.
// We need to debounce the callback, because multiple items might be removed
// in quick succession.
const manager = this._keyManager;
const items = itemsList.toArray();

if (
this._panelAnimationState === 'enter' &&
manager.activeItem &&
items.indexOf(manager.activeItem) === -1
) {
const index = Math.max(0, Math.min(items.length - 1, manager.activeItemIndex || 0));

if (items[index] && !items[index].disabled) {
manager.setActiveItem(index);
} else {
manager.setNextItemActive();
}
}
});
}

ngOnDestroy() {
Expand Down

0 comments on commit 1ea75b9

Please sign in to comment.