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

Navigator: use CSS animations instead of framer-motion #56909

Merged
merged 10 commits into from
Dec 13, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Dec 8, 2023

What?

Related to #55892

Use vanilla CSS animations for the Navigator components instead of framer-motion

Why?

This PR doesn't fix the real cause for #55892, but aims at improving a visual side-effect caused by it

framer-motion animations updated the style attribute of the animating DOM element, setting a new value for every frame. In other words, framer-motion performed some calculations in every animation frame to interpolate the exact values of the animation (ie. translation). These calculations happen on the main thread, and therefore the animation can get quite janky if the main thread is busy with other tasks (see #55892).

Using vanilla CSS allows the animation to run directly on the GPU, running smoothly regardless of how busy the CPU is.

Furthermore, the animations required for this component are simple enough to be written with vanilla CSS, and don't necessarily require the sophisticated orchestration features that framer-motion offers.

How?

  • Moved all styles to separate styles.ts file
  • Replaced framer-motion with CSS animations
  • added contain: strict to the provider screen wrapper to further potentially enhance rendering performance

Note: with this PR, navigator screens don't have an exit animation anymore, as it would require a larger refactor (that's because the previous NavigatorScreen's wrapper element gets removed from the DOM as soon the selected screen id changes).

Testing Instructions

  1. Interact with the Navigator component in Storybook:
    • Make sure that the first screen doesn't have an enter animation
    • Make sure that all subsequent animations (both backward and forward) animate as expected
    • Make sure that the animations are inverted in contexts with RTL direction
    • Make sure that the animations are disabled when the prefers-reduced-motion option is enabled
  2. Open the side editor, and interact with the sidebar:
  3. Open the global styles sidebar in the site editor:
    • Make sure that the sidebar works and looks as on trunk, especially when navigating across the submenus

Screenshots or screencast

Before (trunk)

Notice the jankiness (even while testing on a high-end machine), especially while navigating to/from the "Patterns" navigator screen

navigator-css-before.mp4

After (this PR)

No jankiness

navigator-css-after.mp4

@ciampo ciampo self-assigned this Dec 8, 2023
@ciampo
Copy link
Contributor Author

ciampo commented Dec 8, 2023

@tyxla feel free to test this version of the component and let me know if it's worth continuing the work on this PR.

Copy link

github-actions bot commented Dec 8, 2023

Flaky tests detected in 6599008.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7192313789
📝 Reported issues:

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thank you for trying it out, @ciampo 🙌

This looks great in my testing! Here's a demo:

Screen.Recording.2023-12-11.at.15.40.00.mov

While this doesn't resolve all the performance issues on the site editor, it definitely adds a visual improvement since there's no more clunky resizing of the sidebar screen content. cc @draganescu

We'll definitely have to do some further work to improve site editor performance, but in the meantime, I think we should move forward with this approach.

I've left a few questions and some feedback too, let me know what you think!

packages/components/src/navigator/styles.ts Outdated Show resolved Hide resolved
packages/components/src/navigator/styles.ts Outdated Show resolved Hide resolved
function UnconnectedNavigatorScreen(
props: Props,
props: WordPressComponentProps< NavigatorScreenProps, 'div', false >,
Copy link
Member

Choose a reason for hiding this comment

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

Nice simplification of the types 👍

Any backward compatibility concerns? For example, we won't be able to use any of the framer motion animation events like onAnimationStart / onAnimationComplete.

Copy link
Contributor Author

@ciampo ciampo Dec 13, 2023

Choose a reason for hiding this comment

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

If I recall correctly (and interpret the previous TypeScript code correctly), that shouldn't be a problem because Navigator was previously not documenting props specific to framer-motion that would overlap with standard HTML properties (like onAnimationStart).

So, with the changes from this PR, the component should now accept the default HTML onAnimationStart etc

packages/components/src/navigator/styles.ts Outdated Show resolved Hide resolved
@ciampo ciampo force-pushed the test/navigator-css-only-animations branch from 25875f8 to f84568c Compare December 13, 2023 06:51
@ciampo ciampo requested a review from a team December 13, 2023 07:44
@ciampo ciampo added the [Package] Components /packages/components label Dec 13, 2023
@ciampo ciampo requested review from draganescu and flootr December 13, 2023 07:44
@ciampo ciampo added the [Type] Performance Related to performance efforts label Dec 13, 2023
@ciampo ciampo marked this pull request as ready for review December 13, 2023 07:47
@ciampo ciampo requested a review from ajitbohra as a code owner December 13, 2023 07:47
@ciampo ciampo changed the title Navigator: test using CSS animations instead of framer-motion Navigator: use CSS animations instead of framer-motion Dec 13, 2023
@ciampo ciampo requested review from jasmussen and tyxla December 13, 2023 07:48
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Nothing further from me. Great work 🚀

Performance-wise, the site editor continues to be heavy, so we'll have to continue improving it, but this does fix the visual glitch while navigating between screens that was originally reported in #55892. We'll keep #55892 open anyway because there's a ton we can still improve in that area.

:shipit:

@ciampo ciampo merged commit dc4f3e6 into trunk Dec 13, 2023
52 checks passed
@ciampo ciampo deleted the test/navigator-css-only-animations branch December 13, 2023 11:56
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 13, 2023
@draganescu
Copy link
Contributor

For me this PR in Firefox works a bit weird:

navigator-css-animation.mp4
  • Navigating is sometimes animated, sometimes doesn't look animated
  • Navigating back to templates from its children shows a blank sidebar for a bit
  • Navigating through navigations still stutters

@ciampo
Copy link
Contributor Author

ciampo commented Dec 13, 2023

Hey @draganescu , I just tested Navigator in isolation in Storybook on Firefox and the animations seems to work as expected.

My suspicion is that the site editor is slower in Firefox compared to Chrome, which causes the animations to stutter to the point that it looks like they don't happen at all and/or are glitchy. This is especially true when animating to/from the patterns screen.

As explained above, the changes in this PR can only partially improve the perception of smoothness on the Navigator animation, but really it is just trying to improve a symptom and not the actual issue. The real issue is IMO caused by the site editor canvas performing some CPU-heavy operations.

artemiomorales pushed a commit that referenced this pull request Jan 4, 2024
* Move navigator provider styles to separate file

* Move navigator screen styles to separate file, use CSS animations instead of framer motion

* Remove unused import

* Spacing

* Use standard ease-in-out easing function

* Remove stale comments

* Remove animation-specific tests (as they can't be tested in jsdom)

* CHANGELOG

* Add comment

* Avoid running the `css` function when unnecessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Performance Related to performance efforts
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

4 participants