react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Popover flips even when there's space to render.

Open endaquigley opened this issue 10 months ago • 6 comments

Provide a general summary of the issue here

The Popover component flips even when there's space to render.

The position calculations don't seem to take the maxHeight prop into account.

https://github.com/user-attachments/assets/70f14846-16e3-4c48-b1a8-94cdde49028f

https://codesandbox.io/p/sandbox/react-aria-popover-placement-57fhmq

🤔 Expected Behavior?

The Popover should only flip when there's not enough room to render the content.

😯 Current Behavior

The Popover component flips even when there's space to render the scrollable content.

I included a 1px margin at the top of the Button so there's more space above the component now. As the Popover can't render the full content it switches to the axis that has more space.

💁 Possible Solution

From what I can tell calculatePositionInternal doesn't take the maxHeight prop into account.

https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/overlays/src/calculatePosition.ts#L355C15-L355C39

🖥️ Steps to Reproduce

  1. Render a Select with a Popover that has a maxHeight prop.
  2. Open the Select and notice how the placement prop is not respected.

Version

1.7.0

What browsers are you seeing the problem on?

Chrome

What operating system are you using?

macOS 15.3.1

endaquigley avatar Mar 10 '25 00:03 endaquigley

Thanks for the issue, the sandbox appears to be private, would you mind making it public?

snowystinger avatar Mar 10 '25 00:03 snowystinger

Ahh, sorry about that - the link should be public now.

endaquigley avatar Mar 10 '25 00:03 endaquigley

No worries, thanks for making one!

It's odd that this is happening, because it should be using the real measured size, so maxHeight shouldn't need to be explicitly taken into account. https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/overlays/src/calculatePosition.ts#L150

It appears as though the scrollSize is wrong because it's looking at the child of the Popover, not the Popover itself. You can work around it for now by doing something like https://codesandbox.io/p/sandbox/react-aria-popover-placement-forked-p237ty where I've moved the maxHeight down to the ListBox and the overflow there as well.

We'll have to look into how we want to handle this. Thanks for reporting.

EDIT: I accidentally left shouldFlip=false on in the sandbox when I wrote this. Moving the maxheight and overflow down an element did not address it. We'll need to investigate what's going on.

snowystinger avatar Mar 10 '25 01:03 snowystinger

Thanks for your quick response. I'm not sure if you noticed but you have have shouldFlip={false} set in the previous example. I think I'll stick with the current behaviour for now as that prop can result in some awful looking dropdowns 😅

endaquigley avatar Mar 10 '25 01:03 endaquigley

omg... yes, I do still have that. I have tricked myself.

snowystinger avatar Mar 10 '25 01:03 snowystinger