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

@@ -85,4 +85,12 @@ export interface AnimationLViewData {
 
   // Leave animations that apply to nodes in this view
   running?: Promise<PromiseSettledResult<void>[]>;
+
+  // 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;
 }

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<RepeaterContext<unknown>> {
+  override detach(index: number, skipLeaveAnimations?: boolean): LView<RepeaterContext<unknown>> {
     this.needsIndexUpdate ||= index !== this.length - 1;
+    if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index);
     return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
   }
   override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -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<T>(lContainer: LContainer, index: number): LView<T> {
   const existingLView = detachView(lContainer, index);
   ngDevMode && assertLView(existingLView);

packages/core/src/render3/list_reconciliation.ts

@@ -20,7 +20,7 @@ export abstract class LiveCollection<T, V> {
   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<T, V> {
     }
   }
   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 */));
   }
 }
 

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);
 }

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: `
+          <div>
+            @for (item of shown(); track item) {
+              <p animate.leave="fade" #el>I should slide in {{item}}.</p>
+            }
+          </div>
+        `,
+        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<HTMLParagraphElement>;
+        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);
+    }));
   });
 });