Skip to content

Commit 6c421ed

Browse files
thePunderWomanAndrewKushnir
thePunderWoman
authored and
AndrewKushnir
committed
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
1 parent 9093e0e commit 6c421ed

File tree

3 files changed

+182
-20
lines changed

3 files changed

+182
-20
lines changed

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

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,16 @@ import {
2020
getClassListFromValue,
2121
} from '../../animation/element_removal_registry';
2222
import {getLView, getCurrentTNode, getTView, getAnimationElementRemovalRegistry} from '../state';
23-
import {RENDERER, INJECTOR, CONTEXT, FLAGS, LViewFlags, LView, TView} from '../interfaces/view';
23+
import {
24+
RENDERER,
25+
INJECTOR,
26+
CONTEXT,
27+
FLAGS,
28+
LViewFlags,
29+
LView,
30+
TView,
31+
DECLARATION_LCONTAINER,
32+
} from '../interfaces/view';
2433
import {RuntimeError, RuntimeErrorCode} from '../../errors';
2534
import {getNativeByTNode, storeCleanupWithContext} from '../util/view_utils';
2635
import {performanceMarkFeature} from '../../util/performance';
@@ -30,6 +39,7 @@ import {NgZone} from '../../zone';
3039
import {assertDefined} from '../../util/assert';
3140
import {determineLongestAnimation, LongestAnimation} from '../../animation/longest_animation';
3241
import {TNode} from '../interfaces/node';
42+
import {getBeforeNodeForView} from '../node_manipulation';
3343

