From 3ec8a5c7536cdd2c1db7db4bfbc2d4995156a833 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Thu, 11 Sep 2025 14:19:50 -0700 Subject: [PATCH] fix(core): Prevent leave animations on a move operation (#63745) When a user has `animate.leave` on a list of items in a `@for`, but are only showing a subset using a computed, removing the second to last item results in a move operation on the last item. There's no native atomic move API in the browser. So this results in the element being detached and attached at its new index. The detaching of the node resulted in leave animations firing. This fix addresses this by adding a flag in the `LView[ANIMATIONS]` `AnimationLViewData` interface to allow for skipping animations. During list reconciliation, we set this flag so that the animations are skipped over. The flag is flipped back after the move operation is complete. There is one complication that results from this. The index adjustment of elements in the list happens synchronously while the leave animation is asynchronous. This results in the leaving item getting shifted to the end of the list. This is not ideal but likely can be addressed in a future refactor. fixes: #63544 PR Close #63745 --- packages/core/src/animation/interfaces.ts | 8 +++ .../src/render3/instructions/control_flow.ts | 15 ++++- .../core/src/render3/list_reconciliation.ts | 8 ++- .../core/src/render3/node_manipulation.ts | 16 +++-- .../core/test/acceptance/animation_spec.ts | 62 +++++++++++++++++++ 5 files changed, 101 insertions(+), 8 deletions(-) diff --git a/packages/core/src/animation/interfaces.ts b/packages/core/src/animation/interfaces.ts index aac45817dd0..65bee63448a 100644 --- a/packages/core/src/animation/interfaces.ts +++ b/packages/core/src/animation/interfaces.ts @@ -85,4 +85,12 @@ export interface AnimationLViewData { // Leave animations that apply to nodes in this view running?: Promise[]>; + + // Skip leave animations + // This flag is solely used when move operations occur. DOM Node move + // operations occur in lists, like `@for` loops, and use the same code + // path during move that detaching or removing does. So to prevent + // unexpected disappearing of moving nodes, we use this flag to skip + // the animations in that case. + skipLeaveAnimations?: boolean; } diff --git a/packages/core/src/render3/instructions/control_flow.ts b/packages/core/src/render3/instructions/control_flow.ts index 8ea286e8573..09ab65cc379 100644 --- a/packages/core/src/render3/instructions/control_flow.ts +++ b/packages/core/src/render3/instructions/control_flow.ts @@ -23,6 +23,7 @@ import {CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; import {ComponentTemplate} from '../interfaces/definition'; import {LocalRefExtractor, TAttributes, TNode, TNodeFlags} from '../interfaces/node'; import { + ANIMATIONS, CONTEXT, DECLARATION_COMPONENT_VIEW, HEADER_OFFSET, @@ -46,6 +47,7 @@ import { removeLViewFromLContainer, } from '../view/container'; import {removeDehydratedViews} from '../../hydration/cleanup'; +import {AnimationLViewData} from '../../animation/interfaces'; /** * Creates an LContainer for an ng-template representing a root node @@ -418,8 +420,9 @@ class LiveCollectionLContainerImpl extends LiveCollection< shouldAddViewToDom(this.templateTNode, dehydratedView), ); } - override detach(index: number): LView> { + override detach(index: number, skipLeaveAnimations?: boolean): LView> { this.needsIndexUpdate ||= index !== this.length - 1; + if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index); return detachExistingView>(this.lContainer, index); } override create(index: number, value: unknown): LView> { @@ -567,6 +570,16 @@ function getLContainer(lView: LView, index: number): LContainer { return lContainer; } +function setSkipLeaveAnimations(lContainer: LContainer, index: number): void { + if (lContainer.length <= CONTAINER_HEADER_OFFSET) return; + + const indexInContainer = CONTAINER_HEADER_OFFSET + index; + const viewToDetach = lContainer[indexInContainer]; + if (viewToDetach && viewToDetach[ANIMATIONS]) { + (viewToDetach[ANIMATIONS] as AnimationLViewData).skipLeaveAnimations = true; + } +} + function detachExistingView(lContainer: LContainer, index: number): LView { const existingLView = detachView(lContainer, index); ngDevMode && assertLView(existingLView); diff --git a/packages/core/src/render3/list_reconciliation.ts b/packages/core/src/render3/list_reconciliation.ts index bfc4bb64de3..6cf93e89ddb 100644 --- a/packages/core/src/render3/list_reconciliation.ts +++ b/packages/core/src/render3/list_reconciliation.ts @@ -20,7 +20,7 @@ export abstract class LiveCollection { abstract get length(): number; abstract at(index: number): V; abstract attach(index: number, item: T): void; - abstract detach(index: number): T; + abstract detach(index: number, skipLeaveAnimations?: boolean): T; abstract create(index: number, value: V): T; destroy(item: T): void { // noop by default @@ -45,7 +45,11 @@ export abstract class LiveCollection { } } move(prevIndex: number, newIdx: number): void { - this.attach(newIdx, this.detach(prevIndex)); + // For move operations, the detach code path is the same one used for removing + // DOM nodes, which would trigger `animate.leave` bindings. We need to skip + // those animations in the case of a move operation so the moving elements don't + // unexpectedly disappear. + this.attach(newIdx, this.detach(prevIndex, true /* skipLeaveAnimations */)); } } diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index e9bf0178a76..ee9fcee3657 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -352,12 +352,18 @@ function cleanUpView(tView: TView, lView: LView): void { function runLeaveAnimationsWithCallback(lView: LView | undefined, callback: Function) { if (lView && lView[ANIMATIONS] && lView[ANIMATIONS].leave) { - const runningAnimations = []; - for (let animateFn of lView[ANIMATIONS].leave) { - runningAnimations.push(animateFn()); + if (lView[ANIMATIONS].skipLeaveAnimations) { + lView[ANIMATIONS].skipLeaveAnimations = false; + } else { + const leaveAnimations = lView[ANIMATIONS].leave; + const runningAnimations = []; + for (let index = 0; index < leaveAnimations.length; index++) { + const animateFn = leaveAnimations[index]; + runningAnimations.push(animateFn()); + } + lView[ANIMATIONS].running = Promise.allSettled(runningAnimations); + lView[ANIMATIONS].leave = undefined; } - lView[ANIMATIONS].running = Promise.allSettled(runningAnimations); - lView[ANIMATIONS].leave = undefined; } runAfterLeaveAnimations(lView, callback); } diff --git a/packages/core/test/acceptance/animation_spec.ts b/packages/core/test/acceptance/animation_spec.ts index e7b3c129608..618dd2c1961 100644 --- a/packages/core/test/acceptance/animation_spec.ts +++ b/packages/core/test/acceptance/animation_spec.ts @@ -11,6 +11,7 @@ import {ViewEncapsulation} from '@angular/compiler'; import { AnimationCallbackEvent, Component, + computed, Directive, ElementRef, NgModule, @@ -1539,5 +1540,66 @@ describe('Animation', () => { expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1); expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4); })); + + it('should only remove one element in reactive `@for` loops when removing the second to last item', fakeAsync(() => { + const animateStyles = ` + .fade { + animation: fade-out 500ms; + } + @keyframes fade-out { + from { + opacity: 1; + } + to { + opacity: 0; + } + } + `; + + @Component({ + selector: 'test-cmp', + styles: animateStyles, + template: ` +
+ @for (item of shown(); track item) { +

I should slide in {{item}}.

+ } +
+ `, + encapsulation: ViewEncapsulation.None, + }) + class TestComponent { + items = signal([1, 2, 3, 4, 5, 6]); + shown = computed(() => this.items().slice(0, 3)); + @ViewChild('el', {read: ElementRef}) el!: ElementRef; + max = 6; + + removeSecondToLast() { + this.items.update((old) => { + const newList = [...old]; + newList.splice(1, 1); + return newList; + }); + } + } + TestBed.configureTestingModule({animationsEnabled: true}); + + const fixture = TestBed.createComponent(TestComponent); + const cmp = fixture.componentInstance; + fixture.detectChanges(); + cmp.removeSecondToLast(); + fixture.detectChanges(); + tickAnimationFrames(1); + + expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1); + expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4); + fixture.debugElement + .query(By.css('p.fade')) + .nativeElement.dispatchEvent( + new AnimationEvent('animationend', {animationName: 'fade-out'}), + ); + tick(); + expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3); + })); }); });