fix(DST-1754): keep popovers inside the body's clip box at the window edge - #5776
Open
OsamaAbdellateef wants to merge 6 commits into
Open
fix(DST-1754): keep popovers inside the body's clip box at the window edge#5776OsamaAbdellateef wants to merge 6 commits into
OsamaAbdellateef wants to merge 6 commits into
Conversation
… edge
react-aria positions an overlay against the visual viewport, which still
counts the gutter reserved by `html { scrollbar-gutter: stable }`. The body
is that much narrower and clips at its own box (`body { overflow-x: clip }`),
so an overlay pinned to the right edge landed inside the gutter and was
sliced — 3.42px on a 1280px viewport, taking the right border and the
rounded corners with it.
Fold the reserved gutter into the boundary padding so the overlay stops at
the body's clip box instead of the viewport's. Pages without a reserved
gutter measure 0 and keep the current 12px, so nothing changes there.
Reported from a product in AVW-6159, reproducible in our own docs at
/examples/general.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 63ea2b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Accessibility tests executed. Download the report here. |
Contributor
Coverage Report for Marigold Code Coverage
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
OsamaAbdellateef
marked this pull request as ready for review
September 1, 2026 09:33
sarahgm
requested changes
Sep 1, 2026
|
|
||
| const DEFAULT_CONTAINER_PADDING = 12; | ||
|
|
||
| const getContainerPadding = () => { |
Member
There was a problem hiding this comment.
Tooltip.tsx renders react-aria-components/Tooltip directly with the default containerPadding: 12, and Table/TableEditableCell.tsx imports RAC's Popover directly. Both take the same getContainerDimensions path, so both lose their edge in the same situation. The ticket excludes <Tray> and <Modal> correctly, since neither is trigger-positioned, but it never mentions Tooltip. Either export this helper and use it there too, or record why Tooltip is out of scope.
Worth filing upstream as well (getContainerDimensions should clamp visualViewport.width to documentElement.clientWidth) and linking the issue here, so the workaround can be deleted when it lands.
Contributor
Author
There was a problem hiding this comment.
Done — the helper moved to packages/components/src/Overlay/containerPadding.ts and both call sites use it:
Tooltip.tsxtakes it as acontainerPaddingdefault, same shape as<Popover>.Table/TableEditableCell.tsxpassesgetContainerPadding()to its RAC<Popover>.
So Tooltip was never out of scope; it was just missed. Thanks for catching it.
Upstream is not filed yet. Two reasons worth stating rather than quietly skipping:
- It publishes under the repo owner's name on a repo we don't own, so it needs an explicit go-ahead.
- The framing in your comment — "
getContainerDimensionsshould clampvisualViewport.widthtodocumentElement.clientWidth" — doesn't hold. In the failing case both read 1280 (see my reply on the measurement above), so that clamp is a no-op. The accurate report is narrower:getContainerDimensionstakesvisualViewport.widthas the boundary, but an overlay that locks scrolling removes the scrollbar while a reservedscrollbar-gutterkeeps the layout narrower, so the boundary ends up a gutter wider than the box that clips.
I have the repro and the numbers for that version. Happy to file it and link it here on your say-so.
One caveat for <Tooltip>: it doesn't lock scrolling, so on a page already showing its scrollbar it now gets a correction it didn't need and stops a gutter's width earlier than required. Bounded by the scrollbar width, 0 without a gutter, and written down in the changeset.
| // Stands in for a reserved scrollbar gutter; see the decorator below. | ||
| const SIMULATED_GUTTER = 15; | ||
|
|
||
| export const AtViewportEdge = meta.story({ |
Member
There was a problem hiding this comment.
🔴 Visual Regression Tests — not run
This PR changes UI-affecting files but the Visual-Regression-Tests workflow has not run for this branch, and the change moves overlay geometry by design.
Please trigger it by commenting /run-chromatic on this PR, or by manually dispatching the workflow.
Contributor
Author
There was a problem hiding this comment.
Still not run — flagging rather than quietly leaving it. Dispatching consumes Chromatic quota, so it's the repo owner's call.
Two things that make it more worthwhile than when you wrote this:
- The
AtViewportEdgestory now opts back into Storybook (dev) and into Chromatic (chromatic: { disableSnapshot: false }), so there's an actual snapshot of the geometry this PR changes. As you noted, the meta's!devmeant nothing here was ever visible or snapshotted. - Overlay geometry only moves on pages that reserve a scrollbar gutter, so a Chromatic run is unlikely to show a diff by itself — worth setting expectations before reading the result.
Review follow-up on #5776. `body { overflow-x: clip }` with a visible `html` propagates the clip to the viewport, so the line that clips is `documentElement.clientWidth`, not the body's margin box. The two coincide in our docs because the body has no margin; they stop coinciding for any consumer whose body carries a margin or a centred `max-width`, and the old measurement folded that whole distance into `containerPadding` permanently. The correction is now react-aria's own error — visual viewport width minus the clip box — which is 0 whenever react-aria is already right and immune to body margins. It moves to `Overlay/containerPadding.ts` so `<Tooltip>` and the `<Table>` editable-cell overlay, which take the same `getContainerDimensions` path, get the same fix. The measurement is cached against `window.innerWidth` so a forced layout no longer runs on every render of every menu and select. Tests split along what can be tested honestly: a headless browser reserves no gutter, so the size of the correction is unit-tested against stubbed viewport measurements, and the story test now proves only what a browser can prove — that `containerPadding` moves the overlay and that `<Popover>` forwards it. The old story simulated a body margin, which clips nothing, so it passed for the wrong reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous measurement, `documentElement.clientWidth`, is 0 in exactly the case this fix is about, so the menu was still clipped. Measured in a headed Chrome at 1280px wide with a 15px gutter, against where an absolutely positioned child actually stops being painted: config | true clip | body.right | clientWidth | fixed ------------------------------|-----------|------------|-------------|------ gutter, no scrollbar rendered | 1265 | 1265 | *1280* | 1265 gutter, scrollbar rendered | 1265 | 1265 | 1265 | 1265 no gutter, body margin 100 | 1280 | *1180* | 1280 | 1280 gutter + body margin 100 | 1265 | *1165* | *1280* | 1265 gutter + html margin 50 | 1265 | *1215* | *1280* | 1265 Chrome only subtracts a scrollbar it actually renders, so a reserved but unused gutter narrows the layout without narrowing `clientWidth` — the first row, which is the reported case. The review was right that `body.right` is wrong wherever the page carries a margin (rows 3-5): the clip propagates from the body to the viewport, so the body's own box does not clip. What holds in every case is the fixed-positioning containing block, which is the viewport's scrollport. It costs a throwaway element to measure, which the existing cache already covers. End-to-end on a real `<Menu>` in headed Chrome with the gutter reserved: before 1268.14 against a clip line of 1265 (3.14px sliced, matching the report); after 1253.14. The unit tests now stub only react-aria's input and measure the real clip line, and cover the margin cases the review raised — a headless browser reserves no gutter, so the gutter case stays a manual check. Also opts `AtViewportEdge` back into Storybook (`dev`) and into Chromatic: the meta hides this file because the other two stories are bare fixtures, but this one shows the geometry the fix is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Opting it back into Storybook exposed that it was only ever built to be asserted on, not looked at. The decorator rendered a second element with `id="storybook-root"`. That was invisible while the file was hidden; in the real preview it collides with Storybook's own root. The story now owns a container with its own id and points `OverlayContainerProvider` at it, which works in the preview and in the test runner both. Note that `OverlayContainerProvider` with no `container` is not a fallback to the body, despite what its doc comment says: `getContainer` returns null and react-aria's `Overlay` bails out, rendering nothing. The rest is presentation, since the story is now browsable and snapshotted: a real `<Button>` as the trigger rather than a bare text node, and content in an `<Inset>` so it is not pressed against the popover's rim. The assertion now measures against react-aria's own boundary (`visualViewport.width`) rather than `documentElement.clientWidth`. The two disagree exactly when a gutter is reserved, which is the subject of this fix, so the boundary is the honest reference for what `containerPadding` promises. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both costs of the correction now read together: `containerPadding` is symmetric, and an overlay that does not lock scrolling gets a correction it did not need. Also corrects the mechanism, which is the overlay's own scroll lock rather than a page that happens not to scroll. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The story hard-coded `containerPadding={100}`, so what Storybook rendered was
a popover shoved 100px off its trigger and stretched to fill the gap — a
number picked to make an assertion sharp, on display as if it were the
component's behaviour.
The padding now comes from args, so the story shows the geometry the fix
actually produces and only the test overrides it. The trigger also sits just
off the edge rather than flush against it, which read as clipped — the same
thing the fix is about — and the copy is short enough that the popover is not
a slab.
Contributor
|
Accessibility tests executed. Download the report here. |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A
<Menu>/<ActionMenu>opened near the right edge of the window lost its right border and rounded corners. Reported from a product in AVW-6159 and reproducible in our own docs at/examples/general(the user menu in the top bar).The cause is ours, in
themes/theme-rui/src/preflight.css:html { scrollbar-gutter: stable }reserves a gutter (15px on the measured setup).body { overflow-x: clip }makes the body's own box the line where anything is cut.react-aria keeps an overlay inside its boundary by measuring the visual viewport, which still counts that reserved gutter — so it positions against a box 15px wider than the one that actually clips. Both rules are deliberate (no reflow when the scrollbar appears; a single scroll axis, with
clipchosen overhiddensoposition: stickykeeps working), so this PR leaves them alone and fixes the positioning side instead: the reserved gutter is folded intocontainerPadding, so the overlay stops at the body's clip box.Pages without a reserved gutter measure 0 and keep the current 12px, so nothing changes there. This covers every overlay that goes through our
<Popover>: Menu, ActionMenu, Select, ComboBox, ContextualHelp.Closes DST-1754
Screenshots / Preview
No image capture — the browser pane in my environment would not composite frames, so the page could not be screenshotted. Measured instead on
/examples/generalat a 1280px viewport:window.innerWidthbodyright edge — the real clip lineTest Instructions
pnpm test:sb packages/components/src/Overlay/Popover.stories.tsx— the newAtViewportEdgestory test opens an overlay flush against the right edge and asserts it stays behind the body's clip line.containerPadding = getContainerPadding()toDEFAULT_CONTAINER_PADDINGinPopover.tsxand rerun — it fails withexpected 1187.6 to be less than or equal to 1185, the same ~3px slice.pnpm start, open/examples/general, click the user menu in the top bar. Its right border and rounded corner should be intact.Note on the test: headless browsers use overlay scrollbars, so
scrollbar-gutter: stablereserves nothing and the case cannot reproduce on its own. The story's decorator stands in for the gutter with a body narrower than the visual viewport. Without that the test passes against the broken code — it was vacuous before the decorator was added.Breaking Changes
No. Overlay geometry is unchanged wherever no gutter is reserved. Where one is, overlays near a viewport edge move inward by the gutter width (15px on a typical desktop) — that is the fix.
Checklist
component-testtag where applicable)pnpm changeset)