Skip to content

Commit

Permalink
fix(cdk/a11y): Make focus-trap behavior consistent across zoneful/zon…
Browse files Browse the repository at this point in the history
…eless (#29225)
  • Loading branch information
mmalerba committed Jun 24, 2024
1 parent e2d70a8 commit 5f7680f
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 31 deletions.
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
8 changes: 5 additions & 3 deletions src/cdk/a11y/focus-trap/event-listener-inert-strategy.spec.ts
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

0 comments on commit 5f7680f

Please sign in to comment.