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.

Sign up for GitHub

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

Popover: Avoid paint on popovers when scrolling #46187

Merged
merged 2 commits into from
Dec 12, 2022
Merged

Popover: Avoid paint on popovers when scrolling #46187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfd40ae
Avoid paint on popovers
Dec 12, 2022
b50ca9f
fix e2e test
Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`URLPopover matches the snapshot in its default state 1`] = `
<span>
<div
class="components-popover block-editor-url-popover is-positioned"
style="position: absolute; left: 0px; top: 0px;"
style="position: absolute; top: 0px; left: 0px; transform: none;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -53,7 +53,7 @@ exports[`URLPopover matches the snapshot when the settings are toggled open 1`]
<span>
<div
class="components-popover block-editor-url-popover is-positioned"
style="position: absolute; left: 0px; top: 0px;"
style="position: absolute; top: 0px; left: 0px; transform: none;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -108,7 +108,7 @@ exports[`URLPopover matches the snapshot when there are no settings 1`] = `
<span>
<div
class="components-popover block-editor-url-popover is-positioned"
style="position: absolute; left: 0px; top: 0px;"
style="position: absolute; top: 0px; left: 0px; transform: none;"
tabindex="-1"
>
<div
Expand Down
2 changes: 2 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
- `Popover`: Prevent unnecessary paint caused by using outline ([#46201](https://github.com/WordPress/gutenberg/pull/46201)).
- `PaletteEdit`: Global styles: add onChange actions to color palette items [#45681](https://github.com/WordPress/gutenberg/pull/45681).
- Lighten the border color on control components ([#46252](https://github.com/WordPress/gutenberg/pull/46252)).
- `Popover`: Prevent unnecessary paint when scrolling by using transform instead of top/left positionning ([#46187](https://github.com/WordPress/gutenberg/pull/46187)).
- `CircularOptionPicker`: Prevent unecessary paint on hover ([#46197](https://github.com/WordPress/gutenberg/pull/46197)).

### Experimental

Expand Down
11 changes: 9 additions & 2 deletions packages/components/src/popover/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,15 @@ const UnforwardedPopover = (
? undefined
: {
position: strategy,
left: Number.isNaN( x ) ? 0 : x ?? undefined,
top: Number.isNaN( y ) ? 0 : y ?? undefined,
top: 0,
left: 0,
// `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: Math.round( x ?? 0 ) || undefined,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciampo I ran it but there nothing to commit 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@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)

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing! Happy to aid with that next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

y: Math.round( y ?? 0 ) || undefined,
}
}
>
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ $shadow-popover-border-top-only-alternate: 0 #{-$border-width} 0 $gray-900;

.components-popover {
z-index: z-index(".components-popover");
will-change: transform;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@corentin-gautier corentin-gautier Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ciampo do you recommend i add it back then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷 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!


&.is-expanded {
position: fixed;
Expand Down
2 changes: 1 addition & 1 deletion packages/nux/src/components/dot-tip/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`DotTip should render correctly 1`] = `
aria-label="Editor tips"
class="components-popover nux-dot-tip is-positioned"
role="dialog"
style="position: absolute; opacity: 0; transform: translateX(-2em) scale(0) translateZ(0); transform-origin: 0% 50% 0; left: 0px; top: 0px;"
style="position: absolute; top: 0px; left: 0px; opacity: 0; transform: translateX(-2em) scale(0) translateZ(0); transform-origin: 0% 50% 0;"
tabindex="-1"
>
<div
Expand Down
122 changes: 94 additions & 28 deletions test/e2e/specs/editor/blocks/paragraph.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -207,8 +211,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -219,8 +227,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -231,8 +243,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand Down Expand Up @@ -309,8 +325,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -321,8 +341,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -333,8 +357,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -345,8 +373,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}
} );

Expand Down Expand Up @@ -387,8 +419,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -399,8 +435,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -411,8 +451,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( firstBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
firstBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -423,8 +467,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -435,8 +483,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}

{
Expand All @@ -447,8 +499,12 @@ test.describe( 'Paragraph', () => {
);
await expect( draggingUtils.dropZone ).toBeVisible();
await expect
.poll( () => draggingUtils.dropZone.boundingBox() )
.toEqual( secondBlockBox );
.poll( () =>
draggingUtils.confirmValidDropZonePosition(
secondBlockBox
)
)
.toBeTruthy();
}
} );
} );
Expand Down Expand Up @@ -508,4 +564,14 @@ class DraggingUtils {
await this.page.mouse.move( 0, 0 );
await this.page.mouse.down();
}

async confirmValidDropZonePosition( element ) {
// Check that both x and y axis of the dropzone
// have a less than 1 difference with a given target element
const box = await this.dropZone.boundingBox();
return (
Math.abs( element.x - box.x ) < 1 &&
Math.abs( element.y - box.y ) < 1
);
}
}