react-spectrum
react-spectrum copied to clipboard
adobe
fix(#2766): Virtualizer does not scroll items into view within the browser viewport
Add IntersectionObserver to detect whether or not a focused item is visible within the browser window, and if not scroll it into view.
Closes #2766
✅ Pull Request Checklist:
- [x] Included link to corresponding [React Spectrum GitHub Issue #2766.
- [x] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
- [x] Filled out test instructions.
- [ ] Updated documentation (if it already exists for this component).
- [x] Looked at the Accessibility Practices for this feature - Aria Practices
📝 Test Instructions:
- Open https://reactspectrum.blob.core.windows.net/reactspectrum/5781218176b64217f00332ae3d86388f6ec0ac7e/storybook/index.html?path=/story/listbox--async-loading&id=listbox--async-loading&providerSwitcher-toastPosition=bottom&viewMode=story
- Reduce the inner height of the browser window to be less than the height of the ListBox within the story, ~200px should be good.
- Tab to set keyboard focus to the first Item within the ListBox.
- Use ArrowDown to scroll until you focus an Item within the ListBox that is outside of the browser viewport.
- Notice that the browser will now automatically scroll the
document.bodyto bring the focused Item into view.
Other stories to test:
- https://reactspectrum.blob.core.windows.net/reactspectrum/5781218176b64217f00332ae3d86388f6ec0ac7e/storybook/index.html?path=/story/tableview--async-loading&id=tableview--async-loading&providerSwitcher-toastPosition=bottom&viewMode=story
- https://reactspectrum.blob.core.windows.net/reactspectrum/5781218176b64217f00332ae3d86388f6ec0ac7e/storybook/index.html?path=/story/useselectablelist--static-ul-static-sub-ul&id=useselectablelist--static-ul-static-sub-ul&providerSwitcher-toastPosition=bottom&viewMode=story
- https://reactspectrum.blob.core.windows.net/reactspectrum/5781218176b64217f00332ae3d86388f6ec0ac7e/storybook/index.html?path=/story/focusscope--focus-safely&id=focusscope--focus-safely&providerSwitcher-toastPosition=bottom&viewMode=story
🧢 Your Project:
Adobe/Accessibility Adobe/Admin_Console Jira issues A11Y-5009 and TRON-8092
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
This issue is not specific to Virtualizer. For example, use your same reproduction steps on this example.
The root cause is that we use focusSafely here, which prevents scroll. That is very much intentional (e.g. #317). We currently have another bug where we scroll into view where we don't want to (when tapping a card). I'm not sure that scrolling into view always the behavior we want. I can think of cases e.g. involving popovers where this would cause the popover to close, for example. Will need to think about it more.
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
This seems to work well enough. I'm a bit concerned about how the scrolling may close dropdowns/popovers which could pose a problem for react-aria users who use
useOverlayPosition, specifically if they have something likeshouldFlip: falsecombined with a positioning that would render the dropdown/popover offscreen (reproducible by modifying theuseOverlayTriggerdocs example withshouldFlip:falseandposition: bottom), perhaps edge casey? Our components sidestep this since the overlays don't render offscreen
Hmmm... The behavior one can reproduce adding shouldFlip:false and position: bottom to the useOverlayPosition configuration for an overlay trigger near the bottom of the page seems to be a problem with useOverlayPosition and useCloseOnScroll. We should either not close a popover on scroll when shouldFlip:false, or force flipping when space is not available to render the overlay completely without scrolling.
We will have accessibility problems with any popovers that cannot fit with the available viewport if there is no way to scroll the document body to read the popover content that does not fit without closing the popover. In fact, even without the changes to focusSafely introduced with this PR, the shouldFlip:false and position: bottom example would be unusable by anyone, because a user will not be able to scroll popover content that doesn't fit within the viewport into view without closing the popover. This is also a significant barrier to access for users who zoom in or magnify the browser.
This issue is not specific to Virtualizer. For example, use your same reproduction steps on this example.
The root cause is that we use
focusSafelyhere, which prevents scroll. That is very much intentional (e.g. #317). We currently have another bug where we scroll into view where we don't want to (when tapping a card). I'm not sure that scrolling into view always the behavior we want. I can think of cases e.g. involving popovers where this would cause the popover to close, for example. Will need to think about it more.
I think #317 is treating a symptom rather than the underlying issue. useCloseOnScroll was added to avoid having to reposition an overlay rendered to a portal on scroll. I don't think this is the correct approach. For accessibility, a user should be able to scroll the content of an overlay that extends beyond the viewport into view without closing the overlay. With useCloseOnScroll, as currently implemented, this is not possible.
I don't follow. Popovers should never extend beyond the viewport. The content of the popover itself is scrollable, not the document body.
Regardless, closing on scroll has very little to do with this and it is far from the only reason for the current behavior. In fact, this is the first I’m hearing of it in relation to this PR. We used the preventScroll option long before #317, that just polyfilled it for iOS.
I don't follow. Popovers should never extend beyond the viewport. The content of the popover itself is scrollable, not the document body.
Regardless, closing on scroll has very little to do with this and it is far from the only reason for the current behavior. In fact, this is the first I’m hearing of it in relation to this PR. We used the
preventScrolloption long before #317, that just polyfilled it for iOS.
They should never extend beyond the viewport, except when they do or should; if a user has used pinch and zoom on a mobile device, the contents of an overlay, like ContextualHelp for example, extend beyond the browser viewport, or in the use case @LFDanLu sites above, shouldFlip:false and position: bottom. focusWithoutScrolling is useful in some cases, I just don't think it should replace browser behavior in all cases. I think it is reasonable to have a table or listbox that is taller than the available browser viewport, and in those cases a keyboard users should expect items receiving focus within the table or listbox to scroll into view within both the table or listbox and the browser.
if a user has used pinch and zoom on a mobile device, the contents of an overlay, like ContextualHelp for example, extend beyond the browser viewport
If that's the case, then it is a bug. If you zoom in a desktop browser, we still keep overlays within the viewport. The behavior should be the same on mobile. Otherwise it's really hard to use if the overlay goes outside the viewport. Definitely not intentional.
focusWithoutScrolling is useful in some cases, I just don't think it should replace browser behavior in all cases.
Yes, again, focusWithoutScrolling is just a polyfill of the preventScroll option of the browser focus method. You don't want items scrolling under your pointer when using a mouse or touch screen, so it makes sense that this is used. Agree that for keyboard you want items to scroll into view, but I don't think it's as simple as just changing that option. For virtualizer, we have the additional complexity that the item you want to scroll into view might not even be in the DOM, so we cannot simply rely on browser behavior there.
@devongovett I wonder if I should revert the IntersectionObserver logic back to to the Virtualizer, rather than focusSafely, or make it a separate hook that gets added as needed to controls that should scroll into view within the browser after receiving focus.
Build successful! 🎉
@devongovett and @LFDanLu What I'm trying to say is that the shouldFlip:false and position: bottom use case Daniel describes above would be a broken experience even without the changes in this PR, so it's kind of a moot point.
I don't think of focusSafely as the same as focusWithoutScrolling, even though it currently uses it. In a virtualizer we don't want mouse focus to cause the browser to scroll a focused element. I understand this. However, I don't think focusSafely should completely override a user's expectation for the browser to automatically scroll an item receiving focus into view. The PR is designed to first focusWithoutScrolling, and then once the item has received focus without causing jank, and excluding focus after pointer events, verify that the focused element is within view, and if not, scroll it into view.
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉
Build successful! 🎉