From a71bbe7528fae3b3d8968c200654c82bd9433a53 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Wed, 28 Feb 2024 23:12:02 +0000 Subject: [PATCH] refactor(material/menu): Remove use of zone onStable to focus items --- src/material/menu/menu.spec.ts | 101 ++++++++++++--------------------- src/material/menu/menu.ts | 71 +++++++++++++---------- 2 files changed, 79 insertions(+), 93 deletions(-) diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index eee64ef2ea79..fa1bbaf138ca 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -1,55 +1,53 @@ -import {ComponentFixture, fakeAsync, flush, TestBed, tick} from '@angular/core/testing'; -import {By} from '@angular/platform-browser'; -import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {FocusMonitor} from '@angular/cdk/a11y'; +import {Direction, Directionality} from '@angular/cdk/bidi'; +import { + DOWN_ARROW, + END, + ENTER, + ESCAPE, + HOME, + LEFT_ARROW, + RIGHT_ARROW, + TAB, +} from '@angular/cdk/keycodes'; +import {Overlay, OverlayContainer} from '@angular/cdk/overlay'; +import {ScrollDispatcher, ViewportRuler} from '@angular/cdk/scrolling'; import { + ChangeDetectionStrategy, Component, ElementRef, EventEmitter, Input, Output, - NgZone, + Provider, + QueryList, TemplateRef, + Type, ViewChild, ViewChildren, - QueryList, - Type, - Provider, - ChangeDetectionStrategy, } from '@angular/core'; -import {Direction, Directionality} from '@angular/cdk/bidi'; -import {OverlayContainer, Overlay} from '@angular/cdk/overlay'; -import { - ESCAPE, - LEFT_ARROW, - RIGHT_ARROW, - DOWN_ARROW, - TAB, - HOME, - END, - ENTER, -} from '@angular/cdk/keycodes'; -import {MatMenu, MatMenuModule, MatMenuItem} from './index'; +import {ComponentFixture, fakeAsync, flush, TestBed, tick} from '@angular/core/testing'; import {MatRipple} from '@angular/material/core'; +import {By} from '@angular/platform-browser'; +import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {Subject} from 'rxjs'; import { - dispatchKeyboardEvent, - dispatchMouseEvent, - dispatchEvent, createKeyboardEvent, createMouseEvent, + dispatchEvent, dispatchFakeEvent, + dispatchKeyboardEvent, + dispatchMouseEvent, patchElementFocus, - MockNgZone, } from '../../cdk/testing/private'; -import {Subject} from 'rxjs'; -import {ScrollDispatcher, ViewportRuler} from '@angular/cdk/scrolling'; -import {FocusMonitor} from '@angular/cdk/a11y'; +import {MatMenu, MatMenuItem, MatMenuModule} from './index'; import { + MAT_MENU_DEFAULT_OPTIONS, MAT_MENU_SCROLL_STRATEGY, + MatMenuPanel, MatMenuTrigger, MenuPositionX, MenuPositionY, - MatMenuPanel, - MAT_MENU_DEFAULT_OPTIONS, } from './public-api'; const MENU_PANEL_TOP_PADDING = 8; @@ -772,27 +770,14 @@ describe('MDC-based MatMenu', () => { })); it('should set the proper origin when calling focusFirstItem after the opening sequence has started', () => { - let zone: MockNgZone; - const fixture = createComponent( - SimpleMenu, - [ - { - provide: NgZone, - useFactory: () => (zone = new MockNgZone()), - }, - ], - [FakeIcon], - ); + const fixture = createComponent(SimpleMenu, [], [FakeIcon]); fixture.detectChanges(); spyOn(fixture.componentInstance.items.first, 'focus').and.callThrough(); - const triggerEl = fixture.componentInstance.triggerEl.nativeElement; - - dispatchMouseEvent(triggerEl, 'mousedown'); - triggerEl.click(); - fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.componentInstance.menu.focusFirstItem('mouse'); fixture.componentInstance.menu.focusFirstItem('touch'); - zone!.onStable.next(); + fixture.detectChanges(); expect(fixture.componentInstance.items.first.focus).toHaveBeenCalledOnceWith('touch'); }); @@ -1304,30 +1289,18 @@ describe('MDC-based MatMenu', () => { expect(trigger.menuOpen).withContext('Expected menu to be closed').toBe(false); })); - it('should focus the first menu item when opening a lazy menu via keyboard', fakeAsync(() => { - let zone: MockNgZone; - let fixture = createComponent(SimpleLazyMenu, [ - { - provide: NgZone, - useFactory: () => (zone = new MockNgZone()), - }, - ]); - - fixture.detectChanges(); + it('should focus the first menu item when opening a lazy menu via keyboard', async () => { + const fixture = createComponent(SimpleLazyMenu); + fixture.autoDetectChanges(); // A click without a mousedown before it is considered a keyboard open. fixture.componentInstance.triggerEl.nativeElement.click(); - fixture.detectChanges(); - tick(500); - zone!.simulateZoneExit(); - - // Flush due to the additional tick that is necessary for the FocusMonitor. - flush(); + await fixture.whenStable(); const item = document.querySelector('.mat-mdc-menu-panel [mat-menu-item]')!; expect(document.activeElement).withContext('Expected first item to be focused').toBe(item); - })); + }); it('should be able to open the same menu with a different context', fakeAsync(() => { const fixture = createComponent(LazyMenuWithContext); diff --git a/src/material/menu/menu.ts b/src/material/menu/menu.ts index 943552867401..666226b7ddb2 100644 --- a/src/material/menu/menu.ts +++ b/src/material/menu/menu.ts @@ -27,6 +27,10 @@ import { OnInit, ChangeDetectorRef, booleanAttribute, + afterNextRender, + AfterRenderRef, + inject, + Injector, } from '@angular/core'; import {AnimationEvent} from '@angular/animations'; import {FocusKeyManager, FocusOrigin} from '@angular/cdk/a11y'; @@ -39,8 +43,8 @@ import { UP_ARROW, hasModifierKey, } from '@angular/cdk/keycodes'; -import {merge, Observable, Subject, Subscription} from 'rxjs'; -import {startWith, switchMap, take} from 'rxjs/operators'; +import {merge, Observable, Subject} from 'rxjs'; +import {startWith, switchMap} from 'rxjs/operators'; import {MatMenuItem} from './menu-item'; import {MatMenuPanel, MAT_MENU_PANEL} from './menu-panel'; import {MenuPositionX, MenuPositionY} from './menu-positions'; @@ -115,7 +119,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI private _keyManager: FocusKeyManager; private _xPosition: MenuPositionX; private _yPosition: MenuPositionY; - private _firstItemFocusSubscription?: Subscription; + private _firstItemFocusRef?: AfterRenderRef; private _previousElevation: string; private _elevationPrefix = 'mat-elevation-z'; private _baseElevation = 8; @@ -267,6 +271,8 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI readonly panelId = `mat-menu-panel-${menuPanelUid++}`; + private _injector = inject(Injector); + constructor( elementRef: ElementRef, ngZone: NgZone, @@ -287,7 +293,11 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI constructor( private _elementRef: ElementRef, - private _ngZone: NgZone, + /** + * @deprecated Unused param, will be removed. + * @breaking-change 19.0.0 + */ + _unusedNgZone: NgZone, @Inject(MAT_MENU_DEFAULT_OPTIONS) defaultOptions: MatMenuDefaultOptions, // @breaking-change 15.0.0 `_changeDetectorRef` to become a required parameter. private _changeDetectorRef?: ChangeDetectorRef, @@ -345,7 +355,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this._keyManager?.destroy(); this._directDescendantItems.destroy(); this.closed.complete(); - this._firstItemFocusSubscription?.unsubscribe(); + this._firstItemFocusRef?.destroy(); } /** Stream that emits whenever the hovered menu item changes. */ @@ -415,32 +425,35 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI * @param origin Action from which the focus originated. Used to set the correct styling. */ focusFirstItem(origin: FocusOrigin = 'program'): void { - // Wait for `onStable` to ensure iOS VoiceOver screen reader focuses the first item (#24735). - this._firstItemFocusSubscription?.unsubscribe(); - this._firstItemFocusSubscription = this._ngZone.onStable.pipe(take(1)).subscribe(() => { - let menuPanel: HTMLElement | null = null; - - if (this._directDescendantItems.length) { - // Because the `mat-menuPanel` is at the DOM insertion point, not inside the overlay, we don't - // have a nice way of getting a hold of the menuPanel panel. We can't use a `ViewChild` either - // because the panel is inside an `ng-template`. We work around it by starting from one of - // the items and walking up the DOM. - menuPanel = this._directDescendantItems.first!._getHostElement().closest('[role="menu"]'); - } - - // If an item in the menuPanel is already focused, avoid overriding the focus. - if (!menuPanel || !menuPanel.contains(document.activeElement)) { - const manager = this._keyManager; - manager.setFocusOrigin(origin).setFirstItemActive(); + // Wait for `afterNextRender` to ensure iOS VoiceOver screen reader focuses the first item (#24735). + this._firstItemFocusRef?.destroy(); + this._firstItemFocusRef = afterNextRender( + () => { + let menuPanel: HTMLElement | null = null; + + if (this._directDescendantItems.length) { + // Because the `mat-menuPanel` is at the DOM insertion point, not inside the overlay, we don't + // have a nice way of getting a hold of the menuPanel panel. We can't use a `ViewChild` either + // because the panel is inside an `ng-template`. We work around it by starting from one of + // the items and walking up the DOM. + menuPanel = this._directDescendantItems.first!._getHostElement().closest('[role="menu"]'); + } - // If there's no active item at this point, it means that all the items are disabled. - // Move focus to the menuPanel panel so keyboard events like Escape still work. Also this will - // give _some_ feedback to screen readers. - if (!manager.activeItem && menuPanel) { - menuPanel.focus(); + // If an item in the menuPanel is already focused, avoid overriding the focus. + if (!menuPanel || !menuPanel.contains(document.activeElement)) { + const manager = this._keyManager; + manager.setFocusOrigin(origin).setFirstItemActive(); + + // If there's no active item at this point, it means that all the items are disabled. + // Move focus to the menuPanel panel so keyboard events like Escape still work. Also this will + // give _some_ feedback to screen readers. + if (!manager.activeItem && menuPanel) { + menuPanel.focus(); + } } - } - }); + }, + {injector: this._injector}, + ); } /**