Skip to content

fix(dialog): multiple dialogs not opening with disabled animations #6736

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix(dialog): multiple dialogs not opening with disabled animations #6736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 12 additions & 12 deletions src/lib/datepicker/datepicker.spec.ts
View file
Open in desktop
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
Open in desktop
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(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for this fix? The current tests are using the NoopAnimationsModule and they didn't catch the issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into a way to reproduce that issue inside of the TestBed, but it looks like the animation callbacks fire correctly there.

I tried to look into the NoopAnimationPlayer from @angular/animations and I assume its related to some timing issues with zone.js

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
Open in desktop
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