Popover: Avoid paint on popovers when scrolling #46187

corentin-gautier · 2022-11-30T10:33:23Z

What?

This PR changes the way popovers are positioned from top/left to translate(x, y) to avoid repaint on scroll

Why?

Changing the top/left properties triggers browser repaint which will result in poor performances

Testing Instructions

Open the dev console and enable the paint flashing (under rendering)
Select a block
Scroll so that the block toolbar needs to be re-positioned
There should not be paint on the toolbar

Fixes #45382

Screenshot

Before

Screen.Recording.2022-10-28.at.15.47.36.mov

After

Screen.Recording.2022-11-30.at.14.12.43.mov

github-actions · 2022-11-30T10:33:45Z

👋 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.

tyxla · 2022-12-01T09:00:26Z

packages/components/src/popover/style.scss

@@ -2,6 +2,7 @@ $arrow-triangle-base-size: 14px;

 .components-popover {
 	z-index: z-index(".components-popover");
+	will-change: transform;


See #46197 (comment) for a suggestion regarding how we add / remove this CSS property.

@tyxla since the number of popover is limited I don't think this is a big issue, BUT it might actually not be needed since popovers seem to get a translateZ for free 😄

@tyxla I have reverted the change since the translateZ does the job, however it seems to trigger repaint of some of the elements inside the block toolbar which is ... weird 😅

As @corentin-gautier mentioned, technically we shouldn't need it because of the translateZ() value that framer motion already adds — in that sense, I didn't expect the browser to trigger repaints as reported in the previous message 🤷 although browser heuristics about when to create a new composite layer can change and evolve in time, so we're never really guaranteed.

Regarding the when we should add/remove this prop, it's worth mentioning that the Popover component should be rendered only when the popover is actually visible — therefore, it seems OK to me that we keep that CSS rule as always ON: it's only rendered when the popover is open.

@ciampo do you recommend i add it back then ?

It would be good if you could investigate a bit more into why removing will-change: transform causes paint flashes in the toolbar, and see if we can achieve the desired result in any other way.

But if after the investigation the only guaranteed way to avoid paint flashes if by having will-change: transform always applied, then yes — I would recommend you add it back at that point

@ciampo I looked into it yesterday but couldn't find the cause, it seems some of the svgs inside the toolbar trigger repaint when the toolbar position is updated ...

There are other tricks to avoid paint like using backface-visibility: hidden; so that's an option too :)

Have we tried setting contain: paint on the toolbar buttons, out of curiosity?

There are other tricks to avoid paint like using backface-visibility: hidden; so that's an option too :)

Yup, although that doesn't really change the fact that we'd be "forcing" the popover to be on its own composite layer — so will-change: transform seems like a better (more semantic) choice

@ciampo using contain: paint; is .... worse 😂 It triggers paint of all the buttons (I know it's very weird ahah)

Screen.Recording.2022-12-02.at.11.40.01.mov

🤷 Let's stick to wil-change: transform then! Thank you for the investigation :)

If you ever feel like digging more into this and come up with more insights, I'm always eager to see how we can improve this component further!

tyxla · 2022-12-01T09:09:28Z

packages/components/src/popover/index.tsx

+							// to use `translateX` and `translateY` because those values would
+							// be overridden by the return value of the
+							// `placementToMotionAnimationProps` function in `AnimatedWrapper`
+							x: Math.round( x ?? 0 ) || undefined,


Previously, while animating we were accepting decimal values for x and y, and now we're rounding them. This is likely the reason for a bunch of e2e tests failing.

@tyxla Yes it was suggested by @ciampo here : #45545 (comment)

@tyxla The tests indeed fail because of this but I have no idea how to change the expected values

npm run test:unit:update should update the snapshots — you should then commit those changes

@ciampo I ran it but there nothing to commit 🤔

Do you mind rebasing and fixing conflicts? I'm currently unable to see that tests are failing, which makes it harder to make an informed suggestion 😅

@tyxla I can see unit tests erroring because of the act warning — do you mind helping out here? (I'm going to be AFK in the following weeks)

Sure thing! Happy to aid with that next week.

It looks like the change here results in 0 values not being applied to update popover positions for popovers that are intended to be flush with the edge of the screen. I've opened up a potential fix over in #51320.

ellatrix · 2022-12-01T22:17:10Z

Looks like this is making the toolbar a lot shakier when scrolling?

corentin-gautier · 2022-12-01T22:52:33Z

Looks like this is making the toolbar a lot shakier when scrolling?

Having paint flashing enabled + the element selected in the dev tools makes it shaky, Without this it behaves normally 🙂

jasmussen · 2022-12-02T08:44:35Z

Feels pretty solid to me:

Screeny.Video.2.Dec.2022.at.09.43.26.mov

Thank you for all these PRs! 👏

tyxla

This looks and works well for me, thanks for the great work @corentin-gautier! 🚀

ciampo · 2022-12-02T17:29:54Z

Before merging, do you mind adding a CHANGELOG entry for the changes in the @wordpress/components package, as suggested by Aaron in this other PR?

We could actually add an entry also for #46197 which was merged without one

tyxla · 2022-12-05T10:17:20Z

@corentin-gautier tests appear to be failing for a few reasons here:

E2e tests - failing because the e2e tests expect x and y values to not be rounded, and we're now rounding them. It should be fine to manually adjust them to the new rounded values.
Unit tests - failing because of the act warnings, seems to be because there's a promise running after the popovers are fully positioned, which makes the tests fragile. A straightforward way to fix them is to unmount() at the end of the failing tests, in order to cancel any pending effects that would cause act() warnings.

Let me know if that helps!

corentin-gautier · 2022-12-05T14:05:29Z

@tyxla Hum, yes there's a chance 😅 I now have the same result but not much more info ...

corentin-gautier · 2022-12-05T15:11:29Z

@tyxla I can fix the test by using a rounding function on firstBlockBox

Something like this :

const roundBoundingBox = (obj) => {
	Object.keys(obj).forEach(k => {
		obj[k] = Math.round(obj[k]);
	});
	return obj;
}

But I don't know if this should be the way we fix this

test/e2e/specs/editor/blocks/paragraph.spec.js

corentin-gautier · 2022-12-07T16:19:50Z

@tyxla thing is : if we round the position of the popover, then it will never be strictly equal to the position of the block (which is calculated with a getBoundingClientRect and will return sub-pixel values, https://playwright.dev/docs/api/class-locator#locator-bounding-box)

From what I understand of the tests, their are testing that the dropzone (a BlockPopover) position is equal to the block (a paragraph) position.

tyxla · 2022-12-08T10:25:53Z

@tyxla thing is : if we round the position of the popover, then it will never be strictly equal to the position of the block (which is calculated with a getBoundingClientRect and will return sub-pixel values, https://playwright.dev/docs/api/class-locator#locator-bounding-box)

From what I understand of the tests, their are testing that the dropzone (a BlockPopover) position is equal to the block (a paragraph) position.

Indeed, but then that points to a different problem - we either should change our expectations that the two positions will be equal (meaning, reconsidering the test completely instead of just rounding one of the values), or we should use the same heuristics - either both rounding or both non-rounding. Since rounding both is a no-go, should we reconsider the rounding for the popover position?

corentin-gautier · 2022-12-08T10:39:05Z

Indeed, but then that points to a different problem - we either should change our expectations that the two positions will be equal (meaning, reconsidering the test completely instead of just rounding one of the values), or we should use the same heuristics - either both rounding or both non-rounding. Since rounding both is a no-go, should we reconsider the rounding for the popover position?

As I mentioned before it was suggested by @ciampo here in reference to this

x and y can contain decimals, so unless the transform translation is placed evenly on the device's subpixel grid, then there will be blurring

I think it's reasonable to assume that the values won't be exactly the same (and by that I mean rounded, so not far from the same) because as you said we can't round the block's position but rounding the popover's position seems to be required in order to get a sharp UI

That being said it wasn't rounded before so maybe this could be the subject of another PR ?

tyxla · 2022-12-08T13:10:24Z

That being said it wasn't rounded before so maybe this could be the subject of another PR ?

I'm inclined to agree with that. I'd love to get a second opinion from @mirka if possible.

mirka · 2022-12-09T00:16:35Z

That being said it wasn't rounded before so maybe this could be the subject of another PR ?

I'm inclined to agree with that. I'd love to get a second opinion from @mirka if possible.

You're asking whether we should leave the rounding part out of this PR? I wouldn't be opposed to that, but also it looks easy enough to alter the test assertions so it has tolerance for subpixel differences.

By the way, I think we could encapsulate the intent better if we explicitly tested for the tolerance, rather than apply rounding to the block values. So expect(Math.abs(ax-bx)).toBeLessThan(1) or something like that. Just a thought.

tyxla · 2022-12-09T09:20:23Z

By the way, I think we could encapsulate the intent better if we explicitly tested for the tolerance, rather than apply rounding to the block values. So expect(Math.abs(ax-bx)).toBeLessThan(1) or something like that. Just a thought.

Loving that suggestion FWIW 👍 I'm happy to go with that - WDYT @corentin-gautier?

With regards to the failing unit tests - @jsnajdr mentioned he has a branch that could fix them - would you like to try cherry-picking them to your branch and seeing if it fixes the tests?

jsnajdr · 2022-12-09T13:29:05Z

After #46429 is merged, you can rebase this PR on top of it and it should solve the failing unit tests (that test popover positioning).

corentin-gautier · 2022-12-12T10:52:45Z

@tyxla @mirka what do you think of this : b50ca9f ?

I've tried keeping the tests to be one liners instead of duplicating for x and x

tyxla

This looks great! 💯 Thanks @corentin-gautier 🙌

A potential follow-up could be creating a custom inline matcher to make the assertion more eloquent, but that's not a big deal IMHO, since your comment is useful in making the intent clear.

I'll ship this for you 🚀

Thanks again and keep up the great work 🥇

corentin-gautier requested review from ajitbohra and ellatrix as code owners November 30, 2022 10:33

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 30, 2022

corentin-gautier mentioned this pull request Nov 30, 2022

Avoid browser paint in the editor #46199

Closed

corentin-gautier changed the title ~~Avoid paint on popovers~~ Avoid paint on popovers when scrolling Nov 30, 2022

corentin-gautier force-pushed the fix/popover-paint branch from cd5046e to 45ce6a9 Compare November 30, 2022 13:20

skorasaurus added the [Type] Performance Related to performance efforts label Nov 30, 2022

ciampo requested review from talldan, tyxla, mirka, ciampo and youknowriad November 30, 2022 15:30

ciampo added the [Package] Components /packages/components label Nov 30, 2022

ciampo requested review from brookewp and chad1008 November 30, 2022 15:33

tyxla reviewed Dec 1, 2022

View reviewed changes

corentin-gautier force-pushed the fix/popover-paint branch from 45ce6a9 to 6f05f45 Compare December 1, 2022 10:07

ellatrix requested a review from jasmussen December 1, 2022 22:18

corentin-gautier force-pushed the fix/popover-paint branch from 6f05f45 to e4c3a32 Compare December 2, 2022 10:46

tyxla approved these changes Dec 2, 2022

View reviewed changes

corentin-gautier force-pushed the fix/popover-paint branch 2 times, most recently from 43c04a7 to c253f6e Compare December 2, 2022 13:16

corentin-gautier force-pushed the fix/popover-paint branch from 7b7c6cb to 3ca9e06 Compare December 5, 2022 10:26

tyxla requested changes Dec 6, 2022

View reviewed changes

test/e2e/specs/editor/blocks/paragraph.spec.js Outdated Show resolved Hide resolved

corentin-gautier force-pushed the fix/popover-paint branch 2 times, most recently from 04697d0 to 4a57091 Compare December 7, 2022 16:01

corentin-gautier force-pushed the fix/popover-paint branch from 4a57091 to 02bbe89 Compare December 8, 2022 09:30

jsnajdr mentioned this pull request Dec 9, 2022

Popover: check positioning by adding and testing is-positioned class #46429

Merged

corentin-gautier force-pushed the fix/popover-paint branch from 02bbe89 to a198dfe Compare December 12, 2022 09:07

Avoid paint on popovers

cfd40ae

corentin-gautier force-pushed the fix/popover-paint branch from ac84df6 to cfd40ae Compare December 12, 2022 09:33

fix e2e test

b50ca9f

tyxla approved these changes Dec 12, 2022

View reviewed changes

tyxla merged commit b120213 into WordPress:trunk Dec 12, 2022

github-actions bot added this to the Gutenberg 14.8 milestone Dec 12, 2022

corentin-gautier deleted the fix/popover-paint branch December 12, 2022 12:34

ryanwelcher changed the title ~~Avoid paint on popovers when scrolling~~ Popover: Avoid paint on popovers when scrolling Dec 14, 2022

Mamaduka mentioned this pull request Dec 20, 2022

[Flaky Test] Both blocks are empty paragraph blocks #46463

Closed

tyxla mentioned this pull request Dec 20, 2022

E2E: Fix confirmValidDropZonePosition() when dropzone not ready #46677

Merged

This was referenced Jan 4, 2023

Block Selection Toolbar: Support fixed and sticky blocks by flipping when there is not enough space above the block #46565

Closed

Block Selection Toolbar: Support fixed blocks by flipping toolbar when block goes out of view #46085

Closed

andrewserong mentioned this pull request Jun 8, 2023

Popover: Allow legitimate 0 positions to update popover position #51320

Merged

Popover: Avoid paint on popovers when scrolling #46187

Popover: Avoid paint on popovers when scrolling #46187

Conversation

corentin-gautier commented Nov 30, 2022 • edited Loading

What?

Why?

Testing Instructions

Screenshot

Before

After

github-actions bot commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corentin-gautier Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Dec 1, 2022

corentin-gautier commented Dec 1, 2022 • edited Loading

jasmussen commented Dec 2, 2022

tyxla left a comment

Choose a reason for hiding this comment

ciampo commented Dec 2, 2022 • edited Loading

tyxla commented Dec 5, 2022

corentin-gautier commented Dec 5, 2022

corentin-gautier commented Dec 5, 2022

corentin-gautier commented Dec 7, 2022 • edited Loading

tyxla commented Dec 8, 2022

corentin-gautier commented Dec 8, 2022 • edited Loading

tyxla commented Dec 8, 2022

mirka commented Dec 9, 2022

tyxla commented Dec 9, 2022

jsnajdr commented Dec 9, 2022

corentin-gautier commented Dec 12, 2022

tyxla left a comment

Choose a reason for hiding this comment

corentin-gautier commented Nov 30, 2022 •

edited

Loading

corentin-gautier Dec 2, 2022 •

edited

Loading

corentin-gautier commented Dec 1, 2022 •

edited

Loading

ciampo commented Dec 2, 2022 •

edited

Loading

corentin-gautier commented Dec 7, 2022 •

edited

Loading

corentin-gautier commented Dec 8, 2022 •

edited

Loading