Skip to content

Avoid paint when hovering / scrolling (Popovers) #45545

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

Closed
corentin-gautier wants to merge 1 commit into from
Closed

Avoid paint when hovering / scrolling (Popovers) #45545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
avoid unnecessary paint on animations & block selections
  • Loading branch information
Corentin Gautier committed Nov 28, 2022
commit 3d277ddb2e0f38b6fd1f66cb274f12c42fa8a1f8
50 changes: 3 additions & 47 deletions packages/block-editor/src/components/block-tools/insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ function InbetweenInsertionPointPopover( {
isInserterShown: insertionPoint?.__unstableWithInserter,
};
}, [] );
const isVertical = orientation === 'vertical';

const disableMotion = useReducedMotion();

Expand All @@ -105,65 +104,22 @@ function InbetweenInsertionPointPopover( {
}
}

// Define animation variants for the line element.
const horizontalLine = {
start: {
width: 0,
top: '50%',
bottom: '50%',
x: 0,
},
rest: {
width: 4,
top: 0,
bottom: 0,
x: -2,
},
hover: {
width: 4,
top: 0,
bottom: 0,
x: -2,
},
};
const verticalLine = {
start: {
height: 0,
left: '50%',
right: '50%',
y: 0,
},
rest: {
height: 4,
left: 0,
right: 0,
y: -2,
},
hover: {
height: 4,
left: 0,
right: 0,
y: -2,
},
};
const lineVariants = {
// Initial position starts from the center and invisible.
start: {
...( ! isVertical ? horizontalLine.start : verticalLine.start ),
opacity: 0,
scale: 0,
Copy link
Member

@tyxla tyxla Nov 22, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@corentin-gautier corentin-gautier Nov 22, 2022

Choose a reason for hiding this comment

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

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

},
// The line expands to fill the container. If the inserter is visible it
// is delayed so it appears orchestrated.
rest: {
...( ! isVertical ? horizontalLine.rest : verticalLine.rest ),
opacity: 1,
borderRadius: '2px',
scale: 1,
transition: { delay: isInserterShown ? 0.5 : 0, type: 'tween' },
},
hover: {
...( ! isVertical ? horizontalLine.hover : verticalLine.hover ),
opacity: 1,
borderRadius: '2px',
scale: 1,
transition: { delay: 0.5, type: 'tween' },
},
};
Expand Down
17 changes: 12 additions & 5 deletions packages/block-editor/src/components/block-tools/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,22 @@
.block-editor-block-list__insertion-point-indicator {
position: absolute;
background: var(--wp-admin-theme-color);
border-radius: 2px;
transform-origin: center;
opacity: 0;
will-change: transform, opacity;

.block-editor-block-list__insertion-point.is-vertical > & {
top: 50%;
height: $border-width;
top: calc(50% - 2px);
height: 4px;
width: 100%;
}

.block-editor-block-list__insertion-point.is-horizontal > & {
top: 0;
right: 0;
left: 50%;
width: $border-width;
bottom: 0;
left: calc(50% - 2px);
width: 4px;
}
}

Expand All @@ -32,6 +37,8 @@
// Don't show on mobile.
display: none;
position: absolute;
will-change: transform;

@include break-mobile() {
display: flex;
}
Expand Down
4 changes: 1 addition & 3 deletions packages/block-editor/src/components/list-view/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,15 +197,13 @@
@include reduce-motion("transition");

> * {
will-change: opacity;
opacity: 0;
}

// Show on hover, visible, and show above to keep the hit area size.
&:hover,
&.is-visible {
position: relative;
z-index: 1;

> * {
opacity: 1;
@include edit-post__fade-in-animation;
Expand Down
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"
style="position: absolute; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0;"
style="position: absolute; top: 0px; left: 0px; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -52,7 +52,7 @@ exports[`URLPopover matches the snapshot when the settings are toggled open 1`]
<span>
<div
class="components-popover block-editor-url-popover"
style="position: absolute; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0; left: 0px; top: 0px;"
style="position: absolute; top: 0px; left: 0px; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0;"
tabindex="-1"
>
<div
Expand Down Expand Up @@ -107,7 +107,7 @@ exports[`URLPopover matches the snapshot when there are no settings 1`] = `
<span>
<div
class="components-popover block-editor-url-popover"
style="position: absolute; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0;"
style="position: absolute; top: 0px; left: 0px; opacity: 0; transform: translateY(-2em) scale(0) translateZ(0); transform-origin: 50% 0% 0;"
tabindex="-1"
>
<div
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/circular-option-picker/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ $color-palette-circle-spacing: 12px;
vertical-align: top;
transform: scale(1);
transition: 100ms transform ease;
will-change: transform;
@include reduce-motion("transition");

&:hover {
Expand Down
6 changes: 4 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 @@ -488,8 +488,10 @@ const UnforwardedPopover = (
? undefined
: {
position: strategy,
left: Number.isNaN( x ) ? 0 : x ?? undefined,
top: Number.isNaN( y ) ? 0 : y ?? undefined,
top: 0,
left: 0,
x: x || undefined,
Copy link
Member

@tyxla tyxla Nov 22, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@corentin-gautier corentin-gautier Nov 22, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ciampo ciampo Nov 29, 2022

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

@corentin-gautier corentin-gautier Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@ciampo ciampo Nov 30, 2022

Choose a reason for hiding this comment

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

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)

y: y || 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 @@ -2,6 +2,7 @@ $arrow-triangle-base-size: 14px;

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

&.is-expanded {
position: fixed;
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/resizable-box/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ $resize-handler-container-size: $resize-handler-size + ($grid-unit-05 * 2); // M
right: calc(50% - 1px);
transition: transform 0.1s ease-in;
@include reduce-motion("transition");
will-change: transform;
opacity: 0;
}

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"
role="dialog"
style="position: absolute; opacity: 0; transform: translateX(-2em) scale(0) translateZ(0); transform-origin: 0% 50% 0;"
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