Skip to content

fix(material/dialog): content directives picking up wrong ref for stacked mixed type dialogs #21579

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crisbeto
Copy link
Member

The dialog content directives try to resolve their closest dialog using DI, and if it fails (e.g. inside a template dialog), they fall back to resolving it through the DOM. This becomes problematic if the ng-template was declared inside another dialog component, because it'll pick up the declaration dialog, not the one that the button exists in.

These changes remove the resolution using DI and switch to only going through the DOM.

Fixes #21554.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jan 13, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 13, 2021
jelbourn
jelbourn previously approved these changes Jan 13, 2021
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 13, 2021
@crisbeto crisbeto force-pushed the dialog-stacked-ref branch from a6502df to 8b7a29b Compare March 14, 2021 12:02
@crisbeto crisbeto force-pushed the dialog-stacked-ref branch from 8b7a29b to 8eda82e Compare April 14, 2021 20:52
@crisbeto crisbeto force-pushed the dialog-stacked-ref branch from 8eda82e to 08f36f7 Compare June 3, 2021 15:43
@crisbeto crisbeto force-pushed the dialog-stacked-ref branch from 08f36f7 to e701706 Compare July 18, 2021 17:55
@crisbeto crisbeto requested a review from a team as a code owner July 18, 2021 17:55
@josephperrott josephperrott removed the request for review from a team August 10, 2021 18:38
devversion
devversion previously approved these changes Aug 18, 2021
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
…cked mixed type dialogs

The dialog content directives try to resolve their closest dialog using DI, and if it fails (e.g.
inside a template dialog), they fall back to resolving it through the DOM. This becomes
problematic if the `ng-template` was declared inside another dialog component, because
it'll pick up the declaration dialog, not the one that the button exists in.

These changes remove the resolution using DI and switch to only going through the DOM.

Fixes angular#21554.
@crisbeto
Copy link
Member Author

crisbeto commented Mar 9, 2022

I'll mark this as blocked for now. There should be a better way of doing it with injectors now.

@crisbeto crisbeto added blocked This issue is blocked by some external factor, such as a prerequisite PR and removed action: merge The PR is ready for merge by the caretaker labels Mar 9, 2022
@andrewseguin andrewseguin added needs rebase and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Mar 28, 2022
@andrewseguin andrewseguin dismissed stale reviews from devversion and jelbourn via bf95d4a May 2, 2022 20:32
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@josephperrott josephperrott requested a review from a team as a code owner December 18, 2024 17:40
@josephperrott josephperrott requested review from amysorto and mmalerba and removed request for a team December 18, 2024 17:40
@mmalerba mmalerba removed their request for review February 19, 2025 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews blocked This issue is blocked by some external factor, such as a prerequisite PR cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
7 participants