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

fix(cdk/a11y): Make focus-trap behavior consistent across zoneful/zoneless #29225

Merged
merged 1 commit into from
Jun 24, 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
7 changes: 5 additions & 2 deletions src/cdk/a11y/focus-trap/configurable-focus-trap-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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,
Expand Down Expand Up @@ -65,6 +67,7 @@ export class ConfigurableFocusTrapFactory {
this._focusTrapManager,
this._inertStrategy,
configObject,
this._injector,
);
}
}
9 changes: 5 additions & 4 deletions src/cdk/a11y/focus-trap/configurable-focus-trap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -31,6 +31,7 @@ describe('EventListenerFocusTrapInertStrategy', () => {
const fixture = createComponent(SimpleFocusTrap, providers);
const componentInstance = fixture.componentInstance;
fixture.detectChanges();
flush();

componentInstance.secondFocusableElement.nativeElement.focus();
flush();
Expand All @@ -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();
Expand Down
25 changes: 4 additions & 21 deletions src/cdk/a11y/focus-trap/focus-trap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
booleanAttribute,
inject,
} from '@angular/core';
import {take} from 'rxjs/operators';
import {InteractivityChecker} from '../interactivity-checker/interactivity-checker';

/**
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tools/public_api_guard/cdk/a11y.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading