Skip to content

Commit c91ba96

Browse files
committed
fix(menu): focus lost if active item is removed
Fixes the user's focus being lost if the active item is destroyed while a menu is open. Fixes #14028.
1 parent 473d4c6 commit c91ba96

File tree

2 files changed

+59
-2
lines changed

2 files changed

+59
-2
lines changed

src/material/menu/menu.spec.ts

+36-1
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,26 @@ describe('MatMenu', () => {
200200
expect(document.activeElement).toBe(triggerEl);
201201
});
202202

203+
it('should move focus to another item if the active item is destroyed', fakeAsync(() => {
204+
const fixture = createComponent(MenuWithRepeatedItems, [], [FakeIcon]);
205+
fixture.detectChanges();
206+
const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
207+
208+
triggerEl.click();
209+
fixture.detectChanges();
210+
tick(500);
211+
212+
const items = overlayContainerElement.querySelectorAll('.mat-menu-panel .mat-menu-item');
213+
214+
expect(document.activeElement).toBe(items[0]);
215+
216+
fixture.componentInstance.items.shift();
217+
fixture.detectChanges();
218+
tick(500);
219+
220+
expect(document.activeElement).toBe(items[1]);
221+
}));
222+
203223
it('should be able to set a custom class on the backdrop', fakeAsync(() => {
204224
const fixture = createComponent(SimpleMenu, [], [FakeIcon]);
205225

@@ -257,6 +277,7 @@ describe('MatMenu', () => {
257277
// Add 50 items to make the menu scrollable
258278
fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
259279
fixture.detectChanges();
280+
tick(50);
260281

261282
const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
262283
dispatchFakeEvent(triggerEl, 'mousedown');
@@ -2392,7 +2413,6 @@ class DynamicPanelMenu {
23922413
@ViewChild('two', {static: false}) secondMenu: MatMenu;
23932414
}
23942415

2395-
23962416
@Component({
23972417
template: `
23982418
<button [matMenuTriggerFor]="menu">Toggle menu</button>
@@ -2447,3 +2467,18 @@ class LazyMenuWithOnPush {
24472467
@ViewChild('triggerEl', {static: false, read: ElementRef}) rootTrigger: ElementRef;
24482468
@ViewChild('menuItem', {static: false, read: ElementRef}) menuItemWithSubmenu: ElementRef;
24492469
}
2470+
2471+
@Component({
2472+
template: `
2473+
<button [matMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
2474+
<mat-menu #menu="matMenu">
2475+
<button *ngFor="let item of items" mat-menu-item>{{item}}</button>
2476+
</mat-menu>
2477+
`
2478+
})
2479+
class MenuWithRepeatedItems {
2480+
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
2481+
@ViewChild('triggerEl', {static: false}) triggerEl: ElementRef<HTMLElement>;
2482+
@ViewChild(MatMenu, {static: false}) menu: MatMenu;
2483+
items = ['One', 'Two', 'Three'];
2484+
}

src/material/menu/menu.ts

+23-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import {
4141
OnInit,
4242
} from '@angular/core';
4343
import {merge, Observable, Subject, Subscription} from 'rxjs';
44-
import {startWith, switchMap, take} from 'rxjs/operators';
44+
import {startWith, switchMap, take, debounceTime} from 'rxjs/operators';
4545
import {matMenuAnimations} from './menu-animations';
4646
import {MatMenuContent} from './menu-content';
4747
import {MenuPositionX, MenuPositionY} from './menu-positions';
@@ -250,6 +250,28 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
250250
this._updateDirectDescendants();
251251
this._keyManager = new FocusKeyManager(this._directDescendantItems).withWrap().withTypeAhead();
252252
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.closed.emit('tab'));
253+
254+
// TODO(crisbeto): the `debounce` here should be removed since it's something
255+
// that people have to flush in their tests. It'll be possible once we switch back
256+
// to using a QueryList in #11720.
257+
this._ngZone.runOutsideAngular(() => {
258+
// Move focus to another item, if the active item is removed from the list.
259+
// We need to debounce the callback, because multiple items might be removed
260+
// in quick succession.
261+
this._directDescendantItems.changes.pipe(debounceTime(50)).subscribe(items => {
262+
const manager = this._keyManager;
263+
264+
if (manager.activeItem && items.indexOf(manager.activeItem) === -1) {
265+
const index = Math.max(0, Math.min(items.length - 1, manager.activeItemIndex || 0));
266+
267+
if (items[index] && !items[index].disabled) {
268+
manager.setActiveItem(index);
269+
} else {
270+
manager.setNextItemActive();
271+
}
272+
}
273+
});
274+
});
253275
}
254276

255277
ngOnDestroy() {

0 commit comments

Comments
 (0)