Skip to content

fix(core): preserve leave animation for sibling instances sharing a TNode#69296

Open
kaiguogit wants to merge 1 commit into
angular:mainfrom
kaiguogit:fix-animate-leave-sibling-instances
Open

fix(core): preserve leave animation for sibling instances sharing a TNode#69296
kaiguogit wants to merge 1 commit into
angular:mainfrom
kaiguogit:fix-animate-leave-sibling-instances

Conversation

@kaiguogit

Copy link
Copy Markdown

Fixes #69291

Summary

animate.leave is skipped — the element is removed from the DOM synchronously instead of running its leave animation — whenever a sibling instance of the same template enters in a different DOM parent during the same change-detection tick. This is the common "close section A while opening section B" pattern: accordions, tab groups, exclusive-expansion menus, master/detail navigation, where A and B are two instances of the same child component. It is a regression introduced in 21.2.0; it worked in 21.1.x.

Root cause

leavingNodes (packages/core/src/animation/utils.ts) is keyed by TNode, which is part of the compiled TView and is therefore shared by every instance of a template. So the animated element in instance A and the one in instance B map to the same TNode bucket.

On insert, applyToElementOrContainer calls cancelLeavingNodes(tNode, enteringEl). Its cross-parent branch:

} else if (
  (prevSibling && leavingEl === prevSibling) ||
  (leavingParent && newParent && leavingParent !== newParent)   // ← the problem
) {
  nodes.splice(i, 1);
  leavingEl.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
  leavingEl.parentNode?.removeChild(leavingEl);                 // ← synchronous removal
}

The leavingParent !== newParent removal was added in #67032 to de-duplicate a single dynamic component re-rendered into a fresh overlay/portal parent (e.g. CDK Overlay). But "different parent" is also the normal situation for two legitimate sibling instances, so it produces a false positive: A's still-animating element is force-removed because its host differs from B's host. (#67361 later rescued the node-move case via leavingEl === newElement, but the cross-parent removal for distinct sibling elements was untouched.)

The fix

The collision is purely a matter of tracking by TNode with no instance identity. We add instance identity:

  • leavingNodes now stores {el, declarationView} per leaving node instead of a bare HTMLElement.
  • trackLeavingNodes (called from the Detach branch, which has the leaving node's LView) records the declaration view (lView[DECLARATION_VIEW] ?? lView) of the leaving element.
  • cancelLeavingNodes (called from the Insert branch, which has the entering node's LView) resolves the entering node's declaration view and only performs the cross-parent removal when it matches the leaving node's declaration view — i.e. the same logical view re-rendered. Distinct instances of a shared template have different declaration views, so they are left alone and their animate.leave runs to completion.

The declaration view (rather than the rendered LView, which is a fresh object on every re-render) is used so the same logical insertion point compares equal across re-renders while distinct instances compare unequal. When no owning view is known on either side, the original (conservative) removal behavior is kept.

The prevSibling (same-position @if/VCR toggle) branch and the leavingEl === newElement (drag-and-drop node move, #67361) branch are unchanged.

Why this preserves #67032 and #67361

New regression test

Added to packages/core/test/acceptance/animation_spec.ts in the existing animation element duplication suite: "should run animate.leave for a sibling instance when another instance of the same template enters". It renders two instances of one component (panels sharing a TNode), closes A and opens B in the same tick, and asserts A's panel stays connected to the DOM (leave animation running) until its animationend, instead of being removed synchronously.

I verified this test fails on main ("Expected false to be true" at the leavingPanelA.isConnected assertion — A removed synchronously) and passes with the fix.

Local verification

Built and tested with Bazel locally (Node 22.22.3, pnpm 11.5.2):

  • //packages/core/test/acceptance:acceptance_web_chromiumPASS (2658 passed incl. the new test; the existing fix(core): prevent animated element duplication with dynamic components in zoneless mode #67032 overlay-dedup test green)
  • //packages/core/test/acceptance:acceptance_web_firefoxPASS
  • //packages/core/test/acceptance:acceptance (Node/jasmine) — PASS
  • Confirmed the new test fails without the source fix and passes with it.
  • Bundle-size golden symbol tests: the new internal helper getDeclarationView survives tree-shaking, so the 8 affected bundle.golden_symbols.json files were updated via the :symbol_test.accept targets; all 8 symbol_tests pass.
  • prettier --check passes on all changed source/test files.

Not run locally: the full repository test matrix and the integration/animations e2e harness (could not bring up the integration browser tooling in this environment). CI will cover those.

Uncertainty for reviewers

  • The discriminator is the declaration view. I confirmed empirically (via the two acceptance tests passing simultaneously) that it distinguishes the overlay re-render case from distinct sibling instances. If there is an overlay/portal scenario where the duplicate is re-rendered from a different declaring view than the leaving one, the cross-parent removal would no longer fire for it; I could not construct such a case, and the existing fix(core): prevent animated element duplication with dynamic components in zoneless mode #67032 test does not exercise one, but maintainers may know of one.
  • The new helper adds one symbol (getDeclarationView) to the animation-containing bundles. The golden updates are mechanical, but flagging the size delta explicitly.

…Node

`animate.leave` was skipped — the element was removed from the DOM
synchronously instead of running its leave animation — whenever a
sibling instance of the same template entered in a different DOM parent
during the same change-detection tick (e.g. an exclusive-expansion
accordion or nav where opening section B collapses section A).

`leavingNodes` is keyed by `TNode`, which is shared by every instance of
a template. When a node was inserted, `cancelLeavingNodes` force-removed
any tracked leaving node whose DOM parent differed from the entering
node's parent (the `leavingParent !== newParent` branch added to
de-duplicate a dynamic component re-rendered into a fresh overlay pane).
For two distinct live sibling instances that merely share a `TNode`,
"different parent" is the normal situation, so the still-animating
sibling was ripped out.

Track the declaration view of each leaving element alongside it, and
only perform the cross-parent removal when the entering element belongs
to the same declaration view as the leaving one — i.e. the same logical
view re-rendered, the case the branch was written for. Two distinct
instances of a shared template have different declaration views, so
their `animate.leave` is now left to run to completion.

This preserves the dynamic-component/overlay de-duplication (angular#67032) and
the drag-and-drop node-move rescue (angular#67361), which are unchanged.

Fixes angular#69291
@angular-robot angular-robot Bot added the area: core Issues related to the framework runtime label Jun 10, 2026
@ngbot ngbot Bot added this to the Backlog milestone Jun 10, 2026
@pullapprove pullapprove Bot requested a review from JeanMeche June 10, 2026 20:21
@JeanMeche JeanMeche requested a review from thePunderWoman June 10, 2026 20:21

@thePunderWoman thePunderWoman left a comment

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.

LGTM! Thanks for catching and fixing this!

@thePunderWoman thePunderWoman added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Jun 10, 2026
@ngbot

ngbot Bot commented Jun 11, 2026

Copy link
Copy Markdown

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

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: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

animate.leave skipped for the closing panel when switching panels in an exclusive accordion (sibling instances share a TNode)

2 participants