bug(MatDialog): When opening multiple dialogs in a short amount of time, some of them do not open

[guimabdo]

Is this a regression?

• [X] Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

12.2.0

Description

When opening multiple dialogs in a short amount of time, some of them do not open

Reproduction

The bug can be noticed with the example below, when clicking the "Open Fast" button.

import { Component, TemplateRef, ViewChild } from '@angular/core';
import { MatDialog } from '@angular/material/dialog';

@Component({
  selector: 'app-root',
  template: `
  <button (click)="openFast()">Open Fast</button>
  <button (click)="openSlow()">Open Slow</button>
  <ng-template let-data #template>
    {{data.message}}
    <button mat-dialog-close>close</button>
  </ng-template>
  `
})
export class AppComponent {
  @ViewChild("template") template?: TemplateRef<any>;
  constructor(private matDialog: MatDialog){}
  openFast(){
    this.open('1');
    setTimeout(() => this.open('2'), 1);
    setTimeout(() => this.open('3'), 50);
    setTimeout(() => this.open('4'), 100);
    setTimeout(() => this.open('5'), 150);
    setTimeout(() => this.open('6'), 200);
  }
  openSlow(){
    this.open('1');
    setTimeout(() => this.open('2'), 300);
    setTimeout(() => this.open('3'), 600);
    setTimeout(() => this.open('4'), 900);
    setTimeout(() => this.open('5'), 1200);
    setTimeout(() => this.open('6'), 1500);
  }

  private open(message: string){
    this.matDialog.open(this.template!, { 
      disableClose: true,
      data: { message } 
    });
  }
}

Expected Behavior

"Open fast" button should open 6 dialogs, just like the "Open slow" button.

Actual Behavior

The "Open fast" button opens only 2 dialogs. On most attempts it opens (1) and (5), but in some cases (1) and (6) are open.

Environment

• Angular: 13.0.0
• CDK/Material: 13.0.0
• Browser(s): Chrome
• Operating System (e.g. Windows, macOS, Ubuntu): Windows

[hijamoya]

We encounter the same issue, If we call open in ngOnInit like below:

  ngOnInit() {
      setTimeout(() => this.open('2'), 0);
  }

The dialog dose not open anymore, it is fine in previous version.

However, if we just:

  ngOnInit() {
      this.open('2');
  }

The dialog will be opened correctly.

[lephyrus]

We've also hit this issue. I'm sure it was introduced with this commit. There's a comment on it that points to the underlying issue.

The commit message says the goal is to [a]void opening multiple of the same dialog before animations complete by returning the previous MatDialogRef, but the change does not check whether the two dialogs are the same. This leads to really weird behaviour, which took us a long time to figure out. Here's our use case (which was working fine before):

// injected in real code
declare const dialog: MatDialog;

// somewhere in the codebase
const dialogA = dialog.open(DialogA); // dialog that shows a spinner while async thing A is happening

// somewhere else in codebase, gets executed as part of A succeeding but can fail independently
const dialogB = dialog.open(DialogB); // dialog that shows an error
dialogB.afterClosed().pipe(/* do stuff according to user response */);

The way the timing works out in our case, dialogB will sometimes be a reference to DialogB (MatDialogRef<DialogB>, as guaranteed by the typing of open()), but mostly it will be a reference to DialogA and (more crucially) dialog B wasn't opened and we're processing a response that was given for the other dialog.

Any chance you could take another look at this, @Splaktar?

[Splaktar]

Thank you for the example code. It would be great if you could provide a runnable StackBlitz to help us investigate this more efficiently.

[Splaktar]

Does this still happen when you import NoopAnimations instead of BrowserAnimations?

https://github.com/angular/components/blob/0b02322b68ef340aa3dc4b9909324ac8e4b165b9/src/material/dialog/dialog.ts#L467-L468

[Splaktar]

@lephyrus there is a lot of discussion about the tradeoffs and issues that we faced here in the following PRs

• https://github.com/angular/components/pull/23228
• https://github.com/angular/components/pull/23085

[Splaktar]

If I had a StackBlitz for this, I could try out a few quick things right now and see if I can provide a workaround, but since I don't, I probably won't have time to look into this again this week.

[guimabdo]

Thank you for the example code. It would be great if you could provide a runnable StackBlitz to help us investigate this more efficiently.

