Skip to content

Commit

Permalink
fix(dialog): incorrect dialog state for close animation (#19034)
Browse files Browse the repository at this point in the history
The dialog currently sets its state to `CLOSED` if the
close animation is still in progress. This happens due to a
misplaced state update assignment.

(cherry picked from commit b612fc4)
  • Loading branch information
devversion authored and jelbourn committed Apr 20, 2020
1 parent 59a5786 commit fd0217d
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 19 deletions.
17 changes: 12 additions & 5 deletions src/material/dialog/dialog-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export class MatDialogRef<T, R = any> {
take(1)
).subscribe(() => {
clearTimeout(this._closeFallbackTimeout);
this._overlayRef.dispose();
this._finishDialogClose();
});

_overlayRef.detachments().subscribe(() => {
Expand Down Expand Up @@ -119,17 +119,15 @@ export class MatDialogRef<T, R = any> {
.subscribe(event => {
this._beforeClosed.next(dialogResult);
this._beforeClosed.complete();
this._state = MatDialogState.CLOSED;
this._overlayRef.detachBackdrop();

// The logic that disposes of the overlay depends on the exit animation completing, however
// it isn't guaranteed if the parent view is destroyed while it's running. Add a fallback
// timeout which will clean everything up if the animation hasn't fired within the specified
// amount of time plus 100ms. We don't need to run this outside the NgZone, because for the
// vast majority of cases the timeout will have been cleared before it has the chance to fire.
this._closeFallbackTimeout = setTimeout(() => {
this._overlayRef.dispose();
}, event.totalTime + 100);
this._closeFallbackTimeout = setTimeout(() => this._finishDialogClose(),
event.totalTime + 100);
});

this._containerInstance._startExitAnimation();
Expand Down Expand Up @@ -223,6 +221,15 @@ export class MatDialogRef<T, R = any> {
return this._state;
}

/**
* Finishes the dialog close by updating the state of the dialog
* and disposing the overlay.
*/
private _finishDialogClose() {
this._state = MatDialogState.CLOSED;
this._overlayRef.dispose();
}

/** Fetches the position strategy object from the overlay ref. */
private _getPositionStrategy(): GlobalPositionStrategy {
return this._overlayRef.getConfig().positionStrategy as GlobalPositionStrategy;
Expand Down
68 changes: 54 additions & 14 deletions src/material/dialog/dialog.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
ComponentFactoryResolver
} from '@angular/core';
import {By} from '@angular/platform-browser';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
import {BrowserAnimationsModule, NoopAnimationsModule} from '@angular/platform-browser/animations';
import {Location} from '@angular/common';
import {SpyLocation} from '@angular/common/testing';
import {Directionality} from '@angular/cdk/bidi';
Expand Down Expand Up @@ -777,19 +777,6 @@ describe('MatDialog', () => {
expect(resolver.resolveComponentFactory).toHaveBeenCalled();
}));

it('should return the current state of the dialog', fakeAsync(() => {
const dialogRef = dialog.open(PizzaMsg, {viewContainerRef: testViewContainerRef});

expect(dialogRef.getState()).toBe(MatDialogState.OPEN);
dialogRef.close();
viewContainerFixture.detectChanges();

expect(dialogRef.getState()).toBe(MatDialogState.CLOSING);
flush();

expect(dialogRef.getState()).toBe(MatDialogState.CLOSED);
}));

describe('passing in data', () => {
it('should be able to pass in data', () => {
let config = {
Expand Down Expand Up @@ -1579,6 +1566,59 @@ describe('MatDialog with default options', () => {
});


describe('MatDialog with animations enabled', () => {
let dialog: MatDialog;
let overlayContainer: OverlayContainer;

let testViewContainerRef: ViewContainerRef;
let viewContainerFixture: ComponentFixture<ComponentWithChildViewContainer>;

beforeEach(fakeAsync(() => {
TestBed.configureTestingModule({
imports: [MatDialogModule, DialogTestModule, BrowserAnimationsModule],
});

TestBed.compileComponents();
}));

beforeEach(inject([MatDialog, OverlayContainer], (d: MatDialog, oc: OverlayContainer) => {
dialog = d;
overlayContainer = oc;

viewContainerFixture = TestBed.createComponent(ComponentWithChildViewContainer);
viewContainerFixture.detectChanges();
testViewContainerRef = viewContainerFixture.componentInstance.childViewContainer;
}));

afterEach(() => {
overlayContainer.ngOnDestroy();
});

it('should return the current state of the dialog', fakeAsync(() => {
const dialogRef = dialog.open(PizzaMsg, {viewContainerRef: testViewContainerRef});
// Duration of the close animation in milliseconds. Extracted from the
// Angular animations definition of the dialog.
const dialogCloseDuration = 75;

expect(dialogRef.getState()).toBe(MatDialogState.OPEN);
dialogRef.close();
viewContainerFixture.detectChanges();

expect(dialogRef.getState()).toBe(MatDialogState.CLOSING);

// Ensure that the closing state is still set if half of the animation has
// passed by. The dialog state should be only set to `closed` when the dialog
// finished the close animation.
tick(dialogCloseDuration / 2);
expect(dialogRef.getState()).toBe(MatDialogState.CLOSING);

// Flush the remaining duration of the closing animation. We flush all other remaining
// tasks (e.g. the fallback close timeout) to avoid fakeAsync pending timer failures.
flush();
expect(dialogRef.getState()).toBe(MatDialogState.CLOSED);
}));
});

@Directive({selector: 'dir-with-view-container'})
class DirectiveWithViewContainer {
constructor(public viewContainerRef: ViewContainerRef) { }
Expand Down

0 comments on commit fd0217d

Please sign in to comment.