From 52e38b49a41c4af716d33f343f63ce74d7743bd8 Mon Sep 17 00:00:00 2001 From: Tim Gesemann Date: Thu, 13 Aug 2026 22:12:48 +0200 Subject: [PATCH 1/4] fix: Menu popover doesn't flip when a user-set maxHeight can't fit below the trigger (#10176) Folds the user-provided maxHeight into the height-axis flip-trigger comparison in calculatePositionInternal, so the decision to flip is based on the overlay's true requested size instead of a stale clamped height left over from a previous positioning pass. --- .../src/overlays/calculatePosition.ts | 8 +- .../test/overlays/useOverlayPosition.test.tsx | 81 +++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/packages/react-aria/src/overlays/calculatePosition.ts b/packages/react-aria/src/overlays/calculatePosition.ts index 5c82a9f0f52..04bb0a86ad7 100644 --- a/packages/react-aria/src/overlays/calculatePosition.ts +++ b/packages/react-aria/src/overlays/calculatePosition.ts @@ -461,8 +461,12 @@ export function calculatePositionInternal( isContainerDescendentOfBoundary ); - // Check if the scroll size of the overlay is greater than the available space to determine if we need to flip - if (flip && overlaySize[size] > space) { + // Check if the overlay's size is greater than the available space to determine if we need to flip. + // On the height axis, fold in the user-set maxHeight so a stale clamped measurement from a previous + // positioning pass doesn't mask the overlay's true (unclamped) content size. + let sizeForFlipCheck = + size === 'height' ? (userSetMaxHeight ?? overlaySize[size]) : overlaySize[size]; + if (flip && sizeForFlipCheck > space) { let flippedPlacementInfo = parsePlacement( `${FLIPPED_DIRECTION[placement]} ${crossPlacement}` as Placement ); diff --git a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx index cb517994598..a840754e14b 100644 --- a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx +++ b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx @@ -11,6 +11,7 @@ */ import {fireEvent, render} from '@react-spectrum/test-utils-internal'; +import {I18nProvider} from '../../src/i18n/I18nProvider'; import React, {useRef} from 'react'; import {useOverlayPosition} from '../../src/overlays/useOverlayPosition'; @@ -260,6 +261,86 @@ describe('useOverlayPosition', function () { expect(arrow).toHaveAttribute('aria-hidden', 'true'); expect(arrow).toHaveAttribute('role', 'presentation'); }); + + it('should not flip in RTL when a user maxHeight fits within the available space (regression guard for the #10176 fix)', function () { + // At triggerTop=250, ~406px of room is available below the trigger (see the + // unmodified default-placement test above). A maxHeight just under that should + // still fit below and never trigger a flip. Uses a "start" cross-placement so + // translateRTL exercises the RTL codepath. + let res = render( + + + + ); + let overlay = res.getByTestId('overlay'); + + expect(overlay).toHaveTextContent('placement: bottom'); + expect(overlay).toHaveStyle('max-height: 400px;'); + }); + + describe('menu popover flip regression (#10176)', function () { + // The overlay's natural (unclamped) content height. Mutated by tests to simulate + // content that populates after the initial positioning pass (e.g. RAC collection + // items landing on a second render). + let overlayNaturalHeight = 40; + + beforeEach(() => { + overlayNaturalHeight = 40; + + // Simulate the browser's rendering of a scrollable (overflow: auto) overlay: + // its offsetHeight is clamped to whatever CSS max-height is currently applied, + // even if that's stale from a previous positioning pass. + jest + .spyOn(HTMLElement.prototype, 'offsetHeight', 'get') + .mockImplementation(function (this: HTMLElement) { + if (this.getAttribute?.('data-testid') === 'overlay') { + let styleMaxHeight = + this.style.maxHeight !== '' ? parseInt(this.style.maxHeight, 10) : Infinity; + return Math.min(overlayNaturalHeight, styleMaxHeight); + } + return parseInt(this.style.height, 10) || 0; + }); + }); + + it('flips to the top once content grows, when no maxHeight is set (self-heals)', function () { + // Trigger near the bottom edge: ~50px of room below, ~594px above. + let res = render(); + let overlay = res.getByTestId('overlay'); + + expect(overlay).toHaveTextContent('placement: bottom'); + + // Content populates on a second render (e.g. RAC collection items landing). + // Same trigger position and viewport; only the content height changes. + overlayNaturalHeight = 400; + fireEvent(window, new Event('resize')); + + expect(overlay).toHaveTextContent('placement: top'); + expect(parseInt(overlay.style.maxHeight, 10)).toBeGreaterThan(300); + }); + + it('fixes #10176: flips to the top and grows toward the requested maxHeight, once a user maxHeight is set', function () { + // Same geometry as the self-healing case above, but with a user-provided + // maxHeight (the reporter found maxHeight={350} made the bug reproduce + // reliably instead of intermittently). + let res = render(); + let overlay = res.getByTestId('overlay'); + + // The flip-trigger comparison folds the user-set maxHeight into the overlay's + // rendered size on the height axis, so even on the very first pass it flips to + // the top: the full requested maxHeight (300) needs more room than the ~50px + // available below, and the ~594px available above can satisfy it. + expect(overlay).toHaveTextContent('placement: top'); + expect(parseInt(overlay.style.maxHeight, 10)).toBe(300); + + // Content populates on a second render, identically to the self-healing case. + overlayNaturalHeight = 400; + fireEvent(window, new Event('resize')); + + // Remains flipped to the top, still grown to the full requested maxHeight. + expect(overlay).toHaveTextContent('placement: top'); + expect(parseInt(overlay.style.maxHeight, 10)).toBe(300); + }); + }); }); describe('useOverlayPosition with positioned container', () => { From 76dd4f7e293629575957c597e7e4fcd184f46145 Mon Sep 17 00:00:00 2001 From: Tim Gesemann Date: Sun, 16 Aug 2026 13:29:40 +0200 Subject: [PATCH 2/4] chore: clean up menu popover flip regression tests Ground the new #10176 flip-regression test titles in behavior instead of the issue number, and move them next to the existing sibling maxHeight test instead of after an unrelated arrow test. --- .../test/overlays/useOverlayPosition.test.tsx | 124 +++++++++--------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx index a840754e14b..a296ec0629d 100644 --- a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx +++ b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx @@ -203,66 +203,7 @@ describe('useOverlayPosition', function () { `); }); - it('should close the overlay when the trigger scrolls', function () { - let onClose = jest.fn(); - let res = render( -
- -
- ); - - let scrollable = res.getByTestId('scrollable'); - fireEvent.scroll(scrollable); - expect(onClose).toHaveBeenCalledTimes(1); - }); - - it('should not close the overlay when an adjacent scrollable region scrolls', function () { - let onClose = jest.fn(); - let res = render( -
- -
test
-
- ); - - let scrollable = res.getByTestId('scrollable'); - fireEvent.scroll(scrollable); - expect(onClose).not.toHaveBeenCalled(); - }); - - it('should close the overlay when the body scrolls', function () { - let onClose = jest.fn(); - render(); - - fireEvent.scroll(document.body); - expect(onClose).toHaveBeenCalledTimes(1); - }); - - it('should close the overlay when the document scrolls', function () { - let onClose = jest.fn(); - render(); - - fireEvent.scroll(document); - expect(onClose).toHaveBeenCalledTimes(1); - }); - - it('should close the overlay when target is window in a scroll event', function () { - let onClose = jest.fn(); - render(); - - fireEvent.scroll(window); - expect(onClose).toHaveBeenCalledTimes(1); - }); - - it('arrow should be hidden when using assistive technologies', function () { - let res = render(); - let arrow = res.getByTestId('arrow'); - - expect(arrow).toHaveAttribute('aria-hidden', 'true'); - expect(arrow).toHaveAttribute('role', 'presentation'); - }); - - it('should not flip in RTL when a user maxHeight fits within the available space (regression guard for the #10176 fix)', function () { + it('should not flip in RTL when a user maxHeight fits within the available space', function () { // At triggerTop=250, ~406px of room is available below the trigger (see the // unmodified default-placement test above). A maxHeight just under that should // still fit below and never trigger a flip. Uses a "start" cross-placement so @@ -278,7 +219,7 @@ describe('useOverlayPosition', function () { expect(overlay).toHaveStyle('max-height: 400px;'); }); - describe('menu popover flip regression (#10176)', function () { + describe('flipping when the overlay outgrows its current placement', function () { // The overlay's natural (unclamped) content height. Mutated by tests to simulate // content that populates after the initial positioning pass (e.g. RAC collection // items landing on a second render). @@ -318,7 +259,7 @@ describe('useOverlayPosition', function () { expect(parseInt(overlay.style.maxHeight, 10)).toBeGreaterThan(300); }); - it('fixes #10176: flips to the top and grows toward the requested maxHeight, once a user maxHeight is set', function () { + it('flips to the top and grows toward the requested maxHeight, once a user maxHeight is set', function () { // Same geometry as the self-healing case above, but with a user-provided // maxHeight (the reporter found maxHeight={350} made the bug reproduce // reliably instead of intermittently). @@ -341,6 +282,65 @@ describe('useOverlayPosition', function () { expect(parseInt(overlay.style.maxHeight, 10)).toBe(300); }); }); + + it('should close the overlay when the trigger scrolls', function () { + let onClose = jest.fn(); + let res = render( +
+ +
+ ); + + let scrollable = res.getByTestId('scrollable'); + fireEvent.scroll(scrollable); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('should not close the overlay when an adjacent scrollable region scrolls', function () { + let onClose = jest.fn(); + let res = render( +
+ +
test
+
+ ); + + let scrollable = res.getByTestId('scrollable'); + fireEvent.scroll(scrollable); + expect(onClose).not.toHaveBeenCalled(); + }); + + it('should close the overlay when the body scrolls', function () { + let onClose = jest.fn(); + render(); + + fireEvent.scroll(document.body); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('should close the overlay when the document scrolls', function () { + let onClose = jest.fn(); + render(); + + fireEvent.scroll(document); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('should close the overlay when target is window in a scroll event', function () { + let onClose = jest.fn(); + render(); + + fireEvent.scroll(window); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('arrow should be hidden when using assistive technologies', function () { + let res = render(); + let arrow = res.getByTestId('arrow'); + + expect(arrow).toHaveAttribute('aria-hidden', 'true'); + expect(arrow).toHaveAttribute('role', 'presentation'); + }); }); describe('useOverlayPosition with positioned container', () => { From c5b5090d61a32d0b39f13c731e62d7c0e8aac7fa Mon Sep 17 00:00:00 2001 From: Tim Gesemann Date: Mon, 17 Aug 2026 12:41:42 +0200 Subject: [PATCH 3/4] chore: trim inline comments in menu popover flip fix Pare comments down to the non-obvious why (the offsetHeight mock and the maxHeight fold-in rationale); drop ones that just restated code or assertions already visible nearby. --- .../src/overlays/calculatePosition.ts | 4 +-- .../test/overlays/useOverlayPosition.test.tsx | 26 +++---------------- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/packages/react-aria/src/overlays/calculatePosition.ts b/packages/react-aria/src/overlays/calculatePosition.ts index 04bb0a86ad7..35b7fc7a329 100644 --- a/packages/react-aria/src/overlays/calculatePosition.ts +++ b/packages/react-aria/src/overlays/calculatePosition.ts @@ -461,9 +461,7 @@ export function calculatePositionInternal( isContainerDescendentOfBoundary ); - // Check if the overlay's size is greater than the available space to determine if we need to flip. - // On the height axis, fold in the user-set maxHeight so a stale clamped measurement from a previous - // positioning pass doesn't mask the overlay's true (unclamped) content size. + // On the height axis, use the user-set maxHeight instead of a possibly stale clamped measurement. let sizeForFlipCheck = size === 'height' ? (userSetMaxHeight ?? overlaySize[size]) : overlaySize[size]; if (flip && sizeForFlipCheck > space) { diff --git a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx index a296ec0629d..ccd56485af2 100644 --- a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx +++ b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx @@ -204,10 +204,7 @@ describe('useOverlayPosition', function () { }); it('should not flip in RTL when a user maxHeight fits within the available space', function () { - // At triggerTop=250, ~406px of room is available below the trigger (see the - // unmodified default-placement test above). A maxHeight just under that should - // still fit below and never trigger a flip. Uses a "start" cross-placement so - // translateRTL exercises the RTL codepath. + // 'start' cross-placement exercises the RTL codepath. let res = render( @@ -220,17 +217,14 @@ describe('useOverlayPosition', function () { }); describe('flipping when the overlay outgrows its current placement', function () { - // The overlay's natural (unclamped) content height. Mutated by tests to simulate - // content that populates after the initial positioning pass (e.g. RAC collection - // items landing on a second render). + // Natural (unclamped) content height; tests mutate this to simulate content + // populating after the initial positioning pass. let overlayNaturalHeight = 40; beforeEach(() => { overlayNaturalHeight = 40; - // Simulate the browser's rendering of a scrollable (overflow: auto) overlay: - // its offsetHeight is clamped to whatever CSS max-height is currently applied, - // even if that's stale from a previous positioning pass. + // Simulates a scrollable overlay whose offsetHeight is clamped to its CSS max-height. jest .spyOn(HTMLElement.prototype, 'offsetHeight', 'get') .mockImplementation(function (this: HTMLElement) { @@ -244,14 +238,11 @@ describe('useOverlayPosition', function () { }); it('flips to the top once content grows, when no maxHeight is set (self-heals)', function () { - // Trigger near the bottom edge: ~50px of room below, ~594px above. let res = render(); let overlay = res.getByTestId('overlay'); expect(overlay).toHaveTextContent('placement: bottom'); - // Content populates on a second render (e.g. RAC collection items landing). - // Same trigger position and viewport; only the content height changes. overlayNaturalHeight = 400; fireEvent(window, new Event('resize')); @@ -260,24 +251,15 @@ describe('useOverlayPosition', function () { }); it('flips to the top and grows toward the requested maxHeight, once a user maxHeight is set', function () { - // Same geometry as the self-healing case above, but with a user-provided - // maxHeight (the reporter found maxHeight={350} made the bug reproduce - // reliably instead of intermittently). let res = render(); let overlay = res.getByTestId('overlay'); - // The flip-trigger comparison folds the user-set maxHeight into the overlay's - // rendered size on the height axis, so even on the very first pass it flips to - // the top: the full requested maxHeight (300) needs more room than the ~50px - // available below, and the ~594px available above can satisfy it. expect(overlay).toHaveTextContent('placement: top'); expect(parseInt(overlay.style.maxHeight, 10)).toBe(300); - // Content populates on a second render, identically to the self-healing case. overlayNaturalHeight = 400; fireEvent(window, new Event('resize')); - // Remains flipped to the top, still grown to the full requested maxHeight. expect(overlay).toHaveTextContent('placement: top'); expect(parseInt(overlay.style.maxHeight, 10)).toBe(300); }); From 114218e22879ddcf99c8eb85070465ab4f1dc2b2 Mon Sep 17 00:00:00 2001 From: Tim Gesemann Date: Mon, 17 Aug 2026 13:02:03 +0200 Subject: [PATCH 4/4] chore: drop redundant initializer on overlayNaturalHeight beforeEach already resets it before every test, so the declaration-time value was never actually observed. --- packages/react-aria/test/overlays/useOverlayPosition.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx index ccd56485af2..c28dfe54de5 100644 --- a/packages/react-aria/test/overlays/useOverlayPosition.test.tsx +++ b/packages/react-aria/test/overlays/useOverlayPosition.test.tsx @@ -219,7 +219,7 @@ describe('useOverlayPosition', function () { describe('flipping when the overlay outgrows its current placement', function () { // Natural (unclamped) content height; tests mutate this to simulate content // populating after the initial positioning pass. - let overlayNaturalHeight = 40; + let overlayNaturalHeight: number; beforeEach(() => { overlayNaturalHeight = 40;