Skip to content

fix(overlays): cap visualViewport width at clientWidth to handle scrollbar gutter #9275

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

Open
hasegawa-101 wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix(overlays): cap visualViewport width at clientWidth to handle scrollbar gutter #9275

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1965848
fix(overlays): cap visualViewport width at clientWidth to handle scro…
hasegawa-101 Dec 3, 2025
9f3ca00
Merge branch 'main' into fix/popover-position-scrollbar-gutter
hasegawa-101 Dec 3, 2025
0bbdf4a
fix(overlays): delete space
hasegawa-101 Dec 3, 2025
b4ca1ca
Merge remote-tracking branch 'origin/fix/popover-position-scrollbar-g…
hasegawa-101 Dec 3, 2025
233013e
test(overlays): add visualViewport test case for scrollbar gutter han…
hasegawa-101 Dec 3, 2025
619b900
chore(overlays): remove outdated scrollbar gutter handling logic test
hasegawa-101 Dec 3, 2025
efc27d7
chore(overlays): comment out outdated visualViewport scrollbar gutter…
hasegawa-101 Dec 3, 2025
c47c4f5
Revert "chore(overlays): comment out outdated visualViewport scrollba…
hasegawa-101 Dec 3, 2025
535794e
Revert "chore(overlays): remove outdated scrollbar gutter handling lo…
hasegawa-101 Dec 3, 2025
f9d5612
fix(overlays): round visualViewport width before comparing to clientW…
hasegawa-101 Dec 3, 2025
ffb11b6
Merge branch 'main' into fix/popover-position-scrollbar-gutter
hasegawa-101 Dec 4, 2025
f6e4372
fix(overlays): use totalWidth to cap visualViewport width
hasegawa-101 Dec 5, 2025
1424e7f
Merge remote-tracking branch 'origin/fix/popover-position-scrollbar-g…
hasegawa-101 Dec 5, 2025
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: 6 additions & 0 deletions packages/@react-aria/overlays/src/calculatePosition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@ function getContainerDimensions(containerNode: Element, visualViewport: VisualVi
totalHeight = documentElement.clientHeight;
width = visualViewport?.width ?? totalWidth;
height = visualViewport?.height ?? totalHeight;

// If the visual viewport is larger than the client width, it means that the scrollbar gutter is taking up space
// that the visual viewport is not accounting for. In this case, we should cap the width at the client width.
// if (width > documentElement.clientWidth) {
// width = documentElement.clientWidth;
// }
Copy link
Contributor

@nwidynski nwidynski Dec 3, 2025

Choose a reason for hiding this comment

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

This should probably be rounded to account for sub-pixel differences when zooming.

Copy link
Member

@snowystinger snowystinger Dec 4, 2025

Choose a reason for hiding this comment

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

shouldn't it be ceil? or floor? I don't quite follow what the concern is here though

also, this doesn't need to be an if, can be

width = Math.min(Math.round(width), documentElement.clientWidth)

Copy link
Contributor

@nwidynski nwidynski Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Concern were false-positives in general. I did not take a deeper look at the underlying issue before, but in this case false-positives could result in a tiny gap between the overlay and scrollbar, right? Maybe that's acceptable though - certainly better than cutting content off 🤷

On Firefox & Chrome the clientWidth appears to correspond to Math.round, but Safari apparently uses some other heuristic, neither floor nor ceil.

Copy link
Member

@snowystinger snowystinger Dec 5, 2025

Choose a reason for hiding this comment

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

This is sounding very similar to some previous iterations of work here https://github.com/adobe/react-spectrum/pull/8958/files#diff-78d190ce0d1388eb141efadb6cae01cfc47f354f8f3bc3a700405d0f74e606b6R24

it's now the weekend for me, I'll try to look again soon.

scroll.top = documentElement.scrollTop || containerNode.scrollTop;
scroll.left = documentElement.scrollLeft || containerNode.scrollLeft;

Expand Down
72 changes: 72 additions & 0 deletions packages/@react-aria/overlays/test/calculatePosition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,4 +446,76 @@ describe('calculatePosition', function () {
document.body.removeChild(target);
});
});

// describe('visualViewport larger than clientWidth (scrollbar gutter issue)', () => {
// let clientWidthSpy;
//
// afterEach(() => {
// if (clientWidthSpy) {
// clientWidthSpy.mockRestore();
// }
// });
//
// it('caps width at clientWidth', () => {
// // Mock clientWidth to be smaller than visualViewport
// clientWidthSpy = jest.spyOn(document.documentElement, 'clientWidth', 'get').mockImplementation(() => 985);
//
// // Mock visualViewport
// window.visualViewport = {
// width: 1000,
// height: 600,
// offsetLeft: 0,
// offsetTop: 0,
// pageLeft: 0,
// pageTop: 0,
// scale: 1,
// addEventListener: jest.fn(),
// removeEventListener: jest.fn(),
// dispatchEvent: jest.fn(),
// onresize: null,
// onscroll: null
// } as VisualViewport;
//
// const target = document.createElement('div');
// const overlayNode = document.createElement('div');
// // Use body as boundary to trigger the specific code path
// const container = document.body;
//
// // Setup target position near the right edge
// // Target at left=900, width=50. Center is 925.
// jest.spyOn(target, 'getBoundingClientRect').mockImplementation(() => ({
// top: 0, left: 900, width: 50, height: 50, right: 950, bottom: 50, x: 900, y: 0, toJSON: () => { }
// }));
//
// // Setup overlay size
// // Width=200.
// jest.spyOn(overlayNode, 'getBoundingClientRect').mockImplementation(() => ({
// top: 0, left: 0, width: 200, height: 200, right: 200, bottom: 200, x: 0, y: 0, toJSON: () => { }
// }));
// jest.spyOn(overlayNode, 'offsetWidth', 'get').mockImplementation(() => 200);
// jest.spyOn(overlayNode, 'offsetHeight', 'get').mockImplementation(() => 200);
//
// let result = calculatePosition({
// placement: 'bottom',
// overlayNode,
// targetNode: target,
// scrollNode: overlayNode,
// padding: 0,
// shouldFlip: false,
// boundaryElement: container,
// offset: 0,
// crossOffset: 0,
// arrowSize: 0
// });
//
// // Expected calculation:
// // Boundary width should be capped at 985 (clientWidth) instead of 1000 (visualViewport).
// // Overlay width is 200.
// // Max allowed left position = 985 - 200 = 785.
// // Target center is 925. Centered overlay would be 925 - 100 = 825.
// // 825 > 785, so it should be clamped to 785.
//
// expect(result.position.left).toBe(785);
// });
// });
});