From 92863cca9676478a1b9ace37794e51ab7aa919fa Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 7 Mar 2022 08:09:52 +0100 Subject: [PATCH] fix(material/dialog): don't wait for animation before moving focus (#24121) * fix(material/dialog): don't wait for animation before moving focus For a long time we used to move focus into the dialog after the opening animation was finished, because it allowed for the content to settle in case something like an autocomplete needed to open. This approach has caused us accessibility issues in the past, because we've had to find ways to prevent the user from interacting with content behind the dialog while the opening animation is running. Initially we used to move focus to the dialog container before transferring it into the dialog, but that ended up interrupting the screen reader. In a later change we started returning the previous dialog ref if the same kind of dialog is opened while the animation is still running. The problem with this approach is that it doesn't allow for multiple dialog to be opened quickly (see #24037). These changes aim to address the root cause by moving focus immediately after the dialog content is attached. If this causes regressions with autocompletes inside dialogs, we now have APIs that allow users to customize the focus behavior. Fixes #24037. * fixup! fix(material/dialog): don't wait for animation before moving focus --- .../mdc-dialog/dialog.spec.ts | 7 --- .../mdc-dialog/dialog.ts | 4 ++ src/material/dialog/dialog-config.ts | 3 ++ src/material/dialog/dialog-container.ts | 32 +++++++------- src/material/dialog/dialog.ts | 43 +++++-------------- tools/public_api_guard/material/dialog.md | 9 +++- 6 files changed, 41 insertions(+), 57 deletions(-) diff --git a/src/material-experimental/mdc-dialog/dialog.spec.ts b/src/material-experimental/mdc-dialog/dialog.spec.ts index 6c51b2dd8763..35ef5ab3a453 100644 --- a/src/material-experimental/mdc-dialog/dialog.spec.ts +++ b/src/material-experimental/mdc-dialog/dialog.spec.ts @@ -2016,13 +2016,6 @@ describe('MDC-based MatDialog with animations enabled', () => { flush(); expect(dialogRef.getState()).toBe(MatDialogState.CLOSED); })); - - it("should return the previous dialogRef if the previous dialog hasn't finished animating open", () => { - let dialogRef1: MatDialogRef, dialogRef2: MatDialogRef; - dialogRef1 = dialog.open(PizzaMsg); - dialogRef2 = dialog.open(PizzaMsg); - expect(dialogRef1).toEqual(dialogRef2); - }); }); @Directive({selector: 'dir-with-view-container'}) diff --git a/src/material-experimental/mdc-dialog/dialog.ts b/src/material-experimental/mdc-dialog/dialog.ts index c65839812b2e..81fd8a559032 100644 --- a/src/material-experimental/mdc-dialog/dialog.ts +++ b/src/material-experimental/mdc-dialog/dialog.ts @@ -58,6 +58,10 @@ export class MatDialog extends _MatDialogBase { @Inject(MAT_DIALOG_SCROLL_STRATEGY) scrollStrategy: any, @Optional() @SkipSelf() parentDialog: MatDialog, overlayContainer: OverlayContainer, + /** + * @deprecated No longer used. To be removed. + * @breaking-change 14.0.0 + */ @Optional() @Inject(ANIMATION_MODULE_TYPE) animationMode?: 'NoopAnimations' | 'BrowserAnimations', diff --git a/src/material/dialog/dialog-config.ts b/src/material/dialog/dialog-config.ts index 2265e3a4e422..9fa06481bf1f 100644 --- a/src/material/dialog/dialog-config.ts +++ b/src/material/dialog/dialog-config.ts @@ -110,6 +110,9 @@ export class MatDialogConfig { */ restoreFocus?: boolean = true; + /** Whether to wait for the opening animation to finish before trapping focus. */ + delayFocusTrap?: boolean = true; + /** Scroll strategy to be used for the dialog. */ scrollStrategy?: ScrollStrategy; diff --git a/src/material/dialog/dialog-container.ts b/src/material/dialog/dialog-container.ts index 6dd376913045..162b3e94d9c8 100644 --- a/src/material/dialog/dialog-container.ts +++ b/src/material/dialog/dialog-container.ts @@ -110,10 +110,13 @@ export abstract class _MatDialogContainerBase extends BasePortalOutlet { /** Initializes the dialog container with the attached content. */ _initializeWithAttachedContent() { - this._setupFocusTrap(); + this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement); + // Save the previously focused element. This element will be re-focused // when the dialog closes. - this._capturePreviouslyFocusedElement(); + if (this._document) { + this._elementFocusedBeforeDialogWasOpened = _getFocusedElementPierceShadowDom(); + } } /** @@ -270,18 +273,6 @@ export abstract class _MatDialogContainerBase extends BasePortalOutlet { } } - /** Sets up the focus trap. */ - private _setupFocusTrap() { - this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement); - } - - /** Captures the element that was focused before the dialog was opened. */ - private _capturePreviouslyFocusedElement() { - if (this._document) { - this._elementFocusedBeforeDialogWasOpened = _getFocusedElementPierceShadowDom(); - } - } - /** Focuses the dialog container. */ private _focusDialogContainer() { // Note that there is no focus method when rendering on the server. @@ -333,7 +324,10 @@ export class MatDialogContainer extends _MatDialogContainerBase { /** Callback, invoked whenever an animation on the host completes. */ _onAnimationDone({toState, totalTime}: AnimationEvent) { if (toState === 'enter') { - this._trapFocus(); + if (this._config.delayFocusTrap) { + this._trapFocus(); + } + this._animationStateChanged.next({state: 'opened', totalTime}); } else if (toState === 'exit') { this._restoreFocus(); @@ -358,4 +352,12 @@ export class MatDialogContainer extends _MatDialogContainerBase { // view container is using OnPush change detection. this._changeDetectorRef.markForCheck(); } + + override _initializeWithAttachedContent() { + super._initializeWithAttachedContent(); + + if (!this._config.delayFocusTrap) { + this._trapFocus(); + } + } } diff --git a/src/material/dialog/dialog.ts b/src/material/dialog/dialog.ts index 0aad980a8071..4aee4b08aa60 100644 --- a/src/material/dialog/dialog.ts +++ b/src/material/dialog/dialog.ts @@ -30,7 +30,7 @@ import { TemplateRef, Type, } from '@angular/core'; -import {defer, Observable, of as observableOf, Subject, Subscription} from 'rxjs'; +import {defer, Observable, of as observableOf, Subject} from 'rxjs'; import {startWith} from 'rxjs/operators'; import {MatDialogConfig} from './dialog-config'; import {MatDialogContainer, _MatDialogContainerBase} from './dialog-container'; @@ -80,9 +80,6 @@ export abstract class _MatDialogBase implemen private readonly _afterOpenedAtThisLevel = new Subject>(); private _ariaHiddenElements = new Map(); private _scrollStrategy: () => ScrollStrategy; - private _dialogAnimatingOpen = false; - private _animationStateSubscriptions: Subscription; - private _lastDialogRef: MatDialogRef; /** Keeps track of the currently-open dialogs. */ get openDialogs(): MatDialogRef[] { @@ -120,7 +117,11 @@ export abstract class _MatDialogBase implemen private _dialogRefConstructor: Type>, private _dialogContainerType: Type, private _dialogDataToken: InjectionToken, - private _animationMode?: 'NoopAnimations' | 'BrowserAnimations', + /** + * @deprecated No longer used. To be removed. + * @breaking-change 14.0.0 + */ + _animationMode?: 'NoopAnimations' | 'BrowserAnimations', ) { this._scrollStrategy = scrollStrategy; } @@ -166,38 +167,14 @@ export abstract class _MatDialogBase implemen throw Error(`Dialog with id "${config.id}" exists already. The dialog id must be unique.`); } - // If there is a dialog that is currently animating open, return the MatDialogRef of that dialog - if (this._dialogAnimatingOpen) { - return this._lastDialogRef; - } - const overlayRef = this._createOverlay(config); const dialogContainer = this._attachDialogContainer(overlayRef, config); - if (this._animationMode !== 'NoopAnimations') { - const animationStateSubscription = dialogContainer._animationStateChanged.subscribe( - dialogAnimationEvent => { - if (dialogAnimationEvent.state === 'opening') { - this._dialogAnimatingOpen = true; - } - if (dialogAnimationEvent.state === 'opened') { - this._dialogAnimatingOpen = false; - animationStateSubscription.unsubscribe(); - } - }, - ); - if (!this._animationStateSubscriptions) { - this._animationStateSubscriptions = new Subscription(); - } - this._animationStateSubscriptions.add(animationStateSubscription); - } - const dialogRef = this._attachDialogContent( componentOrTemplateRef, dialogContainer, overlayRef, config, ); - this._lastDialogRef = dialogRef; // If this is the first dialog that we're opening, hide all the non-overlay content. if (!this.openDialogs.length) { @@ -235,10 +212,6 @@ export abstract class _MatDialogBase implemen this._closeDialogs(this._openDialogsAtThisLevel); this._afterAllClosedAtThisLevel.complete(); this._afterOpenedAtThisLevel.complete(); - // Clean up any subscriptions to dialogs that never finished opening. - if (this._animationStateSubscriptions) { - this._animationStateSubscriptions.unsubscribe(); - } } /** @@ -468,6 +441,10 @@ export class MatDialog extends _MatDialogBase { @Inject(MAT_DIALOG_SCROLL_STRATEGY) scrollStrategy: any, @Optional() @SkipSelf() parentDialog: MatDialog, overlayContainer: OverlayContainer, + /** + * @deprecated No longer used. To be removed. + * @breaking-change 14.0.0 + */ @Optional() @Inject(ANIMATION_MODULE_TYPE) animationMode?: 'NoopAnimations' | 'BrowserAnimations', diff --git a/tools/public_api_guard/material/dialog.md b/tools/public_api_guard/material/dialog.md index dbebade3802a..e8cbe9d65967 100644 --- a/tools/public_api_guard/material/dialog.md +++ b/tools/public_api_guard/material/dialog.md @@ -87,7 +87,8 @@ export function MAT_DIALOG_SCROLL_STRATEGY_PROVIDER_FACTORY(overlay: Overlay): ( // @public export class MatDialog extends _MatDialogBase { constructor(overlay: Overlay, injector: Injector, - location: Location_2, defaultOptions: MatDialogConfig, scrollStrategy: any, parentDialog: MatDialog, overlayContainer: OverlayContainer, animationMode?: 'NoopAnimations' | 'BrowserAnimations'); + location: Location_2, defaultOptions: MatDialogConfig, scrollStrategy: any, parentDialog: MatDialog, overlayContainer: OverlayContainer, + animationMode?: 'NoopAnimations' | 'BrowserAnimations'); // (undocumented) static ɵfac: i0.ɵɵFactoryDeclaration; // (undocumented) @@ -110,7 +111,8 @@ export const matDialogAnimations: { // @public export abstract class _MatDialogBase implements OnDestroy { - constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type>, _dialogContainerType: Type, _dialogDataToken: InjectionToken, _animationMode?: "NoopAnimations" | "BrowserAnimations" | undefined); + constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type>, _dialogContainerType: Type, _dialogDataToken: InjectionToken, + _animationMode?: 'NoopAnimations' | 'BrowserAnimations'); readonly afterAllClosed: Observable; get afterOpened(): Subject>; closeAll(): void; @@ -163,6 +165,7 @@ export class MatDialogConfig { closeOnNavigation?: boolean; componentFactoryResolver?: ComponentFactoryResolver; data?: D | null; + delayFocusTrap?: boolean; direction?: Direction; disableClose?: boolean; hasBackdrop?: boolean; @@ -183,6 +186,8 @@ export class MatDialogConfig { // @public export class MatDialogContainer extends _MatDialogContainerBase { + // (undocumented) + _initializeWithAttachedContent(): void; _onAnimationDone({ toState, totalTime }: AnimationEvent_2): void; _onAnimationStart({ toState, totalTime }: AnimationEvent_2): void; _startExitAnimation(): void;