Avoid paint when hovering / scrolling (Popovers)

[corentin-gautier]

What?

This PR mainly addresses performances issues on popovers, when scrolling & animating.
It replaces the use of top, left properties by translate and scale.

⚠️ It also forces the use of scroll (overflow-y: scroll) on the editor and sidebar, doing so avoid paint to be triggered when an element is removed from the DOM but also when typing

Why?

Using paint triggering properties will cause poor performances, making the UI feel slow and unresponsive

Going forward there should be a particular attention on using performance friendly properties (transform, opacity)

How?

By replacing top, left with x, y and using will-change

Testing Instructions

1. Open the developer console and enable paint flashing
2. trigger the inserter, no paint should happen
3. scroll when a block toolbar is active, no paint should happen

Screenshots or screencast

Paint issues on popovers :
https://user-images.githubusercontent.com/4660731/198627124-e3490bc7-91a0-467f-b6ab-4ba6d7a73f28.mov

Fixes #45382

Related #23568

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @corentin-gautier! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[ciampo]

Hey @corentin-gautier , first of all thank you very much for opening this PR — its description is clear and it denotes a certain level of expertise with CSS and browsers' rendering pipeline!

Unfortunately, I see a few potential issues with this PR:

• the scope is very large — this PR currently affects many areas of the editor across many packages
• this PR changes a few variables (like the sidebar width) which are approved by the project's designers
• switching from top/right/bottom/left to transform could not always be possible

For this reason, I would ask you to split this PR into many smaller ones, each addressing a specific component / part of the project. That's going to make it easier to review and understand the feasibility of the changes that you're proposing.

In the meantime, thank you again for kickstarting this effort!

[corentin-gautier]

Hi @ciampo , thanks for the review !

• the scope is very large — this PR currently affects many areas of the editor across many packages

Indeed :D Splitting it into smaller bits should be feasible, it is very time consuming for me though

• this PR changes a few variables (like the sidebar width) which are approved by the project's designers

Yes, to take into account the width of the scrollbar so that it maintains the same width as before, that being said the width of the scrollbar varies accros browsers and I am not aware of any way to get this information using css (like it is possible for "safe-area-inset-top" and such)

• switching from top/right/bottom/left to transform could not always be possible

I did extensive testing and currently I see no issues with using translate, it works for the block toolbar, the inserter, the color palette popover, the margin/padding popover. I might have missed some use cases though, if you think of any in particular feel free to share it with me !

[aquaspy]

@corentin-gautier I see that you mentioned a issue that I'm always checking (#22822) and I feel like this PR will help a lot!

Does the editor feels smoother after these changes? For me, the major issue with Gutenberg currently is performance. The frontend is nice, but it lags for typing long posts. One easy way to see it is to just access https://wordpress.org/gutenberg and try spam enter + text. You will see that it uses a LOT of CPU and lags to create and put the text on all the paragraph blocks that you are creating.

This doesn't happens on Classic Editor, you can spam enter and text as much as you want and it won't lag, even on really old computers.

[corentin-gautier]

@corentin-gautier I see that you mentioned a issue that I'm always checking (#22822) and I feel like this PR will help a lot!

Does the editor feels smoother after these changes? For me, the major issue with Gutenberg currently is performance. The frontend is nice, but it lags for typing long posts. One easy way to see it is to just access https://wordpress.org/gutenberg and try spam enter + text. You will see that it uses a LOT of CPU and lags to create and put the text on all the paragraph blocks that you are creating.

This doesn't happens on Classic Editor, you can spam enter and text as much as you want and it won't lag, even on really old computers.

Hi ! I think it might help but the paint problem is not the main issue here : typing seem to trigger a lot of javascript and I think that's what causing lag.

Would you be able to test my PR to see if it improves things on your computer ?

[ciampo]

As @corentin-gautier says, this PR will mostly improve rendering interactions on interactions that trigger a layout shift in the interface (e.g. opening a popover, scrolling the sidebar, etc..)

As mentioned above, typing performance is likely related to the execution of the JavaScript code for the block editor and its blocks

[corentin-gautier]

It does improve the performances when typing a bit by limiting the repaint of other blocks (only the current block should be repainted, the current version of Gutenberg triggers repaint of every block when typing) but I don't think it's such a huge improvement of performances.

@ciampo I have made a few changes in order to limit the number of lines to review (reverted the changes due to css linting) and simplified the changes a little bit, do you think this could make it easier to review ? The changes are pretty straightforward I think !

[aquaspy]

@corentin-gautier I would LOVE to test it! How do I build Gutenberg on my pc? I know that I can download the code from your fork, but I'm not sure on how to build it 🤔

[corentin-gautier]

@corentin-gautier I would LOVE to test it! How do I build Gutenberg on my pc? I know that I can download the code from your fork, but I'm not sure on how to build it 🤔

I think you can install the zip created by the pipelines here : https://github.com/WordPress/gutenberg/actions/runs/3496204991

[ciampo]

I'll see if I'll have some time next week to dig deeper and give it a proper look. In the meantime, I've also tagged @youknowriad and @tyxla , who may be interested in reviewing these changes too

[tyxla]

Thanks a lot for your work here, @corentin-gautier!

I've left some questions and feedback - let me know what you think.

