Skip to content

fix(core): prevent animated element duplication with dynamic components in zoneless mode#67032

Merged
mattrbeck merged 2 commits intoangular:mainfrom
mattlewis92:fix/animation-element-duplication-zoneless
Feb 13, 2026
Merged

fix(core): prevent animated element duplication with dynamic components in zoneless mode#67032
mattrbeck merged 2 commits intoangular:mainfrom
mattlewis92:fix/animation-element-duplication-zoneless

Conversation

@mattlewis92
Copy link
Copy Markdown
Contributor

Important

This was a fully vibe coded solution by Claude. Maybe there is a simpler way to solve this? At the very least, the test case reproduces the bug (I patched this into our application and verified it does fix the issue)

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #66794

What is the new behavior?

Fast clicking an animated menu to open and close it repeatedly in zoneless mode doesn't leave orphaned DOM nodes

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from JeanMeche February 11, 2026 21:13
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Feb 11, 2026
@ngbot ngbot bot added this to the Backlog milestone Feb 11, 2026
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

This seems fine, but I think we should be more aggressive in potential error cases. Tests look green, which is good, but I'm just a bit worried about the cancelLeavingNodes changes needing a few more guards for safety. Can you (or claude) do an assessment?

Gemini is worried that the leavingNodes keying logic isn't safe and could lead to cross-instance interference, but I think that's outside of the scope of this PR.

}

// Finish remaining animations (wait 100ms real time which is > 10ms animation)
// After all animations complete, 20 toggles (even) = closed = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important note: In this environment animation events are not fired by the browser. They have to be manually dispatched in the test, and I don't see that happening in your test here. You'll see examples of that in earlier tests. So this is likely something you'll want to verify is happening here, otherwise it may be a false positive or false negative.

Copy link
Copy Markdown
Contributor Author

@mattlewis92 mattlewis92 Feb 12, 2026

Choose a reason for hiding this comment

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

Addressed in 9b449d6 (also verified the test still fails with the patch not applied)

