fix(animations): don't consume instructions for animateChild

[dario-piotrowicz]

• TODO: Fill body commit message if PR gets accepted

resolves #41483
resolves #30693

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: #41483 (also #30693)

Animations which include animateChild calls can fail to remove elements from the DOM as their players are ignored since they are unnecessary for the actual animations

What is the new behavior?

The aforementioned players are handled as skipped instead of ignoring them, making their callbacks/cleanups take place and removing the issue of not removing elements from the DOM

Does this PR introduce a breaking change?

• Yes
• No

[dario-piotrowicz]

I just wanted to give some context for the reason for this change

See this stackblitz, if you try transitioning between One and Two you'll see that two elements are incorrectly being shown, you can also see that the main element gets the ng-animated-queued class which doesn't get removed:

The issue here is that animateChild removes the subInstruction for the player relative to the element making the call, effectively ignoring the player. This "ignores" the player as it is not necessary for the animation.

The real problem comes from the fact that the ignored player's callbacks do not get triggered, and that prevents the ng-animate-queued class from being removed (the class is added during trigger initialization), this also blocks the engine.processLeaveNode from being executed (as one of its players is ignored thus never completes):

angular/packages/animations/browser/src/render/transition_animation_engine.ts

Lines 1748 to 1751 in 155742e

	function removeNodesAfterAnimationDone(
	engine: TransitionAnimationEngine, element: any, players: AnimationPlayer[]) {
	optimizeGroupPlayer(players).onDone(() => engine.processLeaveNode(element));
	}

(
note: we don't accumulated dead elements as they are cleaned up during the next trigger initialization via the old player's destruction:

angular/packages/animations/browser/src/render/transition_animation_engine.ts

Lines 248 to 250 in 155742e

	if (player.namespaceId == this.id && player.triggerName == triggerName && player.queued) {
	player.destroy();
	}

)

so I believe that the players should not be completely ignored as their callbacks are still necessary, with my changes thus we end up having more players on the animations' flush, but the extra ones are correctly marked as skipped:

angular/packages/animations/browser/src/render/transition_animation_engine.ts

Lines 1187 to 1191 in 155742e

	const parentPlayers = this.playersByElement.get(parentWithAnimation);
	if (parentPlayers && parentPlayers.length) {
	player.parentPlayer = optimizeGroupPlayer(parentPlayers);
	}
	skippedPlayers.push(player);

skipped players still trigger their callbacks/cleanups without effecting the animation:

angular/packages/animations/browser/src/render/transition_animation_engine.ts

Lines 1217 to 1226 in 155742e

	// the reason why we don't actually play the animation is
	// because all that a skipped player is designed to do is to
	// fire the start/done transition callback events
	skippedPlayers.forEach(player => {
	if (player.parentPlayer) {
	player.syncPlayerEvents(player.parentPlayer);
	} else {
	player.destroy();
	}
	});

the only downside I can see here is that we end up with more players (even though skipped) in the animations' flush (as you can see from the updated unit tests), which I don't think makes much of a difference, and it does make sense because those players do exist and get skipped (from a user perspective, I don't think extra callbacks are called as those players are not associated with triggers, so there shouldn't be much of a difference there too)

[dario-piotrowicz]

I just noticed that this change also fixes #30693 which looks very broken in the current state 🙂
(I haven't looked too much into it, but there the elements never actually get cleaned up so they keep stacking up)

[jessicajaniuk]

I love that this fixes the two bugs. I think we just need some additional code comments and variable name updates to help clarify what's happening here.

[dario-piotrowicz]

@jessicajaniuk thanks a lot for having a look at this!!!! 😍

I didn't add names to the players in the tests as they are sort of throw-away players (they are indeed skipped), but I do agree that it would be much nicer and clearer to give them more meaning and context, especially for someone reading the tests in the future

I'll fix it, thanks 🙂👍

[dario-piotrowicz]

@jessicajaniuk I've renamed the players and added comments before them explaining the acronyms, I hope this is ok (hopefully I am not being too verbose? 😓)

please have another look and tell me what you think 🙂

[dario-piotrowicz]

Not sure why this last classList wasn't being checked 🤔 , I've added the check for good measures 🙂

[dario-piotrowicz]

this skipping/ignoring doesn't convince me, it may cause similar issues of missed cleanup calls 🤔 , I think that if/after this PR gets merged we should also look into this one and check if everything is ok 🤔

[jessicajaniuk]

LGTM 🍪

Thanks for adding all the additional comments and clarifications here. I think it makes a big difference!

reviewed-for: fw-core, fw-animations

[dario-piotrowicz]

@jessicajaniuk sorry if it's a bit last minute 😅, I realized that it would be good to a unit test for this functionality, are you ok with us merging this PR now and me adding the unit test in a separate one? 🙂

[alxhub]

This PR was merged into the repository by commit 38ddae1.

[jessicajaniuk]

@jessicajaniuk sorry if it's a bit last minute 😅, I realized that it would be good to a unit test for this functionality, are you ok with us merging this PR now and me adding the unit test in a separate one? 🙂

Yup! :)

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}