useOverlayPosition: user-defined maxHeight is ignored when userSetMaxHeight < maxHeight

[LosYear]

🙋 Feature Request

I'm working on integrating react-aria into out existing project.

I'm looking for some motivation beneath this decision: #1500 (comment)

It's a common behaviour for us to let users scroll the page if the dropdown with e.g min-height: 300px can't be rendered into viewport. But calculated max-height seems to crop the popup/dropdown to fit the screen.

Although it can be solved either with setting maxHeight bigger that calculated or deleting max-height property from returned styles object, it can't be done that way because of this line:

react-spectrum/packages/@react-aria/overlays/src/useOverlayPosition.ts

Line 149 in 273e828

Object.keys(position.position).forEach(key => (overlayRef.current as HTMLElement).style[key] = position.position[key] + 'px');

Is there any motivation beneath this decision that I'm not taking into consideration. Or can we somehow patch this place (e.g when maxHeight=null or smth like that) to handle this case?

I'm ready to submit a PR but wondering what way is preferred to handle it

–––
Another dirty dirty hack to overcome this problem is to set maxHeight: -Infinity

[LFDanLu]

Historically we either closed a open popover on scroll and/or prevent scrolling of the page while said popover was open, hence why we don't support popovers that extend past the viewport.

[LosYear]

Closing popover on scroll seems legit for most cases.

But I understand preventing scroll while modal elements are opened, such as dialogs, but it seems uncommon behaviour to prevent scroll when popup (popover) is opened? Maybe you have some quotes from any a11y guides for such logic?

[LFDanLu]

From an accessibility perspective, I believe preventing scrolling when a popover is less desirable than allowing scrolling since there are cases where a popover's contents may fall outside the viewport (e.g. 400% zoom). We closed popovers on scroll previously not so much for accessibility but because there was some janky repositioning happening. This behavior then changed to preventing page scrolling due to the usability issues with the previous approach as mentioned here.