fix(core): skip leave animations on view swaps

We accounted for skipping leave animations during moves, but not swaps.
This accounts for the swap cases and updates how we deal with swaps and
moves. Now we always queue animations and then essentially dequeue them
if we attach them back in the same render pass.

fixes: angular#64818
fixes: angular#64730

Author: thePunderWoman

packages/core/src/animation/interfaces.ts

@@ -98,5 +98,11 @@ export interface AnimationLViewData {
   // 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;
+  detach?: boolean;
+
+  // Animation functions that have been queued for this view when the view is detached.
+  // This is used to later remove them from the global animation queue if the view
+  // is attached before the animation queue runs. This is used in cases where views are
+  // moved or swapped during list reconciliation.
+  qeueued?: Function[];
 }

packages/core/src/animation/queue.ts

@@ -8,7 +8,7 @@
 
 import {afterNextRender} from '../render3/after_render/hooks';
 import {InjectionToken, Injector} from '../di';
-import {NodeAnimations} from './interfaces';
+import {AnimationLViewData, NodeAnimations} from './interfaces';
 
 export interface AnimationQueue {
   queue: Set<Function>;
@@ -33,18 +33,44 @@ export const ANIMATION_QUEUE = new InjectionToken<AnimationQueue>(
   },
 );
 
-export function addToAnimationQueue(injector: Injector, animationFns: Function | Function[]) {
+export function addToAnimationQueue(
+  injector: Injector,
+  animationFns: Function | Function[],
+  animationData?: AnimationLViewData,
+) {
   const animationQueue = injector.get(ANIMATION_QUEUE);
   if (Array.isArray(animationFns)) {
     for (const animateFn of animationFns) {
       animationQueue.queue.add(animateFn);
+      // If a node is detached, we need to keep track of the queued animation functions
+      // so we can later remove them from the global animation queue if the view
+      // is re-attached before the animation queue runs.
+      if (animationData?.detach) {
+        (animationData.qeueued ??= []).push(animateFn);
+      }
     }
   } else {
     animationQueue.queue.add(animationFns);
+    // If a node is detached, we need to keep track of the queued animation functions
+    // so we can later remove them from the global animation queue if the view
+    // is re-attached before the animation queue runs.
+    if (animationData?.detach) {
+      (animationData.qeueued ??= []).push(animationFns);
+    }
   }
   animationQueue.scheduler && animationQueue.scheduler(injector);
 }
 
+export function removeFromAnimationQueue(injector: Injector, animationData: AnimationLViewData) {
+  const animationQueue = injector.get(ANIMATION_QUEUE);
+  if (animationData.qeueued) {
+    for (const animationFn of animationData.qeueued) {
+      animationQueue.queue.delete(animationFn);
+    }
+    animationData.qeueued = undefined;
+  }
+}
+
 export function scheduleAnimationQueue(injector: Injector) {
   const animationQueue = injector.get(ANIMATION_QUEUE);
   // We only want to schedule the animation queue if it hasn't already been scheduled.

packages/core/src/render3/instructions/control_flow.ts

@@ -28,6 +28,7 @@ import {
   DECLARATION_COMPONENT_VIEW,
   HEADER_OFFSET,
   HYDRATION,
+  INJECTOR,
   LView,
   TVIEW,
   TView,
@@ -48,6 +49,8 @@ import {
   removeLViewFromLContainer,
 } from '../view/container';
 import {declareNoDirectiveHostTemplate} from './template';
+import {removeFromAnimationQueue} from '../../animation/queue';
+import {allLeavingAnimations} from '../../animation/longest_animation';
 
 /**
  * Creates an LContainer for an ng-template representing a root node
@@ -419,10 +422,11 @@ class LiveCollectionLContainerImpl extends LiveCollection<
       index,
       shouldAddViewToDom(this.templateTNode, dehydratedView),
     );
+    clearDetachFlag(this.lContainer, index);
   }
-  override detach(index: number, skipLeaveAnimations?: boolean): LView<RepeaterContext<unknown>> {
+  override detach(index: number): LView<RepeaterContext<unknown>> {
     this.needsIndexUpdate ||= index !== this.length - 1;
-    if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index);
+    setDetachFlag(this.lContainer, index);
     return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
   }
   override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -570,13 +574,35 @@ function getLContainer(lView: LView, index: number): LContainer {
   return lContainer;
 }
 
-function setSkipLeaveAnimations(lContainer: LContainer, index: number): void {
+function clearDetachFlag(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).detach
+  ) {
+    const animations = viewToDetach[ANIMATIONS] as AnimationLViewData;
+    animations.detach = false;
+    if (animations.qeueued && animations.qeueued.length > 0) {
+      const injector = viewToDetach[INJECTOR];
+      removeFromAnimationQueue(injector, animations);
+      allLeavingAnimations.delete(viewToDetach);
+    }
+  }
+}
+
+function setDetachFlag(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;
+    const animations = viewToDetach[ANIMATIONS] as AnimationLViewData;
+    animations.detach = true;
+    animations.qeueued = [];
   }
 }
 

packages/core/src/render3/list_reconciliation.ts

@@ -21,7 +21,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, skipLeaveAnimations?: boolean): T;
+  abstract detach(index: number): T;
   abstract create(index: number, value: V): T;
   destroy(item: T): void {
     // noop by default
@@ -50,7 +50,7 @@ export abstract class LiveCollection<T, V> {
     // 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 */));
+    this.attach(newIdx, this.detach(prevIndex));
   }
 }
 