3444
const DEFAULT_ANIMATIONS_DISABLED = false;
3545
const areAnimationSupported =
@@ -101,15 +111,25 @@ function clearLeavingNodes(tNode: TNode, el: HTMLElement): void {
101111

102112
/**
103113
* In the case that we have an existing node that's animating away, like when
104-
* an `@if` toggles quickly or `@for` adds and removes elements quickly, we
105-
* need to end the animation for the former node and remove it right away to
106-
* prevent duplicate nodes showing up.
114+
* an `@if` toggles quickly, we need to end the animation for the former node
115+
* and remove it right away to prevent duplicate nodes showing up.
107116
*/
108-
function cancelLeavingNodes(tNode: TNode, el: HTMLElement): void {
109-
leavingNodes
110-
.get(tNode)
111-
?.pop()
112-
?.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
117+
function cancelLeavingNodes(tNode: TNode, lView: LView): void {
118+
const leavingEl = leavingNodes.get(tNode)?.shift();
119+
const lContainer = lView[DECLARATION_LCONTAINER];
120+
if (lContainer) {
121+
// this is the insertion point for the new TNode element.
122+
// it will be inserted before the declaring containers anchor.
123+
const beforeNode = getBeforeNodeForView(tNode.index, lContainer);
124+
// here we need to check the previous sibling of that anchor
125+
const previousNode = beforeNode?.previousSibling;
126+
// We really only want to cancel animations if the leaving node is the
127+
// same as the node before where the new node will be inserted. This is
128+
// the control flow scenario where an if was toggled.
129+
if (leavingEl && previousNode && leavingEl === previousNode) {
130+
leavingEl.dispatchEvent(new CustomEvent('animationend', {detail: {cancel: true}}));
131+
}
132+
}
113133
}
114134

115135
function trackLeavingNodes(tNode: TNode, el: HTMLElement): void {
@@ -178,6 +198,8 @@ export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEn
178198
cleanupFns.push(renderer.listen(nativeElement, 'transitionstart', handleAnimationStart));
179199
});
180200

201+
cancelLeavingNodes(tNode, lView);
202+
181203
trackEnterClasses(nativeElement, activeClasses, cleanupFns);
182204

183205
for (const klass of activeClasses) {
@@ -188,12 +210,6 @@ export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEn
188210
// preventing an animation via selector specificity.
189211
ngZone.runOutsideAngular(() => {
190212
requestAnimationFrame(() => {
191-
// In the case that we have an existing node that's animating away, like when
192-
// an `@if` toggles quickly or `@for` adds and removes elements quickly, we
193-
// need to end the animation for the former node and remove it right away to
194-
// prevent duplicate nodes showing up.
195-
cancelLeavingNodes(tNode, nativeElement);
196-
197213
determineLongestAnimation(nativeElement, longestAnimations, areAnimationSupported);
198214
if (!longestAnimations.has(nativeElement)) {
199215
for (const klass of activeClasses) {
@@ -253,11 +269,8 @@ export function ɵɵanimateEnterListener(value: AnimationFunction): typeof ɵɵa
253269

254270
const tNode = getCurrentTNode()!;
255271
const nativeElement = getNativeByTNode(tNode, lView) as HTMLElement;
256-
// In the case that we have an existing node that's animating away, like when
257-
// an `@if` toggles quickly or `@for` adds and removes elements quickly, we
258-
// need to end the animation for the former node and remove it right away to
259-
// prevent duplicate nodes showing up.
260-
cancelLeavingNodes(tNode, nativeElement);
272+
273+
cancelLeavingNodes(tNode, lView);
261274

262275
value.call(lView[CONTEXT], {target: nativeElement, animationComplete: noOpAnimationComplete});
263276

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,11 @@ export interface RNode {
3434
*/
3535
nextSibling: RNode | null;
3636

37+
/**
38+
* Gets the Node immediately preceding this one in the parent's childNodes
39+
*/
40+
previousSibling: RNode | null;
41+
3742
/**
3843
* Insert a child node.
3944
*

‎packages/core/test/acceptance/animation_spec.ts‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {NgFor} from '@angular/common';
910
import {ViewEncapsulation} from '@angular/compiler';
1011
import {
1112
AnimationCallbackEvent,
@@ -1247,5 +1248,148 @@ describe('Animation', () => {
12471248
const paragraphs = fixture.debugElement.queryAll(By.css('p'));
12481249
expect(paragraphs.length).toBe(1);
12491250
}));
1251+
1252+
it('should always run animations for `@for` loops when adding and removing quickly', fakeAsync(() => {
1253+
const animateStyles = `
1254+
.slide-in {
1255+
animation: slide-in 500ms;
1256+
}
1257+
.fade {
1258+
animation: fade-out 500ms;
1259+
}
1260+
@keyframes slide-in {
1261+
from {
1262+
transform: translateX(-10px);
1263+
}
1264+
to {
1265+
transform: translateX(0);
1266+
}
1267+
}
1268+
@keyframes fade-out {
1269+
from {
1270+
opacity: 1;
1271+
}
1272+
to {
1273+
opacity: 0;
1274+
}
1275+
}
1276+
`;
1277+
1278+
@Component({
1279+
selector: 'test-cmp',
1280+
styles: animateStyles,
1281+
template: `
1282+
<div>
1283+
@for (item of items; track item) {
1284+
<p animate.enter="slide-in" animate.leave="fade" #el>I should slide in {{item}}.</p>
1285+
}
1286+
</div>
1287+
`,
1288+
encapsulation: ViewEncapsulation.None,
1289+
})
1290+
class TestComponent {
1291+
items = [1, 2, 3];
1292+
@ViewChild('el', {read: ElementRef}) el!: ElementRef<HTMLParagraphElement>;
1293+
max = 3;
1294+
1295+
addremove() {
1296+
this.max++;
1297+
this.items.splice(this.items.length, 0, this.max);
1298+
this.items.splice(0, 1);
1299+
}
1300+
}
1301+
TestBed.configureTestingModule({animationsEnabled: true});
1302+
1303+
const fixture = TestBed.createComponent(TestComponent);
1304+
const cmp = fixture.componentInstance;
1305+
fixture.detectChanges();
1306+
tickAnimationFrames(1);
1307+
const paragraphs = fixture.debugElement.queryAll(By.css('p'));
1308+
paragraphs.forEach((p) => {
1309+
p.nativeElement.dispatchEvent(new AnimationEvent('animationstart'));
1310+
p.nativeElement.dispatchEvent(
1311+
new AnimationEvent('animationend', {animationName: 'slide-in'}),
1312+
);
1313+
});
1314+
cmp.addremove();
1315+
fixture.detectChanges();
1316+
tickAnimationFrames(1);
1317+
1318+
expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1);
1319+
expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1);
1320+
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
1321+
}));
1322+
1323+
it('should always run animations for custom repeater loops when adding and removing quickly', fakeAsync(() => {
1324+
const animateStyles = `
1325+
.slide-in {
1326+
animation: slide-in 500ms;
1327+
}
1328+
.fade {
1329+
animation: fade-out 500ms;
1330+
}
1331+
@keyframes slide-in {
1332+
from {
1333+
transform: translateX(-10px);
1334+
}
1335+
to {
1336+
transform: translateX(0);
1337+
}
1338+
}
1339+
@keyframes fade-out {
1340+
from {
1341+
opacity: 1;
1342+
}
1343+
to {
1344+
opacity: 0;
1345+
}
1346+
}
1347+
`;
1348+
1349+
@Component({
1350+
selector: 'test-cmp',
1351+
styles: animateStyles,
1352+
imports: [NgFor],
1353+
template: `
1354+
<div>
1355+
<ng-container *ngFor="let item of items; trackBy: trackByIndex; let i=index">
1356+
<p animate.enter="slide-in" animate.leave="fade" #el>I should slide in {{item}}.</p>
1357+
</ng-container>
1358+
</div>
1359+
`,
1360+
encapsulation: ViewEncapsulation.None,
1361+
})
1362+
class TestComponent {
1363+
items = [1, 2, 3];
1364+
@ViewChild('el', {read: ElementRef}) el!: ElementRef<HTMLParagraphElement>;
1365+
max = 3;
1366+
1367+
addremove() {
1368+
this.max++;
1369+
this.items.splice(this.items.length, 0, this.max);
1370+
this.items.splice(0, 1);
1371+
}
1372+
}
1373+
TestBed.configureTestingModule({animationsEnabled: true});
1374+
1375+
const fixture = TestBed.createComponent(TestComponent);
1376+
const cmp = fixture.componentInstance;
1377+
fixture.detectChanges();
1378+
tickAnimationFrames(1);
1379+
const paragraphs = fixture.debugElement.queryAll(By.css('p'));
1380+
paragraphs.forEach((p) => {
1381+
p.nativeElement.dispatchEvent(new AnimationEvent('animationstart'));
1382+
p.nativeElement.dispatchEvent(
1383+
new AnimationEvent('animationend', {animationName: 'slide-in'}),
1384+
);
1385+
});
1386+
cmp.addremove();
1387+
fixture.detectChanges();
1388+
tickAnimationFrames(1);
1389+
1390+
expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1);
1391+
expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1);
1392+
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
1393+
}));
12501394
});
12511395
});

0 commit comments

Comments
 (0)