// renders in its own container, e.g. CDK Overlay).
// Leaving elements in the same parent that are NOT the previousSibling
// are left alone (e.g. @for items animating out at different positions).
if (leavingEl === prevSibling || (leavingParent && leavingParent !== newParent)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am a little concerned about this line and the potential for null === null in the case that newParent, prevSibling, or leavingEl are null. It's probably worth adding null check guards for safety.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9b449d6

@mattlewis92
Copy link
Copy Markdown
Contributor Author

This seems fine, but I think we should be more aggressive in potential error cases. Tests look green, which is good, but I'm just a bit worried about the cancelLeavingNodes changes needing a few more guards for safety. Can you (or claude) do an assessment?

Gemini is worried that the leavingNodes keying logic isn't safe and could lead to cross-instance interference, but I think that's outside of the scope of this PR.

This was claudes response:

Assessment: cancelLeavingNodes / leavingNodes safety

How it works: leavingNodes is a WeakMap<TNode, HTMLElement[]>. TNodes are shared across all embedded views created from the same template (same TView). This means if two components use the same
template, their leaving elements end up in the same array.

Is cross-instance interference possible? Theoretically, but only if:

  1. Two component instances share the same TemplateRef
  2. Both have leaving elements tracked simultaneously
  3. One inserts a new element while the other is still leaving

The leavingParent !== newParent check would match the other instance's element and cancel it prematurely.

This is not a new risk. The leavingNodes WeakMap keyed by TNode existed before this PR — it was already populated by trackLeavingNodes inside animateLeaveClassRunner and consumed by
cancelLeavingNodes called from ɵɵanimateEnter. The same cross-instance interference risk was already present. This PR doesn't change the keying strategy.

Practical impact: The scenario requires two menus from the same template to close simultaneously, then one to reopen before the other's CSS animation completes. With CDK Overlay, only one menu is
typically open per trigger, making this extremely unlikely. If it did occur, the other menu's leave animation would be cut short — not ideal but not a crash.

Could we make it safer? The key would need something instance-specific (e.g. LView ID or VCR identity), but that's a more invasive refactor of the existing leavingNodes data structure and outside
the scope of this fix.

Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM, and a huge thank you!

@thePunderWoman
Copy link
Copy Markdown
Contributor

@mattlewis92 Looks like you have a minor lint issue to fix.

…ts in zoneless mode

When using ViewContainerRef to rapidly toggle animated elements in
zoneless mode (e.g. CDK Overlay menus), multiple copies of the element
could appear in the DOM. This happened because leave animations were
queued but not yet executed, and the existing `cancelLeavingNodes`
mechanism could not find the leaving element to cancel it — it ran
during template execution before the new element was in the DOM, and
used the declaration container's anchor which doesn't work for
overlay/portal patterns where elements are moved to separate
containers.

This commit replaces the previous approach with a simpler mechanism:

- Track leaving nodes immediately during the Detach tree walk (before
  the leave animation queue flushes) so they are discoverable
- Cancel leaving nodes after DOM insertion in the Insert tree walk,
  checking both `previousSibling` (same-container toggle) and
  different DOM parents (overlay/portal case)
- Revert the ineffective fix from 38263f6 (`removeAnimationsFromQueue`)

This also removes the circular dependency between `animation/utils.ts`
and `render3/node_manipulation.ts` (`getBeforeNodeForView` import).
…omponents in zoneless mode

fixup! fix(core): prevent animated element duplication with dynamic components in zoneless mode

Address review comments:
- Add null guards to cancelLeavingNodes to prevent null === null matches
- Rewrite test to use fakeAsync with tickAnimationFrames and manual
  animationend event dispatch instead of real async delays
@mattlewis92 mattlewis92 force-pushed the fix/animation-element-duplication-zoneless branch from 9b449d6 to b6f00f7 Compare February 12, 2026 17:25
@mattlewis92
Copy link
Copy Markdown
Contributor Author

@mattlewis92 Looks like you have a minor lint issue to fix.

Odd, I didn't change the file it's complaining about. Rebasing against main and hoping that helps 🙏

@thePunderWoman
Copy link
Copy Markdown
Contributor

@mattlewis92 Looks like the rebase took care of it. Thanks!

@thePunderWoman thePunderWoman added target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 12, 2026
@thePunderWoman
Copy link
Copy Markdown
Contributor

@mattlewis92 Bummer...looks like this has conflicts with the patch branch. We can merge it into main targeting the next minor. You'd have to create a patch version of this PR to get it to safely land in patch as well.

@mattlewis92
Copy link
Copy Markdown
Contributor Author

@mattlewis92 Bummer...looks like this has conflicts with the patch branch. We can merge it into main targeting the next minor. You'd have to create a patch version of this PR to get it to safely land in patch as well.

We are actually still on v20 right now so not a big deal for us, I've just been having claude patch these fixes into our angular version 😅 If other people report the same issue I'd be happy to make a patch version as well though!

@thePunderWoman thePunderWoman added the action: merge The PR is ready for merge by the caretaker label Feb 12, 2026
@thePunderWoman thePunderWoman removed the request for review from JeanMeche February 12, 2026 19:22
@mattrbeck mattrbeck merged commit 0806ee3 into angular:main Feb 13, 2026
42 of 45 checks passed
@mattrbeck
Copy link
Copy Markdown
Member

This PR was merged into the repository. The changes were merged into the following branches:

@basrieter
Copy link
Copy Markdown

I don't know 100% sure, but what I see in 21.2.0 is that if I use:

@HostBinding("animate.leave")
animateLeave = "state-leave";

the element will not be removed from the DOM at all.

@mattlewis92
Copy link
Copy Markdown
Contributor Author

mattlewis92 commented Mar 2, 2026

I don't know 100% sure, but what I see in 21.2.0 is that if I use:

@HostBinding("animate.leave")
animateLeave = "state-leave";

the element will not be removed from the DOM at all.

Please could you make a minimal stackblitz that reproduces the problem you're experiencing, and file a new issue with a bug report? It's hard to reproduce the problem to fix it with just the code snippet you provided.

@basrieter
Copy link
Copy Markdown

Done: #67400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants