Skip to content

Commit dcdd1bc

Browse files
thePunderWomanthePunderWoman
committed
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: #64818 fixes: #64730
1 parent d6ef181 commit dcdd1bc

File tree

6 files changed

+133
-46
lines changed

6 files changed

+133
-46
lines changed

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ const MAX_ANIMATION_TIMEOUT_DEFAULT = 4000;
5252
*/
5353
export type AnimationFunction = (event: AnimationCallbackEvent) => void;
5454

55-
export type RunEnterAnimationFn = () => void;
55+
export type RunEnterAnimationFn = VoidFunction;
5656
export type RunLeaveAnimationFn = () => {promise: Promise<void>; resolve: VoidFunction};
5757

5858
export interface LongestAnimation {
@@ -81,13 +81,11 @@ export interface AnimationLViewData {
8181
// We chose to use unknown instead of PromiseSettledResult<void> to avoid requiring the type
8282
running?: Promise<unknown>;
8383

84-
// Skip leave animations
85-
// This flag is solely used when move operations occur. DOM Node move
86-
// operations occur in lists, like `@for` loops, and use the same code
87-
// path during move that detaching or removing does. So to prevent
88-
// unexpected disappearing of moving nodes, we use this flag to skip
89-
// the animations in that case.
90-
skipLeaveAnimations?: boolean;
84+
// Animation functions that have been queued for this view when the view is detached.
85+
// This is used to later remove them from the global animation queue if the view
86+
// is attached before the animation queue runs. This is used in cases where views are
87+
// moved or swapped during list reconciliation.
88+
detachedLeaveAnimationFns?: VoidFunction[];
9189
}
9290

9391
/**

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {afterNextRender} from '../render3/after_render/hooks';
1010
import {InjectionToken, Injector} from '../di';
11-
import {EnterNodeAnimations} from './interfaces';
11+
import {AnimationLViewData, EnterNodeAnimations} from './interfaces';
1212

1313
export interface AnimationQueue {
1414
queue: Set<VoidFunction>;
@@ -36,18 +36,37 @@ export const ANIMATION_QUEUE = new InjectionToken<AnimationQueue>(
3636
export function addToAnimationQueue(
3737
injector: Injector,
3838
animationFns: VoidFunction | VoidFunction[],
39+
animationData?: AnimationLViewData,
3940
) {
4041
const animationQueue = injector.get(ANIMATION_QUEUE);
4142
if (Array.isArray(animationFns)) {
4243
for (const animateFn of animationFns) {
4344
animationQueue.queue.add(animateFn);
45+
// If a node is detached, we need to keep track of the queued animation functions
46+
// so we can later remove them from the global animation queue if the view
47+
// is re-attached before the animation queue runs.
48+
animationData?.detachedLeaveAnimationFns?.push(animateFn);
4449
}
4550
} else {
4651
animationQueue.queue.add(animationFns);
52+
// If a node is detached, we need to keep track of the queued animation functions
53+
// so we can later remove them from the global animation queue if the view
54+
// is re-attached before the animation queue runs.
55+
animationData?.detachedLeaveAnimationFns?.push(animationFns);
4756
}
4857
animationQueue.scheduler && animationQueue.scheduler(injector);
4958
}
5059

60+
export function removeFromAnimationQueue(injector: Injector, animationData: AnimationLViewData) {
61+
const animationQueue = injector.get(ANIMATION_QUEUE);
62+
if (animationData.detachedLeaveAnimationFns) {
63+
for (const animationFn of animationData.detachedLeaveAnimationFns) {
64+
animationQueue.queue.delete(animationFn);
65+
}
66+
animationData.detachedLeaveAnimationFns = undefined;
67+
}
68+
}
69+
5170
export function scheduleAnimationQueue(injector: Injector) {
5271
const animationQueue = injector.get(ANIMATION_QUEUE);
5372
// We only want to schedule the animation queue if it hasn't already been scheduled.

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

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
DECLARATION_COMPONENT_VIEW,
2929
HEADER_OFFSET,
3030
HYDRATION,
31+
INJECTOR,
3132
LView,
3233
TVIEW,
3334
TView,
@@ -39,15 +40,17 @@ import {NO_CHANGE} from '../tokens';
3940
import {getConstant, getTNode} from '../util/view_utils';
4041
import {createAndRenderEmbeddedLView, shouldAddViewToDom} from '../view_manipulation';
4142

42-
import {declareNoDirectiveHostTemplate} from './template';
43+
import {AnimationLViewData} from '../../animation/interfaces';
44+
import {removeDehydratedViews} from '../../hydration/cleanup';
4345
import {
4446
addLViewToLContainer,
4547
detachView,
4648
getLViewFromLContainer,
4749
removeLViewFromLContainer,
4850
} from '../view/container';
49-
import {removeDehydratedViews} from '../../hydration/cleanup';
50-
import {AnimationLViewData} from '../../animation/interfaces';
51+
import {declareNoDirectiveHostTemplate} from './template';
52+
import {removeFromAnimationQueue} from '../../animation/queue';
53+
import {allLeavingAnimations} from '../../animation/longest_animation';
5154

5255
/**
5356
* Creates an LContainer for an ng-template representing a root node
@@ -419,10 +422,11 @@ class LiveCollectionLContainerImpl extends LiveCollection<
419422
index,
420423
shouldAddViewToDom(this.templateTNode, dehydratedView),
421424
);
425+
clearDetachAnimationList(this.lContainer, index);
422426
}
423-
override detach(index: number, skipLeaveAnimations?: boolean): LView<RepeaterContext<unknown>> {
427+
override detach(index: number): LView<RepeaterContext<unknown>> {
424428
this.needsIndexUpdate ||= index !== this.length - 1;
425-
if (skipLeaveAnimations) setSkipLeaveAnimations(this.lContainer, index);
429+
maybeInitDetachAnimationList(this.lContainer, index);
426430
return detachExistingView<RepeaterContext<unknown>>(this.lContainer, index);
427431
}
428432
override create(index: number, value: unknown): LView<RepeaterContext<unknown>> {
@@ -570,13 +574,39 @@ function getLContainer(lView: LView, index: number): LContainer {
570574
return lContainer;
571575
}
572576

573-
function setSkipLeaveAnimations(lContainer: LContainer, index: number): void {
577+
function clearDetachAnimationList(lContainer: LContainer, index: number): void {
578+
if (lContainer.length <= CONTAINER_HEADER_OFFSET) return;
579+
580+
const indexInContainer = CONTAINER_HEADER_OFFSET + index;
581+
const viewToDetach = lContainer[indexInContainer];
582+
const animations = viewToDetach
583+
? (viewToDetach[ANIMATIONS] as AnimationLViewData | undefined)
584+
: undefined;
585+
if (
586+
viewToDetach &&
587+
animations &&
588+
animations.detachedLeaveAnimationFns &&
589+
animations.detachedLeaveAnimationFns.length > 0
590+
) {
591+
const injector = viewToDetach[INJECTOR];
592+
removeFromAnimationQueue(injector, animations);
593+
allLeavingAnimations.delete(viewToDetach);
594+
animations.detachedLeaveAnimationFns = undefined;
595+
}
596+
}
597+
598+
// Initialize the detach leave animation list for a view about to be detached, but only
599+
// if it has leave animations.
600+
function maybeInitDetachAnimationList(lContainer: LContainer, index: number): void {
574601
if (lContainer.length <= CONTAINER_HEADER_OFFSET) return;
575602

576603
const indexInContainer = CONTAINER_HEADER_OFFSET + index;
577604
const viewToDetach = lContainer[indexInContainer];
578-
if (viewToDetach && viewToDetach[ANIMATIONS]) {
579-
(viewToDetach[ANIMATIONS] as AnimationLViewData).skipLeaveAnimations = true;
605+
const animations = viewToDetach
606+
? (viewToDetach[ANIMATIONS] as AnimationLViewData | undefined)
607+
: undefined;
608+
if (animations && animations.leave && animations.leave.size > 0) {
609+
animations.detachedLeaveAnimationFns = [];
580610
}
581611
}
582612

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

Lines changed: 2 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, skipLeaveAnimations?: boolean): T;
23+
abstract detach(index: number): T;
2424
abstract create(index: number, value: V): T;
2525
destroy(item: T): void {
2626
// noop by default
@@ -49,7 +49,7 @@ export abstract class LiveCollection<T, V> {
4949
// DOM nodes, which would trigger `animate.leave` bindings. We need to skip
5050
// those animations in the case of a move operation so the moving elements don't
5151
// unexpectedly disappear.
52-
this.attach(newIdx, this.detach(prevIndex, true /* skipLeaveAnimations */));
52+
this.attach(newIdx, this.detach(prevIndex));
5353
}
5454
}
5555

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

Lines changed: 26 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ import {getLViewParent, getNativeByTNode, unwrapRNode} from './util/view_utils';
8484
import {allLeavingAnimations} from '../animation/longest_animation';
8585
import {Injector} from '../di';
8686
import {addToAnimationQueue, queueEnterAnimations} from '../animation/queue';
87+
import {RunLeaveAnimationFn} from '../animation/interfaces';
8788

8889
const enum WalkTNodeTreeAction {
8990
/** node create in the native environment. Run on initial creation. */
@@ -386,37 +387,35 @@ function runLeaveAnimationsWithCallback(
386387
if (animations == null || animations.leave == undefined || !animations.leave.has(tNode.index))
387388
return callback(false);
388389

389-
// this is solely for move operations to prevent leave animations from running
390-
// on the moved nodes, which would have deleted the node.
391-
if (animations.skipLeaveAnimations) {
392-
animations.skipLeaveAnimations = false;
393-
return callback(false);
394-
}
395-
396390
if (lView) allLeavingAnimations.add(lView);
397391

398-
addToAnimationQueue(injector, () => {
399-
// it's possible that in the time between when the leave animation was
400-
// and the time it was executed, the data structure changed. So we need
401-
// to be safe here.
402-
if (animations.leave && animations.leave.has(tNode.index)) {
403-
const leaveAnimationMap = animations.leave;
404-
const leaveAnimations = leaveAnimationMap.get(tNode.index);
405-
const runningAnimations = [];
406-
if (leaveAnimations) {
407-
for (let index = 0; index < leaveAnimations.animateFns.length; index++) {
408-
const animationFn = leaveAnimations.animateFns[index];
409-
const {promise} = animationFn();
410-
runningAnimations.push(promise);
392+
addToAnimationQueue(
393+
injector,
394+
() => {
395+
// it's possible that in the time between when the leave animation was
396+
// and the time it was executed, the data structure changed. So we need
397+
// to be safe here.
398+
if (animations.leave && animations.leave.has(tNode.index)) {
399+
const leaveAnimationMap = animations.leave;
400+
const leaveAnimations = leaveAnimationMap.get(tNode.index);
401+
const runningAnimations = [];
402+
if (leaveAnimations) {
403+
for (let index = 0; index < leaveAnimations.animateFns.length; index++) {
404+
const animationFn = leaveAnimations.animateFns[index];
405+
const {promise} = animationFn() as ReturnType<RunLeaveAnimationFn>;
406+
runningAnimations.push(promise);
407+
}
408+
animations.detachedLeaveAnimationFns = undefined;
411409
}
410+
animations.running = Promise.allSettled(runningAnimations);
411+
runAfterLeaveAnimations(lView!, callback);
412+
} else {
413+
if (lView) allLeavingAnimations.delete(lView);
414+
callback(false);
412415
}
413-
animations.running = Promise.allSettled(runningAnimations);
414-
runAfterLeaveAnimations(lView!, callback);
415-
} else {
416-
if (lView) allLeavingAnimations.delete(lView);
417-
callback(false);
418-
}
419-
});
416+
},
417+
animations,
418+
);
420419
}
421420

422421
function runAfterLeaveAnimations(lView: LView, callback: Function) {

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2057,6 +2057,47 @@ describe('Animation', () => {
20572057
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3);
20582058
}));
20592059

2060+
it('should not remove elements when swapping or moving nodes', fakeAsync(() => {
2061+
const animateSpy = jasmine.createSpy('animateSpy');
2062+
@Component({
2063+
selector: 'test-cmp',
2064+
template: `
2065+
<div>
2066+
@for (item of items; track item.id) {
2067+
<p (animate.leave)="animate($event)" #el>{{ item.id }}</p>
2068+
}
2069+
</div>
2070+
`,
2071+
encapsulation: ViewEncapsulation.None,
2072+
})
2073+
class TestComponent {
2074+
items = [{id: 1}, {id: 2}, {id: 3}];
2075+
private cd = inject(ChangeDetectorRef);
2076+
2077+
animate(event: AnimationCallbackEvent) {
2078+
animateSpy();
2079+
event.animationComplete();
2080+
}
2081+
2082+
shuffle() {
2083+
this.items = this.shuffleArray(this.items);
2084+
this.cd.markForCheck();
2085+
}
2086+
2087+
shuffleArray<T>(array: readonly T[]): T[] {
2088+
return [array[1], array[2], array[0]];
2089+
}
2090+
}
2091+
TestBed.configureTestingModule({animationsEnabled: true});
2092+
2093+
const fixture = TestBed.createComponent(TestComponent);
2094+
const cmp = fixture.componentInstance;
2095+
cmp.shuffle();
2096+
fixture.detectChanges();
2097+
expect(animateSpy).not.toHaveBeenCalled();
2098+
expect(fixture.debugElement.queryAll(By.css('p')).length).toBe(3);
2099+
}));
2100+
20602101
it('should not remove elements when child element animations finish', fakeAsync(() => {
20612102
const animateStyles = `
20622103
.fade {

0 commit comments

Comments
 (0)