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(material/dialog): don't wait for animation before moving focus #24121

Merged
merged 2 commits into from
Mar 7, 2022
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: 0 additions & 7 deletions src/material-experimental/mdc-dialog/dialog.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PizzaMsg>, dialogRef2: MatDialogRef<PizzaMsg>;
dialogRef1 = dialog.open(PizzaMsg);
dialogRef2 = dialog.open(PizzaMsg);
expect(dialogRef1).toEqual(dialogRef2);
});
});

@Directive({selector: 'dir-with-view-container'})
Expand Down
4 changes: 4 additions & 0 deletions src/material-experimental/mdc-dialog/dialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export class MatDialog extends _MatDialogBase<MatDialogContainer> {
@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',
Expand Down
3 changes: 3 additions & 0 deletions src/material/dialog/dialog-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ export class MatDialogConfig<D = any> {
*/
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;

Expand Down
32 changes: 17 additions & 15 deletions src/material/dialog/dialog-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

/**
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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();
}
}
}
43 changes: 10 additions & 33 deletions src/material/dialog/dialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -80,9 +80,6 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
private readonly _afterOpenedAtThisLevel = new Subject<MatDialogRef<any>>();
private _ariaHiddenElements = new Map<Element, string | null>();
private _scrollStrategy: () => ScrollStrategy;
private _dialogAnimatingOpen = false;
private _animationStateSubscriptions: Subscription;
private _lastDialogRef: MatDialogRef<any>;

/** Keeps track of the currently-open dialogs. */
get openDialogs(): MatDialogRef<any>[] {
Expand Down Expand Up @@ -120,7 +117,11 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implemen
private _dialogRefConstructor: Type<MatDialogRef<any>>,
private _dialogContainerType: Type<C>,
private _dialogDataToken: InjectionToken<any>,
private _animationMode?: 'NoopAnimations' | 'BrowserAnimations',
/**
* @deprecated No longer used. To be removed.
* @breaking-change 14.0.0
*/
_animationMode?: 'NoopAnimations' | 'BrowserAnimations',
) {
this._scrollStrategy = scrollStrategy;
}
Expand Down Expand Up @@ -166,38 +167,14 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> 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<T, R>(
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) {
Expand Down Expand Up @@ -235,10 +212,6 @@ export abstract class _MatDialogBase<C extends _MatDialogContainerBase> 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();
}
}

/**
Expand Down Expand Up @@ -468,6 +441,10 @@ export class MatDialog extends _MatDialogBase<MatDialogContainer> {
@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',
Expand Down
9 changes: 7 additions & 2 deletions tools/public_api_guard/material/dialog.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ export function MAT_DIALOG_SCROLL_STRATEGY_PROVIDER_FACTORY(overlay: Overlay): (
// @public
export class MatDialog extends _MatDialogBase<MatDialogContainer> {
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<MatDialog, [null, null, { optional: true; }, { optional: true; }, null, { optional: true; skipSelf: true; }, null, { optional: true; }]>;
// (undocumented)
Expand All @@ -110,7 +111,8 @@ export const matDialogAnimations: {

// @public
export abstract class _MatDialogBase<C extends _MatDialogContainerBase> implements OnDestroy {
constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase<C> | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type<MatDialogRef<any>>, _dialogContainerType: Type<C>, _dialogDataToken: InjectionToken<any>, _animationMode?: "NoopAnimations" | "BrowserAnimations" | undefined);
constructor(_overlay: Overlay, _injector: Injector, _defaultOptions: MatDialogConfig | undefined, _parentDialog: _MatDialogBase<C> | undefined, _overlayContainer: OverlayContainer, scrollStrategy: any, _dialogRefConstructor: Type<MatDialogRef<any>>, _dialogContainerType: Type<C>, _dialogDataToken: InjectionToken<any>,
_animationMode?: 'NoopAnimations' | 'BrowserAnimations');
Splaktar marked this conversation as resolved.
Show resolved Hide resolved
readonly afterAllClosed: Observable<void>;
get afterOpened(): Subject<MatDialogRef<any>>;
closeAll(): void;
Expand Down Expand Up @@ -163,6 +165,7 @@ export class MatDialogConfig<D = any> {
closeOnNavigation?: boolean;
componentFactoryResolver?: ComponentFactoryResolver;
data?: D | null;
delayFocusTrap?: boolean;
direction?: Direction;
disableClose?: boolean;
hasBackdrop?: boolean;
Expand All @@ -183,6 +186,8 @@ export class MatDialogConfig<D = any> {

// @public
export class MatDialogContainer extends _MatDialogContainerBase {
// (undocumented)
_initializeWithAttachedContent(): void;
_onAnimationDone({ toState, totalTime }: AnimationEvent_2): void;
_onAnimationStart({ toState, totalTime }: AnimationEvent_2): void;
_startExitAnimation(): void;
Expand Down