adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.99k stars 1.13k forks source link

support scrolling outside of overlay #1852

Open deebov opened 3 years ago

deebov commented 3 years ago

🙋 Feature Request

Support for scrolling outside of overlay.

🤔 Expected Behavior

Overlay should not close if you scroll outside the overlay. And should remain it's position relative to the trigger.

😯 Current Behavior

Overlay closes if you scroll outside it.

💁 Possible Solution

🔦 Context

We want to create a Popover component that does not close on outside scroll and remains its relative position to the trigger.

devongovett commented 3 years ago

I think this will be challenging if not impossible to achieve without jank. Scroll events fire asynchronously, which means we won't be able to match the frame rate of the animation with our repositioning logic. That's why we currently hide the overlay when you scroll outside. Another option we are considering is to prevent scrolling entirely, just like we do for modals. That way it won't be hidden, and the user must click outside first.

davidfurlong commented 3 years ago

Are there any suggested workarounds? This behaviour is largely unexpected by our users. Tbh for us a slightly janky repositioning is still preferable to no option of disabling the current hide behaviour

deebov commented 3 years ago

I think this will be challenging if not impossible to achieve without jank. Scroll events fire asynchronously, which means we won't be able to match the frame rate of the animation with our repositioning logic. That's why we currently hide the overlay when you scroll outside. Another option we are considering is to prevent scrolling entirely, just like we do for modals. That way it won't be hidden, and the user must click outside first.

have you seen the popover implementation from Radix? it seems they don't update popover's position every scroll trigger. Maybe react-aria could also implement or at least consider this approach https://radix-ui.com/primitives/docs/components/popover

snowystinger commented 3 years ago

So I think this has more to do with a lucky happenstance, the popover is positioned in a place relative to the scrolling element, so it never needs updating. If that trigger were inside of a second scrollable section or if the trigger was in a fixed position element, I doubt it would be able to not update every time. I'd need to make a codesandbox or something to verify. I suspect this because they block scrolling while the Menu is open.

marceloadsj commented 3 years ago

For who wants to achieve this: https://codesandbox.io/s/react-aria-popover-open-on-scroll-5q8qu?file=/src/App.js

Warning: this is a "hacky" way, as we are passing down a different "state.close" function than the "useOverlayTrigger" expects.

marceloadsj commented 3 years ago

Btw, @snowystinger is right about how radix works with popover: https://codesandbox.io/s/radix-popover-example-8tycq

When rendering it inside a fixed wrapper, the popover does not keep it's position when we scroll the page. This would need a "updatePosition" call, like we are doing (hacky) on the example above.

marceloadsj commented 3 years ago

So, in summary:

Considering that the popover remains open on scroll to improve the UX


1) rendering it with portal at the end of the body:

wrapper/trigger position: ❌ Fixed: the position should be updated so it became a bit janky ❌ Non-Fixed: the position should be updated so it became a bit janky

✅ non-related content is hidden with aria-hidden

For the "❌ Fixed": maybe this could be turned into ✅ with the popover being "fixed" instead of "absolute"?


2) rendering it without portal, so, closer to the wrapper/trigger:

wrapper/trigger position: ❌ Fixed: the position should be updated so it became a bit janky ✅ Non-Fixed: the position is always right

❌ non-related content is not hidden with aria-hidden

marceloadsj commented 3 years ago

Confirming that the comment for "❌ Fixed" of 1st approach works: https://codesandbox.io/s/react-aria-popover-open-on-scroll-fixed-fkug6?file=/src/App.js

bricejar commented 2 years ago

Would I run into issues if I tried to use radix popover with the datepicker for example ?