Skip to content

Commit

Permalink
fix(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 #14028.
  • Loading branch information
crisbeto committed Jun 2, 2019
1 parent 2adf629 commit 65fcff4
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 3 deletions.
38 changes: 37 additions & 1 deletion src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,26 @@ describe('MatMenu', () => {
expect(document.activeElement).not.toBe(triggerEl);
}));

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 @@ -237,6 +257,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 @@ -2299,7 +2320,6 @@ class DynamicPanelMenu {
@ViewChild('two', {static: false}) secondMenu: MatMenu;
}


@Component({
template: `
<button [matMenuTriggerFor]="menu">Toggle menu</button>
Expand All @@ -2313,3 +2333,19 @@ class DynamicPanelMenu {
class MenuWithCheckboxItems {
@ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger;
}


@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) trigger: MatMenuTrigger;
@ViewChild('triggerEl') triggerEl: ElementRef<HTMLElement>;
@ViewChild(MatMenu) menu: MatMenu;
items = ['One', 'Two', 'Three'];
}
27 changes: 25 additions & 2 deletions src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {
OnInit,
} from '@angular/core';
import {merge, Observable, Subject, Subscription} from 'rxjs';
import {startWith, switchMap, take} from 'rxjs/operators';
import {startWith, switchMap, take, debounceTime} from 'rxjs/operators';
import {matMenuAnimations} from './menu-animations';
import {MatMenuContent} from './menu-content';
import {MenuPositionX, MenuPositionY} from './menu-positions';
Expand Down Expand Up @@ -244,11 +244,34 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
ngAfterContentInit() {
this._keyManager = new FocusKeyManager<MatMenuItem>(this._items).withWrap().withTypeAhead();
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.closed.emit('tab'));

// TODO(crisbeto): the `debounce` here should be removed since it's something
// that people have to flush in their tests. It'll be possible once we switch back
// to using a QueryList in #11720.
this._ngZone.runOutsideAngular(() => {
// 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.
this._itemChanges.pipe(debounceTime(50)).subscribe(items => {
const manager = this._keyManager;

if (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() {
this._tabSubscription.unsubscribe();
this.closed.complete();
this._itemChanges.complete();
}

/** Stream that emits whenever the hovered menu item changes. */
Expand Down Expand Up @@ -359,7 +382,7 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
removeItem(item: MatMenuItem) {
const index = this._items.indexOf(item);

if (this._items.indexOf(item) > -1) {
if (index > -1) {
this._items.splice(index, 1);
this._itemChanges.next(this._items);
}
Expand Down

0 comments on commit 65fcff4

Please sign in to comment.