fix(core): Ensures @for loop animations never get cancelled (#63328)

There's special logic in place to prevent duplicate nodes from showing up in the case when an `@if` toggles a view quickly. This had the unfortunate side effect of causing `@for` leave animations to get cancelled when an add and remove happened simultaneously, even if it was a different index. This fix prevents that from happening in the `@for` loop case.

fixes: #63307

PR Close #63328

Author: thePunderWoman

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

@@ -20,7 +20,16 @@ import {
   getClassListFromValue,
 } from '../../animation/element_removal_registry';
 import {getLView, getCurrentTNode, getTView, getAnimationElementRemovalRegistry} from '../state';
-import {RENDERER, INJECTOR, CONTEXT, FLAGS, LViewFlags, LView, TView} from '../interfaces/view';
+import {
+  RENDERER,
+  INJECTOR,
+  CONTEXT,
+  FLAGS,
+  LViewFlags,
+  LView,
+  TView,
+  DECLARATION_LCONTAINER,
+} from '../interfaces/view';
 import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {getNativeByTNode, storeCleanupWithContext} from '../util/view_utils';
 import {performanceMarkFeature} from '../../util/performance';
@@ -30,6 +39,7 @@ import {NgZone} from '../../zone';
 import {assertDefined} from '../../util/assert';
 import {determineLongestAnimation, LongestAnimation} from '../../animation/longest_animation';
 import {TNode} from '../interfaces/node';
+import {getBeforeNodeForView} from '../node_manipulation';
 
 const DEFAULT_ANIMATIONS_DISABLED = false;
 const areAnimationSupported =
@@ -101,15 +111,25 @@ function clearLeavingNodes(tNode: TNode, el: HTMLElement): void {
 
 /**
  * In the case that we have an existing node that's animating away, like when
- * an `@if` toggles quickly or `@for` adds and removes elements quickly, we
- * need to end the animation for the former node and remove it right away to
- * prevent duplicate nodes showing up.
+ * an `@if` toggles quickly, we need to end the animation for the former node
+ * and remove it right away to prevent duplicate nodes showing up.
  */
-function cancelLeavingNodes(tNode: TNode, el: HTMLElement): void {
-  leavingNodes
-    .get(tNode)
-    ?.pop()
-    ?.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
+function cancelLeavingNodes(tNode: TNode, lView: LView): void {
+  const leavingEl = leavingNodes.get(tNode)?.shift();
+  const lContainer = lView[DECLARATION_LCONTAINER];
+  if (lContainer) {
+    // this is the insertion point for the new TNode element.
+    // it will be inserted before the declaring containers anchor.
+    const beforeNode = getBeforeNodeForView(tNode.index, lContainer);
+    // here we need to check the previous sibling of that anchor
+    const previousNode = beforeNode?.previousSibling;
+    // We really only want to cancel animations if the leaving node is the
+    // same as the node before where the new node will be inserted. This is
+    // the control flow scenario where an if was toggled.
+    if (leavingEl && previousNode && leavingEl === previousNode) {
+      leavingEl.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
+    }
+  }
 }
 
 function trackLeavingNodes(tNode: TNode, el: HTMLElement): void {
@@ -178,6 +198,8 @@ export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEn
       cleanupFns.push(renderer.listen(nativeElement, 'transitionstart', handleAnimationStart));
     });
 
+    cancelLeavingNodes(tNode, lView);
+
     trackEnterClasses(nativeElement, activeClasses, cleanupFns);
 
     for (const klass of activeClasses) {
@@ -188,12 +210,6 @@ export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEn
     // preventing an animation via selector specificity.
     ngZone.runOutsideAngular(() => {
       requestAnimationFrame(() => {
-        // In the case that we have an existing node that's animating away, like when
-        // an `@if` toggles quickly or `@for` adds and removes elements quickly, we
-        // need to end the animation for the former node and remove it right away to
-        // prevent duplicate nodes showing up.
-        cancelLeavingNodes(tNode, nativeElement);
-
         determineLongestAnimation(nativeElement, longestAnimations, areAnimationSupported);
         if (!longestAnimations.has(nativeElement)) {
           for (const klass of activeClasses) {
@@ -253,11 +269,8 @@ export function ɵɵanimateEnterListener(value: AnimationFunction): typeof ɵɵa
 
   const tNode = getCurrentTNode()!;
   const nativeElement = getNativeByTNode(tNode, lView) as HTMLElement;
-  // In the case that we have an existing node that's animating away, like when
-  // an `@if` toggles quickly or `@for` adds and removes elements quickly, we
-  // need to end the animation for the former node and remove it right away to
-  // prevent duplicate nodes showing up.
-  cancelLeavingNodes(tNode, nativeElement);
+
+  cancelLeavingNodes(tNode, lView);
 
   value.call(lView[CONTEXT], {target: nativeElement, animationComplete: noOpAnimationComplete});
 

packages/core/src/render3/interfaces/renderer_dom.ts

@@ -34,6 +34,11 @@ export interface RNode {
    */
   nextSibling: RNode | null;
 
+  /**
+   * Gets the Node immediately preceding this one in the parent's childNodes
+   */
+  previousSibling: RNode | null;
+
   /**
    * Insert a child node.
    *

packages/core/test/acceptance/animation_spec.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
+import {NgFor} from '@angular/common';
 import {ViewEncapsulation} from '@angular/compiler';
 import {
   AnimationCallbackEvent,
@@ -1247,5 +1248,148 @@ describe('Animation', () => {
       const paragraphs = fixture.debugElement.queryAll(By.css('p'));
       expect(paragraphs.length).toBe(1);
     }));
+
+    it('should always run animations for `@for` loops when adding and removing quickly', fakeAsync(() => {
+      const animateStyles = `
+        .slide-in {
+          animation: slide-in 500ms;
+        }
+        .fade {
+          animation: fade-out 500ms;
+        }
+        @keyframes slide-in {
+          from {
+            transform: translateX(-10px);
+          }
+          to {
+            transform: translateX(0);
+          }
+        }
+        @keyframes fade-out {
+          from {
+            opacity: 1;
+          }
+          to {
+            opacity: 0;
+          }
+        }
+      `;
+
+      @Component({
+        selector: 'test-cmp',
+        styles: animateStyles,
+        template: `
+          <div>
+            @for (item of items; track item) {
+              <p animate.enter="slide-in" animate.leave="fade" #el>I should slide in {{item}}.</p>
+            }
+          </div>
+        `,
+        encapsulation: ViewEncapsulation.None,
+      })
+      class TestComponent {
+        items = [1, 2, 3];
+        @ViewChild('el', {read: ElementRef}) el!: ElementRef<HTMLParagraphElement>;
+        max = 3;
+
+        addremove() {
+          this.max++;
+          this.items.splice(this.items.length, 0, this.max);
+          this.items.splice(0, 1);
+        }
+      }
+      TestBed.configureTestingModule({animationsEnabled: true});
+
+      const fixture = TestBed.createComponent(TestComponent);
+      const cmp = fixture.componentInstance;
+      fixture.detectChanges();
+      tickAnimationFrames(1);
+      const paragraphs = fixture.debugElement.queryAll(By.css('p'));
+      paragraphs.forEach((p) => {
+        p.nativeElement.dispatchEvent(new AnimationEvent('animationstart'));
+        p.nativeElement.dispatchEvent(
+          new AnimationEvent('animationend', {animationName: 'slide-in'}),
+        );
+      });
+      cmp.addremove();
+      fixture.detectChanges();
+      tickAnimationFrames(1);
+
+      expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1);
+      expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1);
+      expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
+    }));
+
+    it('should always run animations for custom repeater loops when adding and removing quickly', fakeAsync(() => {
+      const animateStyles = `
+        .slide-in {
+          animation: slide-in 500ms;
+        }
+        .fade {
+          animation: fade-out 500ms;
+        }
+        @keyframes slide-in {
+          from {
+            transform: translateX(-10px);
+          }
+          to {
+            transform: translateX(0);
+          }
+        }
+        @keyframes fade-out {
+          from {
+            opacity: 1;
+          }
+          to {
+            opacity: 0;
+          }
+        }
+      `;
+
+      @Component({
+        selector: 'test-cmp',
+        styles: animateStyles,
+        imports: [NgFor],
+        template: `
+          <div>
+            <ng-container *ngFor="let item of items; trackBy: trackByIndex; let i=index">
+              <p animate.enter="slide-in" animate.leave="fade" #el>I should slide in {{item}}.</p>
+            </ng-container>
+          </div>
+        `,
+        encapsulation: ViewEncapsulation.None,
+      })
+      class TestComponent {
+        items = [1, 2, 3];
+        @ViewChild('el', {read: ElementRef}) el!: ElementRef<HTMLParagraphElement>;
+        max = 3;
+
+        addremove() {
+          this.max++;
+          this.items.splice(this.items.length, 0, this.max);
+          this.items.splice(0, 1);
+        }
+      }
+      TestBed.configureTestingModule({animationsEnabled: true});
+
+      const fixture = TestBed.createComponent(TestComponent);
+      const cmp = fixture.componentInstance;
+      fixture.detectChanges();
+      tickAnimationFrames(1);
+      const paragraphs = fixture.debugElement.queryAll(By.css('p'));
+      paragraphs.forEach((p) => {
+        p.nativeElement.dispatchEvent(new AnimationEvent('animationstart'));
+        p.nativeElement.dispatchEvent(
+          new AnimationEvent('animationend', {animationName: 'slide-in'}),
+        );
+      });
+      cmp.addremove();
+      fixture.detectChanges();
+      tickAnimationFrames(1);
+
+      expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1);
+      expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1);
+      expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
+    }));
   });
 });