From 5f7680f76f0c6c5d81f4159fb1d103e8801f2b94 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Mon, 24 Jun 2024 09:15:04 -0700 Subject: [PATCH] fix(cdk/a11y): Make focus-trap behavior consistent across zoneful/zoneless (#29225) --- .../configurable-focus-trap-factory.ts | 7 ++++-- .../focus-trap/configurable-focus-trap.ts | 9 ++++--- .../event-listener-inert-strategy.spec.ts | 8 +++--- src/cdk/a11y/focus-trap/focus-trap.ts | 25 +++---------------- tools/public_api_guard/cdk/a11y.md | 2 +- 5 files changed, 20 insertions(+), 31 deletions(-) diff --git a/src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts b/src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts index f76e6f11a450..dcbad3e10dbc 100644 --- a/src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts +++ b/src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts @@ -7,12 +7,12 @@ */ import {DOCUMENT} from '@angular/common'; -import {Inject, Injectable, Optional, NgZone} from '@angular/core'; +import {Inject, Injectable, Injector, NgZone, Optional, inject} from '@angular/core'; import {InteractivityChecker} from '../interactivity-checker/interactivity-checker'; import {ConfigurableFocusTrap} from './configurable-focus-trap'; import {ConfigurableFocusTrapConfig} from './configurable-focus-trap-config'; -import {FOCUS_TRAP_INERT_STRATEGY, FocusTrapInertStrategy} from './focus-trap-inert-strategy'; import {EventListenerFocusTrapInertStrategy} from './event-listener-inert-strategy'; +import {FOCUS_TRAP_INERT_STRATEGY, FocusTrapInertStrategy} from './focus-trap-inert-strategy'; import {FocusTrapManager} from './focus-trap-manager'; /** Factory that allows easy instantiation of configurable focus traps. */ @@ -21,6 +21,8 @@ export class ConfigurableFocusTrapFactory { private _document: Document; private _inertStrategy: FocusTrapInertStrategy; + private readonly _injector = inject(Injector); + constructor( private _checker: InteractivityChecker, private _ngZone: NgZone, @@ -65,6 +67,7 @@ export class ConfigurableFocusTrapFactory { this._focusTrapManager, this._inertStrategy, configObject, + this._injector, ); } } diff --git a/src/cdk/a11y/focus-trap/configurable-focus-trap.ts b/src/cdk/a11y/focus-trap/configurable-focus-trap.ts index c77f444643d5..17200b619a0d 100644 --- a/src/cdk/a11y/focus-trap/configurable-focus-trap.ts +++ b/src/cdk/a11y/focus-trap/configurable-focus-trap.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {NgZone} from '@angular/core'; +import {Injector, NgZone} from '@angular/core'; import {InteractivityChecker} from '../interactivity-checker/interactivity-checker'; +import {ConfigurableFocusTrapConfig} from './configurable-focus-trap-config'; import {FocusTrap} from './focus-trap'; -import {FocusTrapManager, ManagedFocusTrap} from './focus-trap-manager'; import {FocusTrapInertStrategy} from './focus-trap-inert-strategy'; -import {ConfigurableFocusTrapConfig} from './configurable-focus-trap-config'; +import {FocusTrapManager, ManagedFocusTrap} from './focus-trap-manager'; /** * Class that allows for trapping focus within a DOM element. @@ -41,8 +41,9 @@ export class ConfigurableFocusTrap extends FocusTrap implements ManagedFocusTrap private _focusTrapManager: FocusTrapManager, private _inertStrategy: FocusTrapInertStrategy, config: ConfigurableFocusTrapConfig, + injector?: Injector, ) { - super(_element, _checker, _ngZone, _document, config.defer); + super(_element, _checker, _ngZone, _document, config.defer, injector); this._focusTrapManager.register(this); } diff --git a/src/cdk/a11y/focus-trap/event-listener-inert-strategy.spec.ts b/src/cdk/a11y/focus-trap/event-listener-inert-strategy.spec.ts index 5f28cf5ab768..5aadb345a618 100644 --- a/src/cdk/a11y/focus-trap/event-listener-inert-strategy.spec.ts +++ b/src/cdk/a11y/focus-trap/event-listener-inert-strategy.spec.ts @@ -1,10 +1,10 @@ -import {AfterViewInit, Component, ElementRef, Type, ViewChild, Provider} from '@angular/core'; -import {ComponentFixture, fakeAsync, flush, TestBed} from '@angular/core/testing'; +import {AfterViewInit, Component, ElementRef, Provider, Type, ViewChild} from '@angular/core'; +import {ComponentFixture, TestBed, fakeAsync, flush} from '@angular/core/testing'; import {patchElementFocus} from '../../testing/private'; import { A11yModule, - ConfigurableFocusTrapFactory, ConfigurableFocusTrap, + ConfigurableFocusTrapFactory, EventListenerFocusTrapInertStrategy, FOCUS_TRAP_INERT_STRATEGY, } from '../index'; @@ -31,6 +31,7 @@ describe('EventListenerFocusTrapInertStrategy', () => { const fixture = createComponent(SimpleFocusTrap, providers); const componentInstance = fixture.componentInstance; fixture.detectChanges(); + flush(); componentInstance.secondFocusableElement.nativeElement.focus(); flush(); @@ -43,6 +44,7 @@ describe('EventListenerFocusTrapInertStrategy', () => { it('should not intercept focus if it moved outside the trap and back in again', fakeAsync(() => { const fixture = createComponent(SimpleFocusTrap, providers); fixture.detectChanges(); + flush(); const {secondFocusableElement, outsideFocusableElement} = fixture.componentInstance; outsideFocusableElement.nativeElement.focus(); diff --git a/src/cdk/a11y/focus-trap/focus-trap.ts b/src/cdk/a11y/focus-trap/focus-trap.ts index 0628ae4e9a11..887beee6b53a 100644 --- a/src/cdk/a11y/focus-trap/focus-trap.ts +++ b/src/cdk/a11y/focus-trap/focus-trap.ts @@ -25,7 +25,6 @@ import { booleanAttribute, inject, } from '@angular/core'; -import {take} from 'rxjs/operators'; import {InteractivityChecker} from '../interactivity-checker/interactivity-checker'; /** @@ -359,27 +358,11 @@ export class FocusTrap { /** Executes a function when the zone is stable. */ private _executeOnStable(fn: () => any): void { - // TODO(mmalerba): Make this behave consistently across zonefull / zoneless. - if (!this._ngZone.isStable) { - // Subscribing `onStable` has slightly different behavior than `afterNextRender`. - // `afterNextRender` does not wait for state changes queued up in a Promise - // to avoid change after checked errors. In most cases we would consider this an - // acceptable behavior change, the dialog at least made its best effort to focus the - // first element. However, this is particularly problematic when combined with the - // current behavior of the mat-radio-group, which adjusts the tabindex of its child - // radios based on the selected value of the group. When the selected value is bound - // via `[(ngModel)]` it hits this "state change in a promise" edge-case and can wind up - // putting the focus on a radio button that is not supposed to be eligible to receive - // focus. For now, we side-step this whole sequence of events by continuing to use - // `onStable` in zonefull apps, but it should be noted that zoneless apps can still - // suffer from this issue. - this._ngZone.onStable.pipe(take(1)).subscribe(fn); + // TODO: remove this conditional when injector is required in the constructor. + if (this._injector) { + afterNextRender(fn, {injector: this._injector}); } else { - if (this._injector) { - afterNextRender(fn, {injector: this._injector}); - } else { - fn(); - } + setTimeout(fn); } } } diff --git a/tools/public_api_guard/cdk/a11y.md b/tools/public_api_guard/cdk/a11y.md index b449a42315f2..c967e2cc4a84 100644 --- a/tools/public_api_guard/cdk/a11y.md +++ b/tools/public_api_guard/cdk/a11y.md @@ -127,7 +127,7 @@ export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoC // @public export class ConfigurableFocusTrap extends FocusTrap implements ManagedFocusTrap { - constructor(_element: HTMLElement, _checker: InteractivityChecker, _ngZone: NgZone, _document: Document, _focusTrapManager: FocusTrapManager, _inertStrategy: FocusTrapInertStrategy, config: ConfigurableFocusTrapConfig); + constructor(_element: HTMLElement, _checker: InteractivityChecker, _ngZone: NgZone, _document: Document, _focusTrapManager: FocusTrapManager, _inertStrategy: FocusTrapInertStrategy, config: ConfigurableFocusTrapConfig, injector?: Injector); destroy(): void; _disable(): void; _enable(): void;