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

DOM nodes not cleaned up if view container is destroyed before Dialog close animation starts #17891

Open
deviantpixel opened this issue Dec 6, 2019 · 0 comments · May be fixed by #24739
Open
Labels
area: material/dialog P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects

Comments

@deviantpixel
Copy link

deviantpixel commented Dec 6, 2019

Bug, feature request, or proposal:

When a Dialog is closed before the close animation starts the overlay container is not destroyed. This is similar to #16284 which resulted in a PR #16309.

But this PR doesn't fix the edge case where the animation never even starts before it is destroyed. It assumes the animation has to at least start which is where the setTimeout was added. The solution most likely needs to create the timeout inside the close() function which terminates after the animation timeout starts.

Specifically I see the close function start, end and then the page fully destroys before the animation observable even gets a change to start (never makes it to the .subscribe segment of this._containerInstance.animationStateChanged.

What is the expected behavior?

I would expect that when the close() function is called that the timeout is set immediately (not in an observable) as a fallback in case any part of the animation close observable (even it's start) doesn't occur which will still call this._overlayRef.dispose().

What is the current behavior?

Current behavior is the overlay DOM elements are still present after a path change while a modal is still open.

What are the steps to reproduce?

This is still a good place to reproduce this: https://stackblitz.com/github/mike88ua/redirect_from_dialog
Simple steps are open a dialog and generate a path change while the dialog is still open that is a local Angular path change to some place in the same angular app. The Overlay DOM elements are still present.

What is the use-case or motivation for changing an existing behavior?

Avoid overlay fragments being left on the page.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

  • Angular: 7.3.x
  • Material: 7.3.x
  • Browser: Chrome

Is there anything else we should know?

I will test a version of this change and provide a PR if it works correctly.

@deviantpixel deviantpixel changed the title DOM nodes not cleaned up if view container is destroyed before Dialog close animation completes or starts Dec 10, 2019
@mmalerba mmalerba added the needs triage This issue needs to be triaged by the team label May 20, 2020
@crisbeto crisbeto added area: material/dialog P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels May 26, 2020
@crisbeto crisbeto added this to Triaged in triage #1 via automation May 26, 2020
@mmalerba mmalerba removed the has pr label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: material/dialog P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
3 participants