fix: Popover positioning and various other bugs #8816

snowystinger · 2025-09-04T04:52:01Z

Fixes popover positioning, see first commit's new story to see bad behaviour by scrolling trigger partially out of view horizontally to start. You'll see the popover rendered way out of place. After the second commit, it has been fixed and the popover will be bound by the scrolling element.

Fixes pending state for buttons so they aren't clickable through context. Also adds the pending state to ActionButtons.

Adds the default label which we support in other components to Picker's items.

Moves S2 Table cell focus rings so they render over the cell content.

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

…through context

rspbot · 2025-09-04T05:30:12Z

Build successful! 🎉

…o don't

rspbot · 2025-09-04T06:59:02Z

Build successful! 🎉

LFDanLu

Behavior looks good to me, will need some more time to double check the calculatePosition change (wish it wasn't so brittle)

packages/@react-aria/overlays/src/calculatePosition.ts

LFDanLu · 2025-09-04T23:31:05Z

packages/react-aria-components/src/Button.tsx

+      if (key.startsWith('on')) {
+        props[key] = undefined;
+      }


what about onBlur/onFocus? Should those be removed?

o doh, i saw "focusProps" and assumed that's where that was being handled, you are correct

one question i did have here, is should i move this logic into useButton, it might be easier, but currently useButton doesn't know about pending since there are so many style dependent and dom structure requirements, so it only exists in higher levels

hm, is this mainly a concern about how the spinner may be rendered relative to the button and/or if there is a spinner at all? Not quite sure I fully understand the style/dom structure dependencies here

essentially there are different ways you can set things up
https://react-spectrum.adobe.com/react-aria/Button.html#accessibility

but the event behaviour should probably be the same, it'd just be weird to put that into the hooks i guess since it doesn't include a lot of the other choices

gotcha I think for now we punt on moving the logic into the hooks then

packages/@react-spectrum/s2/src/TableView.tsx

rspbot · 2025-09-04T23:53:19Z

Build successful! 🎉

rspbot · 2025-09-05T00:10:14Z

Build successful! 🎉

rspbot · 2025-09-05T00:17:18Z

Build successful! 🎉

rspbot · 2025-09-05T00:27:11Z

Build successful! 🎉

rspbot · 2025-09-05T00:30:46Z

Build successful! 🎉

LFDanLu · 2025-09-05T21:22:27Z

packages/@react-aria/overlays/src/calculatePosition.ts

@@ -181,7 +181,7 @@ function getDelta(
  // Note that these values are with respect to the visual viewport (aka 0,0 is the top left of the viewport)
  let boundaryStartEdge = boundaryDimensions.scroll[AXIS[axis]] + padding;
  let boundaryEndEdge = boundarySize + boundaryDimensions.scroll[AXIS[axis]] - padding;
-  let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]];
+  let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis];


I double checked this and found that it was tied to pinch zoom positioning: 3722d30. If you comment out what the startEdge/endEdge values here are, try opening the RAC Popover story outside the iframe, and open the popover after pinch zooming on Safari in such a way that the trigger is close to the edge of the left of the window you'll see that the startEdgeOffset calculation is incorrect (the boundary will return 12, but the start edge will return a value that is much larger). Each of the values here should return coordinate values transformed in such a way that a zoomed in window should have its origin at the top left.

As a result, opening a dropdown when the trigger is partially off screen won't do the following (aka preserve the visibility of the dropdown by keeping it in the bounds of the screen)

instead this happens, where the dropdown falls outside of its defined boundary

I see the original issue you alluded to in your description, but I think the calculated boundary edges are also incorrect in that story (they currently return 0 and 300 respectively but should be the left and right edge of the box with respect to the top left corner of the story).

In addition, the startEdgeOffset should just be equal to the offset in that case since that value is already in the proper coordinate system, but at the moment it is receiving a containerOffsetWithBoundary that has negative values that should instead be offsetting the value of boundaryDimensions (aka containerOffsetWithBoundary is representing how far offset the boundary element is with respect to the containing element aka the page). I think we'll need to

fix the boundaryStartEdge and boundaryEndEdge calcs here

Change it to be - containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]]; or update how containerOffsetWithBoundary is being calculated to return positive values (this one is probably more correct if we consider the top left corner to be the 0,0 origin and there for the offset should be positive instead of negative if the container is centered in the page)

Ahhh, thank you, I'll have a look on Monday

