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 angular#14028.
  • Loading branch information
crisbeto committed Feb 27, 2019
1 parent 740a2ee commit fcbf76c
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 3 deletions.
27 changes: 25 additions & 2 deletions src/lib/menu/menu-directive.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 {throwMatMenuInvalidPositionX, throwMatMenuInvalidPositionY} from './menu-errors';
Expand Down Expand Up @@ -259,11 +259,34 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI
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 @@ -374,7 +397,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI
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
38 changes: 37 additions & 1 deletion src/lib/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') secondMenu: MatMenu;
}


@Component({
template: `
<button [matMenuTriggerFor]="menu">Toggle menu</button>
Expand All @@ -2313,3 +2333,19 @@ class DynamicPanelMenu {
class MenuWithCheckboxItems {
@ViewChild(MatMenuTrigger) 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'];
}

0 comments on commit fcbf76c

Please sign in to comment.