Furthermore, I'm on the same boat as @ciampo - this feels like it's touching too many things at the same time, and ideally, we want to ship changes one by one, test them thoroughly and provide opportunities for everyone to give feedback, especially for the bits that introduce visual or functional changes. So, I'd highly recommend splitting the PR into multiple pieces.

[tyxla]

Why do we need to pass x and y to style?

[corentin-gautier]

Because it is translated to translate(x, y) ;)

[ciampo]

The x and y props are framer-motion specific APIs. I was initially thinking of suggesting to use translateX and translateY, as it's more self-explanatory — BUT that would conflict with the Popover's animation logic which would override translateX/translateY values. We should ideally rework that logic so that it doesn't override those values, but for now we can keep using the x and y props and just leave an inline comment.

Also, let's Math.floor() the x and y variables (as explained in https://floating-ui.com/docs/misc#subpixel-and-accelerated-positioning) and let's check for null and NaN.

Overall, applying this feedback could result in something like this:

							// `x` and `y` are framer-motion specific props and are shorthands
							// for `translateX` and `translateY`. Currently it is not possible
							// to use `translateX` and `translateY` because those values would
							// be overridden by the return value of the
							// `placementToMotionAnimationProps` function in `AnimatedWrapper`
							x:
								x === null || Number.isNaN( x )
									? 0
									: Math.floor( x ) ?? undefined,
							y:
								y === null || Number.isNaN( y )
									? 0
									: Math.floor( y ) ?? undefined,

[corentin-gautier]

Hi @ciampo ! thanks for the feedback but I think we could achieve the same result with a simpler : Math.floor(x) || undefined. This would always result in undefined if the value is NaN, 0, "whatever", [], {} etc. which is what we want because then the translate value is not set 👍 (0 will result in translateX(0), undefined will not set anything) and make the code a lot easier to read ! Also wouldn't Math.round be more appropriate ?

[ciampo]

Math.round would work too, but both Math.floor and Math.round technically don't accept null as a value (without that check, you'd get a TypeScript error)

[tyxla]

This is an interesting suggestion! Changing the animation of the insertion point deserves its own PR IMHO. We'll need design feedback on it as well.

[corentin-gautier]

I didn't change the way the animation plays, changing from animating left, right to animating the scale has the same exact effect

[aquaspy]

@corentin-gautier

Sorry for the delay and thanks for the zip. I tested it, and it works really well!

It's not the "perfect" performance, but it's a NICE improvement. I can feel things faster and working better.

I hope they accept this PR

[corentin-gautier]

@ciampo @tyxla I have reverted the changes on the skeleton (Setting the overflow to scroll and changing the sidebar width) so that the PR is be easier to review. It now only focus on the popovers.

Fixing the layout shift when selecting blocks/typing can be addressed in another PR, so I'll keep the issue open in the meantime

(As mentionned here, areas need to have overflow scroll to avoid repaint : #23427 (comment))

[ciampo]

Thank you for the updates, @corentin-gautier !

As requested previously by both @tyxla and myself, I still personally feel like this PR touches too many points. These changes may feel small, but some of these components are ubiquitous in Gutenberg and I'd prefer if we could work in isolation on each component.

My suggestion is that you open a GH issue to track all the parts of the repo that you want to improve, and then open separate PRs for each of those parts:

• the Popover component (i.e. files in components/src/popover + any spanshot updates)
• the CircularOptionPicker component (i.e. files in components/src/circular-option-picker + any snapshot updates)
• the ResizableBox component (ie. files in components/src/resizable-box + any snapshot updates)
• the ListView component (ie files in packages/block-editor/src/components/list-view + any snapshot updates)
• the InsertionPoint component (i.e. insertion point-related files in packages/block-editor/src/components/block-tools + any snapshots)
• changes to the skeleton (I remember some changes to the editor sidebar and its scrolling) and fixing the layout shift when selecting blocks/typing

Each of those PRs should have detailed testing instructions (before/after, which parts of the editor are affected).

This procedure may seem tedious, but it ensures that those changes are well tested and that any developer looking for the history of those changes will have ample context of why they were applied, and how they were tested.

Thank you for your patience and your collaboration — I'm definitely eager to merge those improvements as much as you are!

[ciampo]

The x and y props are framer-motion specific APIs. I was initially thinking of suggesting to use translateX and translateY, as it's more self-explanatory — BUT that would conflict with the Popover's animation logic which would override translateX/translateY values. We should ideally rework that logic so that it doesn't override those values, but for now we can keep using the x and y props and just leave an inline comment.

Also, let's Math.floor() the x and y variables (as explained in https://floating-ui.com/docs/misc#subpixel-and-accelerated-positioning) and let's check for null and NaN.

Overall, applying this feedback could result in something like this:

							// `x` and `y` are framer-motion specific props and are shorthands
							// for `translateX` and `translateY`. Currently it is not possible
							// to use `translateX` and `translateY` because those values would
							// be overridden by the return value of the
							// `placementToMotionAnimationProps` function in `AnimatedWrapper`
							x:
								x === null || Number.isNaN( x )
									? 0
									: Math.floor( x ) ?? undefined,
							y:
								y === null || Number.isNaN( y )
									? 0
									: Math.floor( y ) ?? undefined,

[corentin-gautier]

Hi @ciampo !

As requested I have created an issue here : #46199
This PR can be closed

[skorasaurus]

Thank you!

[ciampo]

Thank you @corentin-gautier, appreciate the effort !