Skip to content

Commit

Permalink
refactor(cdk/a11y): make focus-trap zoneless compatible (#29031)
Browse files Browse the repository at this point in the history
  • Loading branch information
mmalerba committed May 17, 2024
1 parent 3e19768 commit c40cb80
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 37 deletions.
33 changes: 28 additions & 5 deletions src/cdk/a11y/focus-trap/focus-trap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ import {DOCUMENT} from '@angular/common';
import {
AfterContentInit,
Directive,
DoCheck,
ElementRef,
Inject,
Injectable,
Injector,
Input,
NgZone,
OnChanges,
OnDestroy,
DoCheck,
SimpleChanges,
OnChanges,
afterNextRender,
booleanAttribute,
inject,
} from '@angular/core';
Expand Down Expand Up @@ -62,6 +64,8 @@ export class FocusTrap {
readonly _ngZone: NgZone,
readonly _document: Document,
deferAnchors = false,
/** @breaking-change 20.0.0 param to become required */
readonly _injector?: Injector,
) {
if (!deferAnchors) {
this.attachAnchors();
Expand Down Expand Up @@ -355,10 +359,27 @@ export class FocusTrap {

/** Executes a function when the zone is stable. */
private _executeOnStable(fn: () => any): void {
if (this._ngZone.isStable) {
fn();
} else {
// 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);
} else {
if (this._injector) {
afterNextRender(fn, {injector: this._injector});
} else {
fn();
}
}
}
}
Expand All @@ -369,6 +390,7 @@ export class FocusTrap {
@Injectable({providedIn: 'root'})
export class FocusTrapFactory {
private _document: Document;
private _injector = inject(Injector);

constructor(
private _checker: InteractivityChecker,
Expand All @@ -392,6 +414,7 @@ export class FocusTrapFactory {
this._ngZone,
this._document,
deferCaptureElements,
this._injector,
);
}
}
Expand Down
33 changes: 4 additions & 29 deletions src/cdk/dialog/dialog-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@ import {
ElementRef,
EmbeddedViewRef,
Inject,
Injector,
NgZone,
OnDestroy,
Optional,
ViewChild,
ViewEncapsulation,
afterNextRender,
inject,
} from '@angular/core';
import {take} from 'rxjs/operators';
import {DialogConfig} from './dialog-config';

export function throwDialogContentAlreadyAttachedError() {
Expand Down Expand Up @@ -105,8 +102,6 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>

protected readonly _changeDetectorRef = inject(ChangeDetectorRef);

private _injector = inject(Injector);

constructor(
protected _elementRef: ElementRef,
protected _focusTrapFactory: FocusTrapFactory,
Expand Down Expand Up @@ -271,33 +266,13 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
break;
case true:
case 'first-tabbable':
const doFocus = () => {
const focusedSuccessfully = this._focusTrap?.focusInitialElement();
// If we weren't able to find a focusable element in the dialog, then focus the
// dialog container instead.
this._focusTrap?.focusInitialElementWhenReady().then(focusedSuccessfully => {
// If we weren't able to find a focusable element in the dialog, then focus the dialog
// container instead.
if (!focusedSuccessfully) {
this._focusDialogContainer();
}
};

// 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(doFocus);
} else {
afterNextRender(doFocus, {injector: this._injector});
}
});
break;
case 'first-heading':
this._focusByCssSelector('h1, h2, h3, h4, h5, h6, [role="heading"]');
Expand Down
4 changes: 3 additions & 1 deletion tools/public_api_guard/cdk/a11y.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ export type FocusOrigin = 'touch' | 'mouse' | 'keyboard' | 'program' | null;

// @public
export class FocusTrap {
constructor(_element: HTMLElement, _checker: InteractivityChecker, _ngZone: NgZone, _document: Document, deferAnchors?: boolean);
constructor(_element: HTMLElement, _checker: InteractivityChecker, _ngZone: NgZone, _document: Document, deferAnchors?: boolean,
_injector?: Injector | undefined);
attachAnchors(): boolean;
destroy(): void;
// (undocumented)
Expand All @@ -239,6 +240,7 @@ export class FocusTrap {
focusLastTabbableElement(options?: FocusOptions): boolean;
focusLastTabbableElementWhenReady(options?: FocusOptions): Promise<boolean>;
hasAttached(): boolean;
readonly _injector?: Injector | undefined;
// (undocumented)
readonly _ngZone: NgZone;
// (undocumented)
Expand Down
3 changes: 1 addition & 2 deletions tslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,7 @@
// Tests may need to verify behavior with zones.
"**/*.spec.ts",
// TODO(mmalerba): following files to be cleaned up and removed from this list:
"**/cdk/a11y/focus-trap/focus-trap.ts",
"**/cdk/dialog/dialog-container.ts"
"**/cdk/a11y/focus-trap/focus-trap.ts"
]
]
},
Expand Down

0 comments on commit c40cb80

Please sign in to comment.