Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

### Bug Fixes

- `Popover`: Keep the `will-change: transform` hint only while the popover is moving, so its content is no longer left on a compositing layer that Chrome can render blurry ([#77359](https://github.com/WordPress/gutenberg/pull/77359)).
- `RadioControl`: Keep radio circles visually 16px at all viewport widths while retaining a 24px pointer target ([#83840](https://github.com/WordPress/gutenberg/pull/83840)).
- `Text`: Make single-line truncation and `isBlock` overrides consistent across the main document and iframes ([#82573](https://github.com/WordPress/gutenberg/pull/82573)).
- `CardDivider`: Apply the Card's width, display, and border color consistently inside iframes ([#82577](https://github.com/WordPress/gutenberg/pull/82577)).
Expand Down
33 changes: 33 additions & 0 deletions packages/components/src/popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ export const SLOT_NAME = 'Popover';
* @type {number}
*/
const OVERFLOW_PADDING = 8;
/**
* How long to wait after the last position update before dropping the
* `will-change` hint.
*
* @type {number}
*/
const WILL_CHANGE_IDLE_TIMEOUT = 200;
// An SVG displaying a triangle facing down, filled with a solid
// color and bordered in such a way to create an arrow-like effect.
// Keeping the SVG's viewbox squared simplify the arrow positioning
Expand Down Expand Up @@ -330,6 +337,8 @@ const UnforwardedPopover = (
y,
// Object with "regular" refs to both "reference" and "floating"
refs,
// The "reference" and "floating" DOM elements
elements,
// Type of CSS position property to use (absolute or fixed)
strategy,
update,
Expand Down Expand Up @@ -392,12 +401,36 @@ const UnforwardedPopover = (
refs,
] );

const popoverElementRef = useRef< HTMLElement | null >( null );

const mergedFloatingRef = useMergeRefs( [
refs.setFloating,
dialogRef,
popoverElementRef,
forwardedRef,
] );

// Keep the `will-change: transform` hint on only while the popover is
// actually moving. Leaving it on for good keeps the popover on its own
// compositing layer, which Chrome can render blurry, while never hinting
// makes the popover repaint on every scroll frame (see #46187). The
// floating element can also be swapped without moving, e.g. when a
// `Popover.Slot` mounts, so a new element restarts the timer too.
useLayoutEffect( () => {
const popoverElement = popoverElementRef.current;
if ( ! popoverElement ) {
return;
}

popoverElement.style.willChange = 'transform';

const timeoutId = setTimeout( () => {
popoverElement.style.willChange = 'auto';
}, WILL_CHANGE_IDLE_TIMEOUT );

return () => clearTimeout( timeoutId );
}, [ x, y, elements.floating ] );

const style = isExpanded
? undefined
: {
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/popover/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ $shadow-popover-border-top-only-alternate: 0 #{-$border-width} 0 $gray-900;
.components-popover {
@include reset;
z-index: z-index(".components-popover");
will-change: transform;

&.is-expanded {
position: fixed;
Expand Down
107 changes: 107 additions & 0 deletions packages/components/src/popover/test/index.browser.test.tsx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: neither new test moves an already-settled popover, so both still pass if the x/y dependencies are removed.

Could we cover movement after idle, including the hint staying on until movement stops?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A new test was added in c57786d.

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
placementToMotionAnimationProps,
} from '../utils';
import Popover from '..';
import '../style.scss';
import { Provider as SlotFillProvider } from '../../slot-fill';
import type { PopoverProps } from '../types';
import { PopoverInsideIframeRenderedInExternalSlot } from './utils/index.js';
Expand Down Expand Up @@ -843,6 +844,112 @@ describe( 'Popover', () => {
} );
} );

it( 'hints at transform only while it is being repositioned', async () => {
await render(
<Popover animate={ false } data-testid="popover-element">
Inside popover
</Popover>
);

const popover = screen.getByTestId( 'popover-element' );

// Positioning has just run, so the compositing hint is on.
expect( getComputedStyle( popover ).willChange ).toBe( 'transform' );

// Once the popover settles, the hint is dropped so the popover stops
// occupying a compositing layer of its own, which is what makes
// Chrome render it blurry.
await waitFor( () =>
expect( getComputedStyle( popover ).willChange ).toBe( 'auto' )
);
} );

it( 'hints at transform again when a settled popover moves, until it stops moving', async () => {
const Test = ( { mainAxis }: { mainAxis: number } ) => (
<Popover
placement="right-start"
offset={ { mainAxis, crossAxis: 0 } }
animate={ false }
flip={ false }
shift={ false }
data-testid="popover-element"
>
Inside popover
</Popover>
);
const { rerender } = await render( <Test mainAxis={ 0 } /> );

const popover = screen.getByTestId( 'popover-element' );
await waitFor( () =>
expect( getComputedStyle( popover ).willChange ).toBe( 'auto' )
);

vi.useFakeTimers( { toFake: [ 'setTimeout', 'clearTimeout' ] } );
try {
const moveTo = async ( mainAxis: number ) => {
const { left } = popover.getBoundingClientRect();
await rerender( <Test mainAxis={ mainAxis } /> );
// `waitFor` stalls on the fake timers and `vi.waitFor` advances
// them, so poll on real timers instead.
await expect
.poll( () => popover.getBoundingClientRect().left )
.toBeCloseTo( left + 10 );
};

// Moving the settled popover brings the hint back.
await moveTo( 10 );
expect( getComputedStyle( popover ).willChange ).toBe(
'transform'
);

// Moving again before the idle timeout keeps the hint on past the
// first timeout.
vi.advanceTimersByTime( 150 );
await moveTo( 20 );
vi.advanceTimersByTime( 150 );
expect( getComputedStyle( popover ).willChange ).toBe(
'transform'
);

// Once the popover stops moving, the hint is dropped.
vi.advanceTimersByTime( 50 );
expect( getComputedStyle( popover ).willChange ).toBe( 'auto' );
} finally {
vi.useRealTimers();
}
} );

it( 'drops the transform hint when the popover moves into a slot without repositioning', async () => {
const Test = ( { hasSlot }: { hasSlot: boolean } ) => (
<SlotFillProvider>
<Popover animate={ false } data-testid="popover-element">
Inside popover
</Popover>
{ hasSlot && <Popover.Slot /> }
</SlotFillProvider>
);
const { rerender } = await render( <Test hasSlot={ false } /> );

const inlinePopover = screen.getByTestId( 'popover-element' );
await waitFor( () =>
expect( getComputedStyle( inlinePopover ).willChange ).toBe(
'auto'
)
);

// Mounting the slot swaps in a new floating element at the same
// coordinates, and that one has to lose the hint too.
await rerender( <Test hasSlot /> );

const slottedPopover = screen.getByTestId( 'popover-element' );
expect( slottedPopover ).not.toBe( inlinePopover );
await waitFor( () =>
expect( getComputedStyle( slottedPopover ).willChange ).toBe(
'auto'
)
);
} );

it( 'should call a consumer-provided onKeyDown alongside close-on-Escape', async () => {
const user = userEvent.setup();
const onClose = vi.fn();
Expand Down
Loading