@@ -131,6 +131,7 @@ export function reconcile<T, V>(
       // compare from the beginning
       const liveStartValue = liveCollection.at(liveStartIdx);
       const newStartValue = newCollection[liveStartIdx];
+      const newLastValue = newCollection[newEndIdx];
 
       if (ngDevMode) {
         recordDuplicateKeys(duplicateKeys!, trackByFn(liveStartIdx, newStartValue), liveStartIdx);
@@ -180,6 +181,7 @@ export function reconcile<T, V>(
       const liveStartKey = trackByFn(liveStartIdx, liveStartValue);
       const liveEndKey = trackByFn(liveEndIdx, liveEndValue);
       const newStartKey = trackByFn(liveStartIdx, newStartValue);
+
       if (Object.is(newStartKey, liveEndKey)) {
         const newEndKey = trackByFn(newEndIdx, newEndValue);
         // detect swap on both ends;

packages/core/src/render3/node_manipulation.ts

@@ -386,37 +386,35 @@ function runLeaveAnimationsWithCallback(
   if (animations == null || animations.leave == undefined || !animations.leave.has(tNode.index))
     return callback(false);
 
-  // this is solely for move operations to prevent leave animations from running
-  // on the moved nodes, which would have deleted the node.
-  if (animations.skipLeaveAnimations) {
-    animations.skipLeaveAnimations = false;
-    return callback(false);
-  }
-
   if (lView) allLeavingAnimations.add(lView);
 
-  addToAnimationQueue(injector, () => {
-    // it's possible that in the time between when the leave animation was
-    // and the time it was executed, the data structure changed. So we need
-    // to be safe here.
-    if (animations.leave && animations.leave.has(tNode.index)) {
-      const leaveAnimationMap = animations.leave;
-      const leaveAnimations = leaveAnimationMap.get(tNode.index);
-      const runningAnimations = [];
-      if (leaveAnimations) {
-        for (let index = 0; index < leaveAnimations.animateFns.length; index++) {
-          const animationFn = leaveAnimations.animateFns[index];
-          const {promise} = animationFn();
-          runningAnimations.push(promise);
+  addToAnimationQueue(
+    injector,
+    () => {
+      // it's possible that in the time between when the leave animation was
+      // and the time it was executed, the data structure changed. So we need
+      // to be safe here.
+      if (animations.leave && animations.leave.has(tNode.index)) {
+        const leaveAnimationMap = animations.leave;
+        const leaveAnimations = leaveAnimationMap.get(tNode.index);
+        const runningAnimations = [];
+        if (leaveAnimations) {
+          for (let index = 0; index < leaveAnimations.animateFns.length; index++) {
+            const animationFn = leaveAnimations.animateFns[index];
+            const {promise} = animationFn();
+            runningAnimations.push(promise);
+          }
+          animations.qeueued = undefined;
         }
+        animations.running = Promise.allSettled(runningAnimations);
+        runAfterLeaveAnimations(lView!, callback);
+      } else {
+        if (lView) allLeavingAnimations.delete(lView);
+        callback(false);
       }
-      animations.running = Promise.allSettled(runningAnimations);
-      runAfterLeaveAnimations(lView!, callback);
-    } else {
-      if (lView) allLeavingAnimations.delete(lView);
-      callback(false);
-    }
-  });
+    },
+    animations,
+  );
 }
 
 function runAfterLeaveAnimations(lView: LView, callback: Function) {

packages/core/test/acceptance/animation_spec.ts

@@ -2057,6 +2057,47 @@ describe('Animation', () => {
       expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3);
     }));
 
+    it('should not remove elements when swapping or moving nodes', fakeAsync(() => {
+      const animateSpy = jasmine.createSpy('animateSpy');
+      @Component({
+        selector: 'test-cmp',
+        template: `
+          <div>
+            @for (item of items; track item.id) {
+              <p (animate.leave)="animate($event)" #el>{{ item.id }}</p>
+            }
+          </div>
+        `,
+        encapsulation: ViewEncapsulation.None,
+      })
+      class TestComponent {
+        items = [{id: 1}, {id: 2}, {id: 3}];
+        private cd = inject(ChangeDetectorRef);
+
+        animate(event: AnimationCallbackEvent) {
+          animateSpy();
+          event.animationComplete();
+        }
+
+        shuffle() {
+          this.items = this.shuffleArray(this.items);
+          this.cd.markForCheck();
+        }
+
+        shuffleArray<T>(array: readonly T[]): T[] {
+          return [array[1], array[2], array[0]];
+        }
+      }
+      TestBed.configureTestingModule({animationsEnabled: true});
+
+      const fixture = TestBed.createComponent(TestComponent);
+      const cmp = fixture.componentInstance;
+      cmp.shuffle();
+      fixture.detectChanges();
+      expect(animateSpy).not.toHaveBeenCalled();
+      expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3);
+    }));
+
     it('should not remove elements when child element animations finish', fakeAsync(() => {
       const animateStyles = `
         .fade {