Skip to content

Commit

Permalink
fix(menu): internal focus state out of sync if item is focused progra…
Browse files Browse the repository at this point in the history
…mmatically (#17762)

Fixes the internal key manager of the menu being out of sync if one of the menu items is focused via its `focus` method.

Fixes #17761.
  • Loading branch information
crisbeto authored and jelbourn committed Dec 4, 2019
1 parent fe151e6 commit af6c13f
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 0 deletions.
32 changes: 32 additions & 0 deletions src/material-experimental/mdc-menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,37 @@ describe('MDC-based MatMenu', () => {
flush();
}));

it('should sync the focus order when an item is focused programmatically', fakeAsync(() => {
const fixture = createComponent(SimpleMenuWithRepeater);

// Add some more items to work with.
for (let i = 0; i < 5; i++) {
fixture.componentInstance.items.push({label: `Extra ${i}`, disabled: false});
}

fixture.detectChanges();
fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
tick(500);

const menuPanel = document.querySelector('.mat-mdc-menu-panel')!;
const items = menuPanel.querySelectorAll('.mat-mdc-menu-panel [mat-menu-item]');

expect(document.activeElement).toBe(items[0], 'Expected first item to be focused on open');

fixture.componentInstance.itemInstances.toArray()[3].focus();
fixture.detectChanges();

expect(document.activeElement).toBe(items[3], 'Expected fourth item to be focused');

dispatchKeyboardEvent(menuPanel, 'keydown', DOWN_ARROW);
fixture.detectChanges();
tick();

expect(document.activeElement).toBe(items[4], 'Expected fifth 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);
Expand Down Expand Up @@ -2454,6 +2485,7 @@ class MenuWithCheckboxItems {
class SimpleMenuWithRepeater {
@ViewChild(MatMenuTrigger) trigger: MatMenuTrigger;
@ViewChild(MatMenu) menu: MatMenu;
@ViewChildren(MatMenuItem) itemInstances: QueryList<MatMenuItem>;
items = [{label: 'Pizza', disabled: false}, {label: 'Pasta', disabled: false}];
}

Expand Down
6 changes: 6 additions & 0 deletions src/material/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ export class MatMenuItem extends _MatMenuItemMixinBase
/** Stream that emits when the menu item is hovered. */
readonly _hovered: Subject<MatMenuItem> = new Subject<MatMenuItem>();

/** Stream that emits when the menu item is focused. */
readonly _focused = new Subject<MatMenuItem>();

/** Whether the menu item is highlighted. */
_highlighted: boolean = false;

Expand Down Expand Up @@ -102,6 +105,8 @@ export class MatMenuItem extends _MatMenuItemMixinBase
} else {
this._getHostElement().focus(options);
}

this._focused.next(this);
}

ngOnDestroy() {
Expand All @@ -114,6 +119,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase
}

this._hovered.complete();
this._focused.complete();
}

/** Used to set the `tabindex`. */
Expand Down
32 changes: 32 additions & 0 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,37 @@ describe('MatMenu', () => {
flush();
}));

it('should sync the focus order when an item is focused programmatically', fakeAsync(() => {
const fixture = createComponent(SimpleMenuWithRepeater);

// Add some more items to work with.
for (let i = 0; i < 5; i++) {
fixture.componentInstance.items.push({label: `Extra ${i}`, disabled: false});
}

fixture.detectChanges();
fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
tick(500);

const menuPanel = document.querySelector('.mat-menu-panel')!;
const items = menuPanel.querySelectorAll('.mat-menu-panel [mat-menu-item]');

expect(document.activeElement).toBe(items[0], 'Expected first item to be focused on open');

fixture.componentInstance.itemInstances.toArray()[3].focus();
fixture.detectChanges();

expect(document.activeElement).toBe(items[3], 'Expected fourth item to be focused');

dispatchKeyboardEvent(menuPanel, 'keydown', DOWN_ARROW);
fixture.detectChanges();
tick();

expect(document.activeElement).toBe(items[4], 'Expected fifth item to be focused');
flush();
}));

it('should open submenus when the menu is inside an OnPush component', fakeAsync(() => {
const fixture = createComponent(LazyMenuWithOnPush);
fixture.detectChanges();
Expand Down Expand Up @@ -2442,6 +2473,7 @@ class MenuWithCheckboxItems {
class SimpleMenuWithRepeater {
@ViewChild(MatMenuTrigger) trigger: MatMenuTrigger;
@ViewChild(MatMenu) menu: MatMenu;
@ViewChildren(MatMenuItem) itemInstances: QueryList<MatMenuItem>;
items = [{label: 'Pizza', disabled: false}, {label: 'Pasta', disabled: false}];
}

Expand Down
8 changes: 8 additions & 0 deletions src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,14 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
this._updateDirectDescendants();
this._keyManager = new FocusKeyManager(this._directDescendantItems).withWrap().withTypeAhead();
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.closed.emit('tab'));

// If a user manually (programatically) focuses a menu item, we need to reflect that focus
// change back to the key manager. Note that we don't need to unsubscribe here because _focused
// is internal and we know that it gets completed on destroy.
this._directDescendantItems.changes.pipe(
startWith(this._directDescendantItems),
switchMap(items => merge<MatMenuItem>(...items.map((item: MatMenuItem) => item._focused)))
).subscribe(focusedItem => this._keyManager.updateActiveItem(focusedItem));
}

ngOnDestroy() {
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/material/menu.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export interface MatMenuDefaultOptions {
}

export declare class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOption, CanDisable, CanDisableRipple, OnDestroy {
readonly _focused: Subject<MatMenuItem>;
_highlighted: boolean;
readonly _hovered: Subject<MatMenuItem>;
_parentMenu?: MatMenuPanel<MatMenuItem> | undefined;
Expand Down

0 comments on commit af6c13f

Please sign in to comment.