Skip to content

Commit

Permalink
refactor(material/menu): Remove use of zone onStable to focus items
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmalerba
mmalerba committed Mar 22, 2024
1 parent 108cce3 commit a71bbe7
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 93 deletions.
101 changes: 37 additions & 64 deletions src/material/menu/menu.spec.ts
View file
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
View file
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

0 comments on commit a71bbe7

Please sign in to comment.