Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(material/menu): Remove use of zone onStable to focus items #28663

Merged
merged 1 commit into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 37 additions & 64 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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');
});
Expand Down Expand Up @@ -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);
Expand Down
71 changes: 42 additions & 29 deletions src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -115,7 +119,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI
private _keyManager: FocusKeyManager<MatMenuItem>;
private _xPosition: MenuPositionX;
private _yPosition: MenuPositionY;
private _firstItemFocusSubscription?: Subscription;
private _firstItemFocusRef?: AfterRenderRef;
private _previousElevation: string;
private _elevationPrefix = 'mat-elevation-z';
private _baseElevation = 8;
Expand Down Expand Up @@ -267,6 +271,8 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI

readonly panelId = `mat-menu-panel-${menuPanelUid++}`;

private _injector = inject(Injector);

constructor(
elementRef: ElementRef<HTMLElement>,
ngZone: NgZone,
Expand All @@ -287,7 +293,11 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI

constructor(
private _elementRef: ElementRef<HTMLElement>,
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,
Expand Down Expand Up @@ -345,7 +355,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, 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. */
Expand Down Expand Up @@ -415,32 +425,35 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, 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},
);
}

/**
Expand Down
Loading