no worries, happy to sit with you and debug if you'd like as well, this stuff is tricky :/

rspbot · 2025-09-08T05:59:40Z

Build successful! 🎉

…ontainerOffsetWithBoundary

rspbot · 2025-09-09T23:44:08Z

Build successful! 🎉

snowystinger · 2025-09-09T23:40:33Z

packages/@react-aria/overlays/src/calculatePosition.ts

@@ -507,7 +529,7 @@ export function calculatePosition(opts: PositionOpts): PositionResult {
  // If the container is the HTML element wrapping the body element, the retrieved scrollTop/scrollLeft will be equal to the
  // body element's scroll. Set the container's scroll values to 0 since the overlay's edge position value in getDelta don't then need to be further offset
  // by the container scroll since they are essentially the same containing element and thus in the same coordinate system
-  let containerOffsetWithBoundary: Offset = boundaryElement.tagName === 'BODY' ? getOffset(container, false) : getPosition(container, boundaryElement, false);
+  let containerOffsetWithBoundary: Offset = boundaryElement.tagName === 'BODY' ? getOffset(container, false) : getPosition(boundaryElement, container,  false);


flipped so that getPosition works in the same direction as getOffset

snowystinger · 2025-09-09T23:42:18Z

packages/@react-aria/overlays/test/calculatePosition.test.ts

@@ -138,13 +138,16 @@ describe('calculatePosition', function () {
    };

    const container = createElementWithDimensions('div', containerDimensions);
+    Object.assign(container.style, {


calculate position will look for the containing block, which should be container and not document.body as it is in the default case. The default case should mostly be covered by chromatic. These tests cover our other main case where the overlay is inside the container that calculate position will use.

snowystinger · 2025-09-09T23:42:27Z

packages/@react-aria/overlays/test/useOverlayPosition.test.tsx

@@ -89,7 +89,7 @@ describe('useOverlayPosition', function () {
      position: absolute;
      z-index: 100000;
      left: 12px;
-      bottom: 518px;
+      bottom: 350px;


these were wrong

snowystinger · 2025-09-09T23:42:52Z

packages/@react-spectrum/menu/chromatic/Submenu.stories.tsx

@@ -143,6 +143,9 @@ Default.play = async ({canvasElement}) => {
  let body = canvasElement.ownerDocument.body;
  let menu = await within(body).getByRole('menu');
  let menuItems = within(menu).getAllByRole('menuitem');
+
+  // clean up any previous click state


this is the fix for chromatic's stories that threw errors

snowystinger · 2025-09-09T23:47:00Z

packages/@react-spectrum/s2/src/ActionButton.tsx

+                  order: 0
+                })
+              }],
+              [ImageContext, {


should we just add opacity to be overridable to every component? visibility?

snowystinger · 2025-09-09T23:47:38Z

packages/@react-spectrum/s2/src/ActionButton.tsx

+                staticColor: staticColor,
+                size: props.size === 'XS' ? undefined : props.size,
+                isDisabled: isDisabled,
+                styles: style({position: 'absolute', top: '--badgeTop', insetStart: '--badgePosition', marginTop: 'calc((self(height) * -1)/2)', marginStart: 'calc((self(height) * -1)/2)'})


probably shouldn't use notifications and pending together? very much an edge case

i assume it would disappear or something but i doubt design as thought of this case

yeah, i'm leaving this alone for now

snowystinger · 2025-09-09T23:49:01Z

packages/@react-spectrum/s2/src/Picker.tsx

@@ -526,6 +527,11 @@ const PickerButton = createHideableComponent(function PickerButton<T extends obj
                      [TextContext, {
                        slots: {
                          description: {},
+                          [DEFAULT_SLOT]: {styles: style({


this is already how we do it in Menu and ComboBox

snowystinger · 2025-09-09T23:50:55Z

packages/@react-spectrum/s2/src/SelectBoxGroup.tsx


  return (
    <ListBoxItem
      isDisabled={isDisabled}
      ref={ref}
      className={renderProps => (UNSAFE_className || '') + selectBoxStyles({
        ...renderProps,
+        isFocusVisible: isFocusVisible && renderProps.isFocused,


This is because Menu/Picker would remove the focus ring if the mouse moves over it, but a SelectBox should behave like a Radio or Checkbox, and should not have this behaviour

snowystinger · 2025-09-09T23:53:17Z

packages/react-aria-components/src/Button.tsx

@@ -85,10 +85,10 @@ export const ButtonContext = createContext<ContextValue<ButtonContextValue, HTML
 */
 export const Button = /*#__PURE__*/ createHideableComponent(function Button(props: ButtonProps, ref: ForwardedRef<HTMLButtonElement>) {
  [props, ref] = useContextProps(props, ref, ButtonContext);
-  props = disablePendingProps(props);
  let ctx = props as ButtonContextValue;
  let {isPending} = ctx;
  let {buttonProps, isPressed} = useButton(props, ref);


Pressable props were added inside useButton via the Pressable context

rspbot · 2025-09-10T00:37:55Z

Build successful! 🎉

rspbot · 2025-09-10T00:46:11Z

Build successful! 🎉

reidbarber

Nice! LGTM

LFDanLu · 2025-09-10T22:25:11Z

packages/@react-aria/overlays/src/calculatePosition.ts

+  const containerHeight = (isContainerPositioned ? containerOffsetWithBoundary.height : containerDimensions[TOTAL_SIZE.height]);
+  // For cases where position is set via "bottom" instead of "top", we need to calculate the true overlay top
+  // with respect to the container.
+  let overlayTop = position.top != null ? position.top : (containerHeight - (position.bottom ?? 0) - overlayHeight);


ugh just found this case:

it seems pretty tied to this scroll position where there the trigger is basically flush with the boundary give or take a little bit. https://reactspectrum.blob.core.windows.net/reactspectrum/4097f3c57914b81db847e123fdc996f066df81b3/storybook/index.html?path=/story/react-aria-components-popover--scrolling-boundary-container&providerSwitcher-express=false

decision after chatting, we'll bring this to the team, but since it's not a regression, we can move forward

rspbot · 2025-09-10T22:46:37Z

Build successful! 🎉

rspbot · 2025-09-10T22:46:38Z

## API Changes

@react-spectrum/s2

/@react-spectrum/s2:ActionButton

 ActionButton {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-controls?: string
   aria-current?: boolean | 'true' | 'false' | 'page' | 'step' | 'location' | 'date' | 'time'
   aria-describedby?: string
   aria-details?: string
   aria-disabled?: boolean | 'true' | 'false'
   aria-expanded?: boolean | 'true' | 'false'
   aria-haspopup?: boolean | 'menu' | 'listbox' | 'tree' | 'grid' | 'dialog' | 'true' | 'false'
   aria-label?: string
   aria-labelledby?: string
   aria-pressed?: boolean | 'true' | 'false' | 'mixed'
   autoFocus?: boolean
   children: ReactNode
   excludeFromTabOrder?: boolean
   form?: string
   formAction?: string
   formEncType?: string
   formMethod?: string
   formNoValidate?: boolean
   formTarget?: string
   id?: string
   isDisabled?: boolean
+  isPending?: boolean
   isQuiet?: boolean
   name?: string
   onBlur?: (FocusEvent<Target>) => void
   onFocus?: (FocusEvent<Target>) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
   onPress?: (PressEvent) => void
   onPressChange?: (boolean) => void
   onPressEnd?: (PressEvent) => void
   onPressStart?: (PressEvent) => void
   onPressUp?: (PressEvent) => void
   preventFocusOnPress?: boolean
   size?: 'XS' | 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   staticColor?: 'black' | 'white' | 'auto'
   styles?: StylesProp
   type?: 'button' | 'submit' | 'reset' = 'button'
   value?: string
 }

/@react-spectrum/s2:ActionButtonProps

 ActionButtonProps {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-controls?: string
   aria-current?: boolean | 'true' | 'false' | 'page' | 'step' | 'location' | 'date' | 'time'
   aria-describedby?: string
   aria-details?: string
   aria-disabled?: boolean | 'true' | 'false'
   aria-expanded?: boolean | 'true' | 'false'
   aria-haspopup?: boolean | 'menu' | 'listbox' | 'tree' | 'grid' | 'dialog' | 'true' | 'false'
   aria-label?: string
   aria-labelledby?: string
   aria-pressed?: boolean | 'true' | 'false' | 'mixed'
   autoFocus?: boolean
   children: ReactNode
   excludeFromTabOrder?: boolean
   form?: string
   formAction?: string
   formEncType?: string
   formMethod?: string
   formNoValidate?: boolean
   formTarget?: string
   id?: string
   isDisabled?: boolean
+  isPending?: boolean
   isQuiet?: boolean
   name?: string
   onBlur?: (FocusEvent<Target>) => void
   onFocus?: (FocusEvent<Target>) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
   onPress?: (PressEvent) => void
   onPressChange?: (boolean) => void
   onPressEnd?: (PressEvent) => void
   onPressStart?: (PressEvent) => void
   onPressUp?: (PressEvent) => void
   preventFocusOnPress?: boolean
   size?: 'XS' | 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   staticColor?: 'black' | 'white' | 'auto'
   styles?: StylesProp
   type?: 'button' | 'submit' | 'reset' = 'button'
   value?: string
 }

LFDanLu

LGTM, verified that it also fixes #2874 locally. Checked a couple of other positioning issues but no such luck

snowystinger added 11 commits September 4, 2025 14:30

fix: popover positioning

7edf43f

fix the position

feb6eca

add pending state to action buttons

79efc7b

support a default label slot in s2 picker

1fbe635

make it so cell focus ring is always on top of the content

356292a

actually disable interactions on buttons in pending state, including …

fb185e5

…through context

fix lint

7304bf8

fix more lint

7e1a5eb

fix ActionButton avatar visibility during pending

7a76308

fix lint for real

b2270bd

fix lint for real__FINAL

d3f1749

snowystinger added the ready for review label Sep 4, 2025

snowystinger added 4 commits September 4, 2025 16:27

SelectBoxGroup shouldn't lose focus ring for mouse like checkbox/radi…

336bfc8

…o don't

fix lint...

cdbe1da

fix lint and remove extra console log

959e4c5

fix tests by making them more realistic

9f9eba9

LFDanLu reviewed Sep 4, 2025

View reviewed changes

leave focus events on pending button alone

6170b84

snowystinger added 2 commits September 5, 2025 10:01

Add delays

0df0fbc

longer delay? and click body first?

3d005dc

snowystinger added 2 commits September 5, 2025 10:18

click body before on mobile as well

404caee

cleanup extraneous timeouts

c7b5d94

LFDanLu reviewed Sep 5, 2025

View reviewed changes

Merge branch 'main' into fix-various-bugs

fc7e685

LFDanLu and others added 5 commits September 8, 2025 17:52

scuffed fix, figure out if we should be flipping other instances of c…

0e92920

…ontainerOffsetWithBoundary

make story more useful

2ac39a0

more half fixes for positioning

808b199

fix import

ee68148

fix overlay position!!!!!

d83cf83

snowystinger commented Sep 9, 2025

View reviewed changes

snowystinger and others added 2 commits September 10, 2025 10:29

fix html as the container

229343d

Merge branch 'main' into fix-various-bugs

4097f3c

reidbarber previously approved these changes Sep 10, 2025

View reviewed changes

LFDanLu reviewed Sep 10, 2025

View reviewed changes

fix scrolled case

35bc27b

snowystinger dismissed reidbarber’s stale review via 35bc27b September 10, 2025 22:37

reidbarber approved these changes Sep 10, 2025

View reviewed changes

LFDanLu approved these changes Sep 10, 2025

View reviewed changes

snowystinger added this pull request to the merge queue Sep 10, 2025

Merged via the queue into main with commit 38a7696 Sep 11, 2025
35 checks passed

snowystinger deleted the fix-various-bugs branch September 11, 2025 00:00

snowystinger mentioned this pull request Sep 11, 2025

useOverlayPosition regression for non-viewport boundary/container #6333

Open

fix: Popover positioning and various other bugs #8816

fix: Popover positioning and various other bugs #8816

Uh oh!

Conversation

snowystinger commented Sep 4, 2025 • edited by LFDanLu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

rspbot commented Sep 4, 2025

Uh oh!

rspbot commented Sep 4, 2025

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rspbot commented Sep 4, 2025

Uh oh!

rspbot commented Sep 5, 2025

Uh oh!

rspbot commented Sep 5, 2025

Uh oh!

rspbot commented Sep 5, 2025

Uh oh!

rspbot commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 8, 2025

Uh oh!

rspbot commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 10, 2025

Uh oh!

rspbot commented Sep 10, 2025

Uh oh!

reidbarber left a comment

snowystinger commented Sep 4, 2025 •

edited by LFDanLu

Loading

snowystinger Sep 5, 2025 •

edited

Loading

LFDanLu Sep 10, 2025 •

edited

Loading