Skip to content
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

feat: CSS animation keyframes registry to attach animations by name #7015

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

MatiPl01
Copy link
Member

Summary

This PR adds CSSAnimationKeyframesRegistry to store animation configuration objects that may be used by multiple views.

In the previous implementation we created separate style interpolators for each CSSAnimation object in CPP, which is created separately for every animation on different views.

The new implementation registers animation keyframes in the native side just once for the animation, so when the user creates an animation with css.keyframes and reuses it on multiple views, the same AnimationStyleInterpolator instance will be reused for each of views where the same animation is used.

Inline animation keyframes still work in the same way as before, so even if the same keyframes are used for multiple views, separate animations will be created for these views.

Test plan

Open the example app and see that everything still works

@MatiPl01 MatiPl01 self-assigned this Feb 11, 2025
@MatiPl01 MatiPl01 requested a review from piaskowyk February 11, 2025 17:26
@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch from acb590e to b53d230 Compare February 13, 2025 16:06
@MatiPl01 MatiPl01 force-pushed the @matipl01/css-keyframes-registry branch from 4758801 to 61992a0 Compare February 13, 2025 16:27
@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch from de7d7eb to 672f435 Compare February 15, 2025 21:43
@MatiPl01 MatiPl01 force-pushed the @matipl01/css-keyframes-registry branch from 61992a0 to 2db4ac2 Compare February 15, 2025 23:05
* side only when used for the first time and unregistered when removed from the
* last view that uses them.
*/
export default class CSSKeyframesRegistry {
Copy link
Member Author

@MatiPl01 MatiPl01 Feb 15, 2025

Choose a reason for hiding this comment

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

I am removing this registry in the #7027 but I decided to keep it in this PR not to mess everything up, so you can skip this file while doing a review

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but as I understand it, we currently need to call the JSI function from JS twice during the mounting render, specifically with registerCSSKeyframes and registerCSSAnimations. I think it might be more efficient to pass the keyframesConfig directly to registerCSSAnimations and have it call the registerCSSKeyframes method internally. This approach could reduce the number of JSI calls, which are relatively expensive. What do you think?

@MatiPl01
Copy link
Member Author

Correct me if I'm wrong, but as I understand it, we currently need to call the JSI function from JS twice during the mounting render, specifically with registerCSSKeyframes and registerCSSAnimations. I think it might be more efficient to pass the keyframesConfig directly to registerCSSAnimations and have it call the registerCSSKeyframes method internally. This approach could reduce the number of JSI calls, which are relatively expensive. What do you think?

I purposely created 2 separate function for that to separate animation keyframes registration from attaching animation to the specific view. This will allow us to use the animation by name (e.g. in the animation shorthand) when we cannot get animation keyframes in the animations manager that calls registerCSSAnimations. In such a case, when the animation shorthand is used, the only thing that this manager knows is the animation name.

On the other hand, we may want to create a keyframes registry on the JS side as well (which I removed as I wanted to rely on the garbage collector) due to the problems with garbage collection that may happen too early causing the animation shorthand string to become invalid. I think we should discuss about that internally and figure out a valid solution.

After this change, we can reduce a number of JSI calls (but it makes sense only for inline animations). When it comes to animations that can be reused multiple times, I think that it is still better to register keyframes just once, and then use only the animation name to attach animation to views via registerCSSAnimations.

@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch from 5a3f873 to c5c593f Compare February 18, 2025 16:51
Base automatically changed from @matipl01/make-css-interpolators-stateless to main February 18, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants