From dc4fbcf66fe1a9d598029bfc1b308650fa353f21 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 27 Feb 2022 20:51:33 +0100 Subject: [PATCH] fix(material/menu): focus lost if active item is removed (#14039) Fixes the user's focus being lost if the active item is destroyed while a menu is open. Fixes #14028. --- .../mdc-menu/menu.spec.ts | 37 +++++++++++++++++++ src/material/menu/menu-item.ts | 9 +++-- src/material/menu/menu.spec.ts | 36 ++++++++++++++++++ src/material/menu/menu.ts | 18 +++++++++ tools/public_api_guard/material/menu.md | 2 + 5 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/material-experimental/mdc-menu/menu.spec.ts b/src/material-experimental/mdc-menu/menu.spec.ts index 44b959838815..43ba01ee11b8 100644 --- a/src/material-experimental/mdc-menu/menu.spec.ts +++ b/src/material-experimental/mdc-menu/menu.spec.ts @@ -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]); @@ -3091,3 +3113,18 @@ class StaticAriaLabelledByMenu {} template: '', }) class StaticAriaDescribedbyMenu {} + +@Component({ + template: ` + + + + + `, +}) +class MenuWithRepeatedItems { + @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; + @ViewChild('triggerEl', {static: false}) triggerEl: ElementRef; + @ViewChild(MatMenu, {static: false}) menu: MatMenu; + items = ['One', 'Two', 'Three']; +} diff --git a/src/material/menu/menu-item.ts b/src/material/menu/menu-item.ts index 1756ffd5e38a..d9e903b7aa81 100644 --- a/src/material/menu/menu-item.ts +++ b/src/material/menu/menu-item.ts @@ -76,8 +76,7 @@ export class MatMenuItem _triggersSubmenu: boolean = false; /** - * @deprecated `document` parameter to be removed, `changeDetectorRef` and - * `focusMonitor` to become required. + * @deprecated `_document`, `changeDetectorRef` and `focusMonitor` to become required. * @breaking-change 12.0.0 */ constructor( @@ -90,7 +89,7 @@ export class MatMenuItem constructor( private _elementRef: ElementRef, - @Inject(DOCUMENT) _document?: any, + @Inject(DOCUMENT) private _document?: any, private _focusMonitor?: FocusMonitor, @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel, private _changeDetectorRef?: ChangeDetectorRef, @@ -176,4 +175,8 @@ export class MatMenuItem this._highlighted = isHighlighted; this._changeDetectorRef?.markForCheck(); } + + _hasFocus(): boolean { + return this._document && this._document.activeElement === this._getHostElement(); + } } diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index 8f7e52cf46c2..8dc06ec502f4 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -261,6 +261,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]); @@ -353,6 +373,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'); @@ -3075,3 +3096,18 @@ class StaticAriaLabelledByMenu {} template: '', }) class StaticAriaDescribedbyMenu {} + +@Component({ + template: ` + + + + + `, +}) +class MenuWithRepeatedItems { + @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; + @ViewChild('triggerEl', {static: false}) triggerEl: ElementRef; + @ViewChild(MatMenu, {static: false}) menu: MatMenu; + items = ['One', 'Two', 'Three']; +} diff --git a/src/material/menu/menu.ts b/src/material/menu/menu.ts index 1463c6d7a721..fcf0d2fa54d5 100644 --- a/src/material/menu/menu.ts +++ b/src/material/menu/menu.ts @@ -298,6 +298,24 @@ 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) => { + // 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; + + if (this._panelAnimationState === 'enter' && manager.activeItem?._hasFocus()) { + const items = itemsList.toArray(); + 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() { diff --git a/tools/public_api_guard/material/menu.md b/tools/public_api_guard/material/menu.md index 9ad5863217a9..68e33fb5a809 100644 --- a/tools/public_api_guard/material/menu.md +++ b/tools/public_api_guard/material/menu.md @@ -203,6 +203,8 @@ export class MatMenuItem extends _MatMenuItemBase implements FocusableOption, Ca getLabel(): string; _getTabIndex(): string; _handleMouseEnter(): void; + // (undocumented) + _hasFocus(): boolean; _highlighted: boolean; readonly _hovered: Subject; // (undocumented)