@Splaktar I tried but couldn't. I think StackBlitz has some issue with Angular 13: https://github.com/stackblitz/core/issues/1657

[Splaktar]

Doh. Thank you for linking that StackBlitz issue. Let me see if I can get you a working Angular v13 StackBlitz template.

[Splaktar]

PR https://github.com/angular/components/pull/13466 could potentially help, so you could disable the animation or at least know how long they would run in these cases. However, it's not clear if or when that will land.

[hijamoya]

@Splaktar Can you provide an option to disable this behavior?, this impacts our product so much, many dialogs does not open correctly after upgrade the version.

Thanks!

[Totati]

I've could make a stackblitz example, it can be forked.

[guimabdo]

My case is very similar to @lephyrus 's: a busy indicator that may be followed by an error message. To work around I ended up changing the busy indicator to not use MatDialog. I still have to review our applications for specific cases. I think there might be some form dialog that opens another prompt/alert dialog when opening. If I find this case, I'll add some delay.

[lephyrus]

@lephyrus there is a lot of discussion about the tradeoffs and issues that we faced here in the following PRs

• fix(material/dialog): improve screen reader support when opened #23228
• fix(material/dialog): improve screen reader support when opened #23085

Thanks, @Splaktar. Here's what I'm not clear on: The current behaviour is that no dialog will be opened while another one is opening. Is that the intention of the change? In the PR discussion, there is talk of componentOrTemplateRef === this._lastDialogOpenType (which isn't part of the code now), the commit message says "avoid opening multiple of the same dialog", and the test for the behaviour uses the same component:

https://github.com/angular/components/blob/699e19b8eba8d5554e44ae4fba6e828a86c7ac64/src/material-experimental/mdc-dialog/dialog.spec.ts#L2010-L2012

Currently, this would pass but seems very strange (i.e. the reference I get from open() might be totally unrelated to the component I passed, which also violates the return type):

fooRef = dialog.open(Foo);
barRef = dialog.open(Bar);
expect(barRef).toEqual(fooRef);
expect(barRef.componentInstance instanceof Foo).toBe(true);

[albert-asin]

adding a delay as @guimabdo says works as workarround

[markwhitfeld]

FYI, StackBlitz now supports Angular 13. You can go ahead and make your repro if still needed.

Great news! Support for Angular 13 has finally rolled out. ... For the next day or two (until 12 December 2021), there is a possibility of an initial slow down in starting up the dev server or running NGCC (or potentially even timeouts) as the packages repopulate into the cache as they are used, but it should come right after a refresh or two if you do have any issues. Anything else should be reported as a new issue.

Related StackBlitz issue comment: https://github.com/stackblitz/core/issues/1657#issuecomment-989779722

[Splaktar]

The current behaviour is that no dialog will be opened while another one is opening. Is that the intention of the change?

Yes.

In the PR discussion, there is talk of componentOrTemplateRef === this._lastDialogOpenType (which isn't part of the code now)

Yes, per code review feedback and due to breaking existing tests we moved away from that.

the commit message says "avoid opening multiple of the same dialog",

I think that part of the commit message just didn't get updated before the final merge of the PR. Sorry about that. This does cover the most common case we were focused on though, which was multiple clicks/key presses on a button (or other trigger) causing the dialog to be opened twice.

Currently, this would pass but seems very strange (i.e. the reference I get from open() might be totally unrelated to the component I passed, which also violates the return type)

Wouldn't that trigger a TypeScript compiler error?

[Splaktar]

Also on the topic of opening multiple dialogs at the same time, the Material Design guidelines for Dialogs include this description:

A dialog is a type of modal window that appears in front of app content to provide critical information or ask for a decision. Dialogs disable all app functionality when they appear, and remain on screen until confirmed, dismissed, or a required action has been taken.

There are a number of other recommendations there that clearly call out something like "dialog that shows a spinner while async thing A is happening" as an improper use of a dialog.

In those cases, you may want to look at building a custom component for your application using CDK Overlay which provides a number of position and scroll strategies.

In https://github.com/angular/components/issues/6761#issuecomment-353746167, @crisbeto mentioned that we officially support opening multiple dialogs on the page. It's not clear if he was intending to say that we support opening multiple dialogs in a very short period of time (before they can animate open). That said, I believe that the fix there is what caused the a11y issue in https://github.com/angular/components/issues/21840 that I just fixed.

[marsc]

We're facing the same issue in our application since Angular 13. Rolled back to version 12.x. This is a breaking change! If this is the intended behaviour we've to do a very large refactoring just to update to version 13 again. Very bad.

[Splaktar]

@marsc can you please share your use case and a reproduction?

[marsc]

@marsc can you please share your use case and a reproduction?

Sure: https://stackblitz.com/edit/angular13-mat-dialog?file=src/app/app.component.ts https://stackblitz.com/edit/angular12-mat-dialog?file=src/app/app.component.ts

As you will notice "Open Dialog Async" doesn't work in version 13 but in version 12. Opening a dialog directly after another one closes is a very common use case in our app since we started using Angular years ago.

[Kademlia]

Also on the topic of opening multiple dialogs at the same time, the Material Design guidelines for Dialogs include this description:

A dialog is a type of modal window that appears in front of app content to provide critical information or ask for a decision. Dialogs disable all app functionality when they appear, and remain on screen until confirmed, dismissed, or a required action has been taken.

We have the same problem here. I do not know how to solve the following example:

1. User decides to delete a file
2. A MatDialog is opened "Are you sure you want to delete file XY?"
3. After clicking "Yes, delete" another MatDialog needs to popup instantly with "Unable to delete File. No permission!" (In both cases the described behavior is needed and the two different dialogs will popup instantly, not needing arbitrary animation time)

How should I implement this usecase without using MatDialog two times directly after another?

[marsc]

Also on the topic of opening multiple dialogs at the same time, the Material Design guidelines for Dialogs include this description:

A dialog is a type of modal window that appears in front of app content to provide critical information or ask for a decision. Dialogs disable all app functionality when they appear, and remain on screen until confirmed, dismissed, or a required action has been taken.

We have the same problem here. I do not know how to solve the following example:

1. User decides to delete a file
2. A MatDialog is opened "Are you sure you want to delete file XY?"
3. After clicking "Yes, delete" another MatDialog needs to popup instantly with "Unable to delete File. No permission!" (In both cases the described behavior is needed and the two different dialogs will popup instantly, not needing arbitrary animation time)

How should I implement this usecase without using MatDialog two times directly after another?

@Kademlia in this case you can do something like this:

this.matDialog.open( ... ).afterClosed().subscribe(() => { this.matDialog.open( ... ); });

[dushkostanoeski]

@marsc there lies the problem. If the task from the first dialog finishes before the first dialog is fully opened, the second dialog won't show up.

[Splaktar]

PR https://github.com/angular/components/pull/24121 has been tested and reviewed. It should land shortly and fix this.

[aikrez]

Any ETA? This is the only thing that is blocking the upgrade of my project.

[Splaktar]

@aikrez there is nothing blocking the PR from being merged. It's been fully reviewed and approved. I can't give an ETA though.

[Splaktar]

@aikrez okay, I do have an update. There have been some undesirable side effects of this change in the presubmit tests. They are being investigated and potential solutions and workarounds are being evaluated.

[rasoulMrz]

Such a shame! It has been 2.5 months and the issue remains? that's awefull!

[rasoulMrz]

@aikrez okay, I do have an update. There have been some undesirable side effects of this change in the presubmit tests. They are being investigated and potential solutions and workarounds are being evaluated.

Any updates?

[P-de-Jong]

Would like to know too when this PR is getting merged. We're having a lot of problems with this issue at the moment 😢

[proggler23]

It seems like opening a dialog from within another dialog also fails if there happened to be an error shortly before (within the animation duration). I cannot listen on "dialogOpened" observable, as this only triggers once, but the error will trigger the dialogContainer.start animation again.

[crisbeto]

I reworked the fix for this on Friday to make it easier to merge and got it passing all the internal checks. It'll be merged in on Monday and should be out in the next patch release.

[DoodahProductions]

Hello, thanks for this fix ! I'm not sure what npm package I should update to fetch this fix, may you help me ? Thanks :)

[crisbeto]

The fix will be in @angular/material, but we haven't made a release with the new code yet. It should be out later today/tomorrow.

[Splaktar]

This fix is included in 13.2.6 and 14.0.0-next.6.

[P-de-Jong]

We have updated our applications to the new patch and everything seems to work as intended. Thanks!

[angular-automatic-lock-bot[bot]]

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}