Skip to content

feat(core): add enter and leave animation instructions #62682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thePunderWoman
Copy link
Contributor

This adds the instructions to support enter and leave animations on nodes.

Does this PR introduce a breaking change?

  • Yes
  • No

@thePunderWoman thePunderWoman added area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release labels Jul 17, 2025
@ngbot ngbot bot added this to the Backlog milestone Jul 17, 2025
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jul 17, 2025
@thePunderWoman thePunderWoman force-pushed the animate-instructions branch 2 times, most recently from 843db01 to 46066a1 Compare July 17, 2025 11:04
@pullapprove pullapprove bot requested review from crisbeto, kirjs and mmalerba July 17, 2025 11:04
@thePunderWoman thePunderWoman force-pushed the animate-instructions branch 2 times, most recently from 3c38d4a to 056dcb3 Compare July 17, 2025 11:53
Copy link

github-actions bot commented Jul 17, 2025

Deployed adev-preview for 85f73c2 to: https://ng-dev-previews-fw--pr-angular-angular-62682-adev-prev-yvy73xs1.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

animate(el: Element, removeFn: Function): void {
if (!this.outElements.has(el)) return;
const details = this.outElements.get(el)!;
const timeout = setTimeout(() => removeFn(), 4000);
Copy link
Member

@JeanMeche JeanMeche Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4000 feels like a magic number here. Can we use a named a named constant and/or have a comment on why this value. (Would animations longer than 4s just get removed after ther 4s ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly. I've added a const and associated comment about why 4 seconds.

Copy link
Contributor Author

@thePunderWoman thePunderWoman Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also to quickly answer your question here, if you're using an animation function (rather than classes) for animate.leave, you are required to call the complete function to tell the framework when to remove the element. The feedback on the RFC was to have a timeout for safety, and so I went with the same timeout duration as cross document view transitions. So after a 4 second timeout, the element gets removed. If your animation is done before that, no issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we can have an injection token that configures it? And the default would still be 4s.

@thePunderWoman thePunderWoman force-pushed the animate-instructions branch 2 times, most recently from 72ef660 to 4d2fc63 Compare July 17, 2025 14:34
@thePunderWoman thePunderWoman added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 17, 2025
@@ -667,6 +679,23 @@ export class ElementRef<T = any> {
nativeElement: T;
}

// @public
export class ElementRegistry {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this symbol in the public API, was it intentionally exposed?

@@ -96,6 +97,7 @@
"EffectRefImpl",
"EffectScheduler",
"ElementRef",
"ElementRegistry",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unexpected, should we try making it tree-shakable (maybe via using lightweight injection token in the non-tree-shakable part and providing an actual implementation once the feature is brought it via special instructions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how we would be able to do this since there's no provider function for Animations.

Copy link
Member

@JeanMeche JeanMeche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the features prop, like we did for standalone in the past (#58288 removed it)

*
* @codeGenApi
*/
export function ɵɵanimateEnter(value: string | Function): typeof ɵɵanimateEnter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is pretty long, so I was thinking that it might benefit from either having more docs, explaining the overall logic and/or refactor the logic and extract it into multiple functions (I suspect that some of the functions can also be reused across instructions within this file).

Copy link
Contributor Author

@thePunderWoman thePunderWoman Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there was a lot of currying happening here because of needed variable values and wanting to be able to clean up the event listeners. I've switched to listeners that only fire once now, which allowed this to be broken up. So, this should hopefully work well. Otherwise I'll have to figure out a good cleanup solution.

Update: We really only need to clean up the listeners for animate.enter and only two of them. I've found a nice clean way to deal with that. This should be decently organized now.

): LongestAnimation | undefined {
if (!(event.target instanceof Element)) return;
const nativeElement = event.target;
if (typeof nativeElement.getAnimations === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this check in a few places and I was wondering if we are checking for the getAnimations function presence on a given element or it's a test whether it's generally supported in the current environment? If the latter - we can just do it once somewhere and reuse the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is primarily because getAnimations does not exist outside of a browser environment. There's two cases where we check it, one is here and the other is when we set up a cancellation on enter animations. I don't know that it makes sense to use it only once in that case. The cancellation function only is used in animateEnter cases.

@@ -273,6 +281,7 @@ class DefaultDomRenderer2 implements Renderer2 {
private readonly ngZone: NgZone,
private readonly platformIsServer: boolean,
private readonly tracingService: TracingService<TracingSnapshot> | null,
private readonly registry: ElementRegistry | null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely where the ElementRegistry symbol is "leaking" (becomes non-tree-shakable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. The DOM renderer is the key place this registry is needed to delay removal of elements.

@thePunderWoman thePunderWoman force-pushed the animate-instructions branch 2 times, most recently from 277e2bf to e6569a6 Compare July 18, 2025 12:25
@thePunderWoman
Copy link
Contributor Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for animate.enter and animate.leave instructions for animations on nodes. The changes include new public APIs like AnimationCallbackEvent, AnimationFunction, and ANIMATIONS_DISABLED, a new ElementRegistry service to manage animations for leaving elements, and the core implementation in the rendering instructions. The changes are well-supported by a comprehensive suite of acceptance tests. I've identified a couple of potential issues in packages/core/src/animation.ts and packages/core/src/render3/instructions/animation.ts. Please see my detailed comments.

This adds the instructions to support enter and leave animations on nodes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer adev: preview area: core Issues related to the framework runtime detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants