Skip to content

Commit

Permalink
fix(dialog): multiple dialogs not opening with disabled animations
Browse files Browse the repository at this point in the history 
With SHA 36f708c, second dialogs can be only opened if the first dialog finished animating.

Due to the recent update to Angular 4.3, the animation trigger callbacks are firing in a wrong order (done callback fires before start), which causes the _isAnimating property to be set to true while the animation already finished.

@crisbeto Not too happy how the tests ended up.

Fixes #6719
  • Loading branch information
@devversion
devversion committed Sep 30, 2017
1 parent 01622b1 commit b13ceca
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 80 deletions.
24 changes: 12 additions & 12 deletions src/lib/datepicker/datepicker.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
dispatchMouseEvent,
} from '@angular/cdk/testing';
import {Component, ViewChild} from '@angular/core';
import {async, ComponentFixture, inject, TestBed} from '@angular/core/testing';
import {async, ComponentFixture, inject, TestBed, fakeAsync, flush} from '@angular/core/testing';
import {FormControl, FormsModule, ReactiveFormsModule} from '@angular/forms';
import {
DEC,
Expand Down Expand Up @@ -166,7 +166,7 @@ describe('MatDatepicker', () => {
.toBe(true, 'Expected default ESCAPE action to be prevented.');
});

it('close should close dialog', () => {
it('close should close dialog', fakeAsync(() => {
testComponent.touch = true;
fixture.detectChanges();

Expand All @@ -177,13 +177,13 @@ describe('MatDatepicker', () => {

testComponent.datepicker.close();
fixture.detectChanges();
flush();

fixture.whenStable().then(() => {
expect(document.querySelector('mat-dialog-container')).toBeNull();
});
});
expect(document.querySelector('mat-dialog-container')).toBeNull();
flush();
}));

it('setting selected should update input and close calendar', () => {
it('setting selected should update input and close calendar', fakeAsync(() => {
testComponent.touch = true;
fixture.detectChanges();

Expand All @@ -196,12 +196,12 @@ describe('MatDatepicker', () => {
let cells = document.querySelectorAll('.mat-calendar-body-cell');
dispatchMouseEvent(cells[1], 'click');
fixture.detectChanges();
flush();

fixture.whenStable().then(() => {
expect(document.querySelector('mat-dialog-container')).toBeNull();
expect(testComponent.datepickerInput.value).toEqual(new Date(2020, JAN, 2));
});
});
expect(document.querySelector('mat-dialog-container')).toBeNull();
expect(testComponent.datepickerInput.value).toEqual(new Date(2020, JAN, 2));
flush();
}));

it('clicking the currently selected date should close the calendar ' +
'without firing selectedChanged', () => {
Expand Down
9 changes: 7 additions & 2 deletions src/lib/dialog/dialog-container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,13 @@ export class MatDialogContainer extends BasePortalHost {
this._restoreFocus();
}

this._animationStateChanged.emit(event);
this._isAnimating = false;
// Note: as of Angular 4.3, the animations module seems to fire the `start` callback before
// the end if animations are disabled. Make this call async to ensure that it still fires
// at the appropriate time.
Promise.resolve().then(() => {
this._animationStateChanged.emit(event);
this._isAnimating = false;
});
}

/** Callback, invoked when an animation on the host starts. */
Expand Down
136 changes: 70 additions & 66 deletions src/lib/dialog/dialog.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
inject,
TestBed,
tick,
flush,
} from '@angular/core/testing';
import {
ChangeDetectionStrategy,
Expand Down Expand Up @@ -165,21 +166,21 @@ describe('MatDialog', () => {
expect(dialogContainerElement.getAttribute('aria-describedby')).toBe('description-element');
});

it('should close a dialog and get back a result', async(() => {
it('should close a dialog and get back a result', fakeAsync(() => {
let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
let afterCloseCallback = jasmine.createSpy('afterClose callback');

dialogRef.afterClosed().subscribe(afterCloseCallback);
dialogRef.close('Charmander');
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(afterCloseCallback).toHaveBeenCalledWith('Charmander');
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
});
expect(afterCloseCallback).toHaveBeenCalledWith('Charmander');
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
flush();
}));

it('should close a dialog and get back a result before it is closed', async(() => {
it('should close a dialog and get back a result before it is closed', fakeAsync(() => {
const dialogRef = dialog.open(PizzaMsg, {viewContainerRef: testViewContainerRef});

// beforeClose should emit before dialog container is destroyed
Expand All @@ -191,24 +192,24 @@ describe('MatDialog', () => {
dialogRef.beforeClose().subscribe(beforeCloseHandler);
dialogRef.close('Bulbasaurus');
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(beforeCloseHandler).toHaveBeenCalledWith('Bulbasaurus');
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
});
expect(beforeCloseHandler).toHaveBeenCalledWith('Bulbasaurus');
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
flush();
}));

it('should close a dialog via the escape key', async(() => {
it('should close a dialog via the escape key', fakeAsync(() => {
dialog.open(PizzaMsg, {
viewContainerRef: testViewContainerRef
});

dispatchKeyboardEvent(document, 'keydown', ESCAPE);
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
});
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
flush();
}));

it('should close from a ViewContainerRef with OnPush change detection', fakeAsync(() => {
Expand Down Expand Up @@ -236,7 +237,7 @@ describe('MatDialog', () => {
.toBe(0, 'Expected no open dialogs.');
}));

it('should close when clicking on the overlay backdrop', async(() => {
it('should close when clicking on the overlay backdrop', fakeAsync(() => {
dialog.open(PizzaMsg, {
viewContainerRef: testViewContainerRef
});
Expand All @@ -247,13 +248,13 @@ describe('MatDialog', () => {

backdrop.click();
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeFalsy();
});
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeFalsy();
flush();
}));

it('should emit the backdropClick stream when clicking on the overlay backdrop', async(() => {
it('should emit the backdropClick stream when clicking on the overlay backdrop', fakeAsync(() => {
const dialogRef = dialog.open(PizzaMsg, {
viewContainerRef: testViewContainerRef
});
Expand All @@ -269,12 +270,12 @@ describe('MatDialog', () => {
expect(spy).toHaveBeenCalledTimes(1);

viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
// Additional clicks after the dialog has closed should not be emitted
backdrop.click();
expect(spy).toHaveBeenCalledTimes(1);
});
// Additional clicks after the dialog has closed should not be emitted
backdrop.click();
expect(spy).toHaveBeenCalledTimes(1);
flush();
}));

it('should notify the observers if a dialog has been opened', () => {
Expand All @@ -285,7 +286,7 @@ describe('MatDialog', () => {
});
});

it('should notify the observers if all open dialogs have finished closing', async(() => {
it('should notify the observers if all open dialogs have finished closing', fakeAsync(() => {
const ref1 = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
const ref2 = dialog.open(ContentElementDialog, { viewContainerRef: testViewContainerRef });
const spy = jasmine.createSpy('afterAllClosed spy');
Expand All @@ -294,14 +295,16 @@ describe('MatDialog', () => {

ref1.close();
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(spy).not.toHaveBeenCalled();
expect(spy).not.toHaveBeenCalled();

ref2.close();
viewContainerFixture.detectChanges();
viewContainerFixture.whenStable().then(() => expect(spy).toHaveBeenCalled());
});
ref2.close();
viewContainerFixture.detectChanges();
flush();

expect(spy).toHaveBeenCalled();
flush();
}));

it('should emit the afterAllClosed stream on subscribe if there are no open dialogs', () => {
Expand Down Expand Up @@ -441,8 +444,8 @@ describe('MatDialog', () => {

expect(dialogRef.componentInstance.directionality.value).toBe('rtl');
});

it('should close all of the dialogs', async(() => {
it('should close all of the dialogs', fakeAsync(() => {
dialog.open(PizzaMsg);
dialog.open(PizzaMsg);
dialog.open(PizzaMsg);
Expand All @@ -451,10 +454,10 @@ describe('MatDialog', () => {

dialog.closeAll();
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
});
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
flush();
}));

it('should set the proper animation states', () => {
Expand All @@ -469,32 +472,32 @@ describe('MatDialog', () => {
expect(dialogContainer._state).toBe('exit');
});

it('should close all dialogs when the user goes forwards/backwards in history', async(() => {
it('should close all dialogs when the user goes forwards/backwards in history', fakeAsync(() => {
dialog.open(PizzaMsg);
dialog.open(PizzaMsg);

expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(2);

mockLocation.simulateUrlPop('');
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
});
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
flush();
}));

it('should close all open dialogs when the location hash changes', async(() => {
it('should close all open dialogs when the location hash changes', fakeAsync(() => {
dialog.open(PizzaMsg);
dialog.open(PizzaMsg);

expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(2);

mockLocation.simulateHashChange('');
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
});
expect(overlayContainerElement.querySelectorAll('mat-dialog-container').length).toBe(0);
flush();
}));

it('should have the componentInstance available in the afterClosed callback', fakeAsync(() => {
Expand Down Expand Up @@ -543,17 +546,17 @@ describe('MatDialog', () => {
});
});

it('should not keep a reference to the component after the dialog is closed', async(() => {
it('should not keep a reference to the component after the dialog is closed', fakeAsync(() => {
let dialogRef = dialog.open(PizzaMsg);

expect(dialogRef.componentInstance).toBeTruthy();

dialogRef.close();
viewContainerFixture.detectChanges();
flush();

viewContainerFixture.whenStable().then(() => {
expect(dialogRef.componentInstance).toBeFalsy('Expected reference to have been cleared.');
});
expect(dialogRef.componentInstance).toBeFalsy('Expected reference to have been cleared.');
flush();
}));

it('should assign a unique id to each dialog', () => {
Expand Down Expand Up @@ -607,7 +610,7 @@ describe('MatDialog', () => {
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeTruthy();
});

it('should allow for the disableClose option to be updated while open', async(() => {
it('should allow for the disableClose option to be updated while open', fakeAsync(() => {
let dialogRef = dialog.open(PizzaMsg, {
disableClose: true,
viewContainerRef: testViewContainerRef
Expand All @@ -624,9 +627,10 @@ describe('MatDialog', () => {
backdrop.click();

viewContainerFixture.detectChanges();
viewContainerFixture.whenStable().then(() => {
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeFalsy();
});
flush();

expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeFalsy();
flush();
}));
});

Expand Down Expand Up @@ -887,7 +891,7 @@ describe('MatDialog with a parent MatDialog', () => {
});

it('should close dialogs opened by a parent when calling closeAll on a child MatDialog',
async(() => {
fakeAsync(() => {
parentDialog.open(PizzaMsg);
fixture.detectChanges();

Expand All @@ -896,15 +900,15 @@ describe('MatDialog with a parent MatDialog', () => {

childDialog.closeAll();
fixture.detectChanges();
flush();

fixture.whenStable().then(() => {
expect(overlayContainerElement.textContent!.trim())
.toBe('', 'Expected closeAll on child MatDialog to close dialog opened by parent');
});
expect(overlayContainerElement.textContent!.trim())
.toBe('', 'Expected closeAll on child MatDialog to close dialog opened by parent');
flush();
}));

it('should close dialogs opened by a child when calling closeAll on a parent MatDialog',
async(() => {
fakeAsync(() => {
childDialog.open(PizzaMsg);
fixture.detectChanges();

Expand All @@ -913,22 +917,22 @@ describe('MatDialog with a parent MatDialog', () => {

parentDialog.closeAll();
fixture.detectChanges();
flush();

fixture.whenStable().then(() => {
expect(overlayContainerElement.textContent!.trim())
.toBe('', 'Expected closeAll on parent MatDialog to close dialog opened by child');
});
expect(overlayContainerElement.textContent!.trim())
.toBe('', 'Expected closeAll on parent MatDialog to close dialog opened by child');
flush();
}));

it('should close the top dialog via the escape key', async(() => {
it('should close the top dialog via the escape key', fakeAsync(() => {
childDialog.open(PizzaMsg);

dispatchKeyboardEvent(document, 'keydown', ESCAPE);
fixture.detectChanges();
flush();

fixture.whenStable().then(() => {
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
});
expect(overlayContainerElement.querySelector('mat-dialog-container')).toBeNull();
flush();
}));
});

Expand Down

0 comments on commit b13ceca

Please sign in to comment.