Skip to content

Commit a4001c4

Browse files
thePunderWomanthePunderWoman
committed
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
1 parent 464bff9 commit a4001c4

File tree

5 files changed

+101
-8
lines changed

5 files changed

+101
-8
lines changed

‎packages/core/src/animation/interfaces.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,12 @@ export interface AnimationLViewData {
8585

8686
// Leave animations that apply to nodes in this view
8787
running?: Promise<PromiseSettledResult<void>[]>;
88+
89+
// Skip leave animations
90+
// This flag is solely used when move operations occur. DOM Node move
91+
// operations occur in lists, like `@for` loops, and use the same code
92+
// path during move that detaching or removing does. So to prevent
93+
// unexpected disappearing of moving nodes, we use this flag to skip
94+
// the animations in that case.
95+
skipLeaveAnimations?: boolean;
8896
}

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container';
2323
import {ComponentTemplate} from '../interfaces/definition';
2424
import {LocalRefExtractor, TAttributes, TNode, TNodeFlags} from '../interfaces/node';
2525
import {
26+
ANIMATIONS,
2627
CONTEXT,
2728
DECLARATION_COMPONENT_VIEW,
2829
HEADER_OFFSET,
@@ -46,6 +47,7 @@ import {
4647
removeLViewFromLContainer,
4748
} from '../view/container';
4849
import {removeDehydratedViews} from '../../hydration/cleanup';
50+
import {AnimationLViewData} from '../../animation/interfaces';
4951

5052
/**
5153
* Creates an LContainer for an ng-template representing a root node
@@ -418,8 +420,9 @@ class LiveCollectionLContainerImpl extends LiveCollection<
418420
shouldAddViewToDom(this.templateTNode, dehydratedView),
419421
);
420422
}
421-
override detach(index: number): LView<RepeaterContext<unknown>> {
423+
override detach(index: number, skipLeaveAnimations?: boolean): LView<RepeaterContext<unknown>> {
422424
this.needsIndexUpdate ||= index !== this.length - 1;
425+
if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index);
423426
return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
424427
}
425428
override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -567,6 +570,16 @@ function getLContainer(lView: LView, index: number): LContainer {
567570
return lContainer;
568571
}
569572

573+
function setSkipLeaveAnimations(lContainer: LContainer, index: number): void {
574+
if (lContainer.length <= CONTAINER_HEADER_OFFSET) return;
575+
576+
const indexInContainer = CONTAINER_HEADER_OFFSET + index;
577+
const viewToDetach = lContainer[indexInContainer];
578+
if (viewToDetach && viewToDetach[ANIMATIONS]) {
579+
(viewToDetach[ANIMATIONS] as AnimationLViewData).skipLeaveAnimations = true;
580+
}
581+
}
582+
570583
function detachExistingView<T>(lContainer: LContainer, index: number): LView<T> {
571584
const existingLView = detachView(lContainer, index);
572585
ngDevMode && assertLView(existingLView);

‎packages/core/src/render3/list_reconciliation.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ export abstract class LiveCollection<T, V> {
2020
abstract get length(): number;
2121
abstract at(index: number): V;
2222
abstract attach(index: number, item: T): void;
23-
abstract detach(index: number): T;
23+
abstract detach(index: number, skipLeaveAnimations?: boolean): T;
2424
abstract create(index: number, value: V): T;
2525
destroy(item: T): void {
2626
// noop by default
@@ -45,7 +45,11 @@ export abstract class LiveCollection<T, V> {
4545
}
4646
}
4747
move(prevIndex: number, newIdx: number): void {
48-
this.attach(newIdx, this.detach(prevIndex));
48+
// For move operations, the detach code path is the same one used for removing
49+
// DOM nodes, which would trigger `animate.leave` bindings. We need to skip
50+
// those animations in the case of a move operation so the moving elements don't
51+
// unexpectedly disappear.
52+
this.attach(newIdx, this.detach(prevIndex, true /* skipLeaveAnimations */));
4953
}
5054
}
5155

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -352,12 +352,18 @@ function cleanUpView(tView: TView, lView: LView): void {
352352

353353
function runLeaveAnimationsWithCallback(lView: LView | undefined, callback: Function) {
354354
if (lView && lView[ANIMATIONS] && lView[ANIMATIONS].leave) {
355-
const runningAnimations = [];
356-
for (let animateFn of lView[ANIMATIONS].leave) {
357-
runningAnimations.push(animateFn());
355+
if (lView[ANIMATIONS].skipLeaveAnimations) {
356+
lView[ANIMATIONS].skipLeaveAnimations = false;
357+
} else {
358+
const leaveAnimations = lView[ANIMATIONS].leave;
359+
const runningAnimations = [];
360+
for (let index = 0; index < leaveAnimations.length; index++) {
361+
const animateFn = leaveAnimations[index];
362+
runningAnimations.push(animateFn());
363+
}
364+
lView[ANIMATIONS].running = Promise.allSettled(runningAnimations);
365+
lView[ANIMATIONS].leave = undefined;
358366
}
359-
lView[ANIMATIONS].running = Promise.allSettled(runningAnimations);
360-
lView[ANIMATIONS].leave = undefined;
361367
}
362368
runAfterLeaveAnimations(lView, callback);
363369
}

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {ViewEncapsulation} from '@angular/compiler';
1111
import {
1212
AnimationCallbackEvent,
1313
Component,
14+
computed,
1415
Directive,
1516
ElementRef,
1617
NgModule,
@@ -1539,5 +1540,66 @@ describe('Animation', () => {
15391540
expect(fixture.debugElement.queryAll(By.css('p.slide-in')).length).toBe(1);
15401541
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
15411542
}));
1543+
1544+
it('should only remove one element in reactive `@for` loops when removing the second to last item', fakeAsync(() => {
1545+
const animateStyles = `
1546+
.fade {
1547+
animation: fade-out 500ms;
1548+
}
1549+
@keyframes fade-out {
1550+
from {
1551+
opacity: 1;
1552+
}
1553+
to {
1554+
opacity: 0;
1555+
}
1556+
}
1557+
`;
1558+
1559+
@Component({
1560+
selector: 'test-cmp',
1561+
styles: animateStyles,
1562+
template: `
1563+
<div>
1564+
@for (item of shown(); track item) {
1565+
<p animate.leave="fade" #el>I should slide in {{item}}.</p>
1566+
}
1567+
</div>
1568+
`,
1569+
encapsulation: ViewEncapsulation.None,
1570+
})
1571+
class TestComponent {
1572+
items = signal([1, 2, 3, 4, 5, 6]);
1573+
shown = computed(() => this.items().slice(0, 3));
1574+
@ViewChild('el', {read: ElementRef}) el!: ElementRef<HTMLParagraphElement>;
1575+
max = 6;
1576+
1577+
removeSecondToLast() {
1578+
this.items.update((old) => {
1579+
const newList = [...old];
1580+
newList.splice(1, 1);
1581+
return newList;
1582+
});
1583+
}
1584+
}
1585+
TestBed.configureTestingModule({animationsEnabled: true});
1586+
1587+
const fixture = TestBed.createComponent(TestComponent);
1588+
const cmp = fixture.componentInstance;
1589+
fixture.detectChanges();
1590+
cmp.removeSecondToLast();
1591+
fixture.detectChanges();
1592+
tickAnimationFrames(1);
1593+
1594+
expect(fixture.debugElement.queryAll(By.css('p.fade')).length).toBe(1);
1595+
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(4);
1596+
fixture.debugElement
1597+
.query(By.css('p.fade'))
1598+
.nativeElement.dispatchEvent(
1599+
new AnimationEvent('animationend', {animationName: 'fade-out'}),
1600+
);
1601+
tick();
1602+
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3);
1603+
}));
15421604
});
15431605
});

0 commit comments

Comments
 (0)