feat: Automatically bound virtualizer visible rectangle to window viewport#9784
feat: Automatically bound virtualizer visible rectangle to window viewport#9784devongovett wants to merge 5 commits intomainfrom
Conversation
| // Attach a document-level capturing scroll listener so we can account for scrollable ancestors. | ||
| useEffect(() => { | ||
| document.addEventListener('scroll', onScroll, true); | ||
| return () => document.removeEventListener('scroll', onScroll, true); |
There was a problem hiding this comment.
Using a capturing scroll listener seems better than attaching scroll listeners to all ancestor elements, but does mean we will receive events for potentially unrelated elements. The nodeContains check inside onScroll should account for this and quickly return early, but we should do some performance testing here.
|
Build successful! 🎉 |
| // If scrolling is not currently prevented then we aren't in a overlay nor is a overlay open, just use element.scrollIntoView to bring the element into view | ||
| // Also ignore in chrome because of this bug: https://issues.chromium.org/issues/40074749 | ||
| if (!isScrollPrevented && !isChrome()) { | ||
| if (!isScrollPrevented) { |
There was a problem hiding this comment.
Fixes scroll into view when table is taller than the viewport. Underlying chrome bug is now fixed: https://issues.chromium.org/issues/40074749
There was a problem hiding this comment.
Hm, why did the shim not work correctly there? Could maybe be addressed in #9780?
There was a problem hiding this comment.
I didn't dive too deep into it, but I think it's the same issue Daniel was addressing.
|
Build successful! 🎉 |
| <Context.Provider value={{state, dropState}}> | ||
| <Virtualizer | ||
| {...mergeProps(collectionProps, listBoxProps)} | ||
| onScroll={undefined} |
There was a problem hiding this comment.
do we need these? At a glance the Virtualizer type has this as optional
There was a problem hiding this comment.
there was a type error because collectionProps and listBoxProps have HTMLAttributes, which uses React's onScroll type, which is a UIEvent. But Virtualizer needs the native scroll event, which is just Event (idk why React is different). This was not caught before because the onScroll listener inside ScrollView was untyped (e was implicitly any).
There was a problem hiding this comment.
IIRC, this could be a breaking change in the way its handled here? Before, we invoked the callback twice actually, once with a native event and once in our callback. Now the user wouldn't be receiving a UIEvent instance anymore.
There was a problem hiding this comment.
this was already true but was not correctly reflected in the types
There was a problem hiding this comment.
That's not what I meant. Your change to useScrollView L74 pulls this prop out from otherProps, where as before it was both passthrough'd AND invoked via props.onScroll. This means a user relying on receiving a UIEvent wont do so anymore after this change.
I specifically excluded onScroll from being passed to useScrollView in #9115 because of that. Or what am I missing? Why was this already true?
There was a problem hiding this comment.
oh that's true but I checked that collectionProps and listBoxProps do not have an onScroll handler
There was a problem hiding this comment.
That's true, but someone may be using react-arias Virtualizer or ScrollView independently. In any way, I just wanted to raise the issue because I remembered being bummed about having to make that backwards compatible 😅. It might not be a big deal to break, idk.
There was a problem hiding this comment.
oops, I was creating a Rect instead of a Size and typescript wasn't catching it. 🤦
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:TreeEmptyStateRenderProps-TreeEmptyStateRenderProps {
- allowsDragging: boolean
- isFocusVisible: boolean
- isFocused: boolean
- selectionMode: SelectionMode
- state: TreeState<unknown>
-}@react-aria/utils/@react-aria/utils:getNonce-getNonce {
- doc?: Document
- returnVal: undefined
-}@react-aria/virtualizer/@react-aria/virtualizer:Virtualizer Virtualizer <O, T extends {}, V extends ReactNode> extends HTMLAttributes {
children: (string, {}) => ReactNode
collection: Collection<{}>
isLoading?: boolean
layout: Layout<{}, O>
layoutOptions?: O
onLoadMore?: () => void
+ onScroll?: (Event) => void
persistedKeys?: Set<Key> | null
renderWrapper?: RenderWrapper<{}, ReactNode>
scrollDirection?: 'horizontal' | 'vertical' | 'both'
}/@react-aria/virtualizer:ScrollView ScrollView extends HTMLAttributes {
children?: ReactNode
contentSize: Size
innerStyle?: CSSProperties
+ onScroll?: (Event) => void
onScrollEnd?: () => void
onScrollStart?: () => void
onVisibleRectChange: (Rect) => void
scrollDirection?: 'horizontal' | 'vertical' | 'both' |
|
|
||
| let updateVisibleRect = useCallback(() => { | ||
| // Intersect the window viewport with the scroll view itself to find the actual visible rectangle. | ||
| // This allows virtualized components to have unbounded height but still virtualize when scrolled with the page. |
There was a problem hiding this comment.
oo, no more required explicit height, nice!
I assume it applies to widths as well
There was a problem hiding this comment.
@LFDanLu I think this should simplify our tests as well, we can more easily mock the virtualizer size vs the item sizes
There was a problem hiding this comment.
Btw, the VIRT_ON flag breaks in certain scenarios. I addressed these cases with further improvements specific to virtualizer in test environments within #9115.
| let visibleRect = new Rect( | ||
| state.viewportOffset.x + state.scrollPosition.x, | ||
| state.viewportOffset.y + state.scrollPosition.y, | ||
| Math.max(0, Math.min(state.size.width - state.viewportOffset.x, state.viewportSize.width)), |
There was a problem hiding this comment.
viewportOffset may be greater than the size of the scrollview (when it is entirely scrolled out of view)
| let scrollLeft = getScrollLeft(target, direction); | ||
| state.scrollPosition = new Point( | ||
| Math.max(0, Math.min(scrollLeft, contentSize.width - state.size.width)), | ||
| Math.max(0, Math.min(scrollTop, contentSize.height - state.size.height)) | ||
| ); |
There was a problem hiding this comment.
FYI, this logic does not work in various edge cases. See #9115 for changes done to getScrollOffset.ts and getOffsetType.ts.
There was a problem hiding this comment.
the logic has not changed in this PR, but what are the cases? I couldn't easily tell from the PR.
There was a problem hiding this comment.
Clamping needs to be aware of the offset type, due to overscroll. I moved clamping into getScrollLeft and also added the same for getScrollTop, because of flex: <row/col>-reverse.
For example, in RTL layouts, we can get a negative float here, due to overscroll at the lower boundary.
Lastly, I noted that we probably should be more strict about these utilities being used everywhere we access or set scroll offsets. Often times there are subtle bugs introduced because of not doing so. One of the reasons why moving scroll observation into utils makes sense imo 😉
| // Track the size of the entire window viewport, which is used to bound the size of the virtualizer's visible rectangle. | ||
| useLayoutEffect(() => { | ||
| // Initialize viewportRect before updating size for the first time. | ||
| state.viewportSize = new Size(window.innerWidth, window.innerHeight); |
There was a problem hiding this comment.
This includes scrollbars. We might want to use document.scrollingElement.clientWidth/Height? Or was there specific intent to keep it tied to the window object for test environments?
There was a problem hiding this comment.
it's simpler to mock, and accuracy is not really important here because this is simply an upper-bound on the number of items to display.
| style.overflowX = 'auto'; | ||
| style.overflowY = 'hidden'; | ||
| } else if (scrollDirection === 'vertical' || contentSize.width === state.width) { | ||
| } else if (scrollDirection === 'vertical' || contentSize.width === state.size.width) { |
There was a problem hiding this comment.
Unrelated, but a question I raised in #9115. Do you still happen to carry reproduction steps for this happening? I couldn't see this issue in my testing after refactoring.
There was a problem hiding this comment.
oof, I do not unfortunately, but if memory serves it would happen when stepping through various browser zoom levels.
| // If scrolling is not currently prevented then we aren't in a overlay nor is a overlay open, just use element.scrollIntoView to bring the element into view | ||
| // Also ignore in chrome because of this bug: https://issues.chromium.org/issues/40074749 | ||
| if (!isScrollPrevented && !isChrome()) { | ||
| if (!isScrollPrevented) { |
There was a problem hiding this comment.
Hm, why did the shim not work correctly there? Could maybe be addressed in #9780?
| <Context.Provider value={{state, dropState}}> | ||
| <Virtualizer | ||
| {...mergeProps(collectionProps, listBoxProps)} | ||
| onScroll={undefined} |
There was a problem hiding this comment.
IIRC, this could be a breaking change in the way its handled here? Before, we invoked the callback twice actually, once with a native event and once in our callback. Now the user wouldn't be receiving a UIEvent instance anymore.
|
|
||
| let updateVisibleRect = useCallback(() => { | ||
| // Intersect the window viewport with the scroll view itself to find the actual visible rectangle. | ||
| // This allows virtualized components to have unbounded height but still virtualize when scrolled with the page. |
There was a problem hiding this comment.
Btw, the VIRT_ON flag breaks in certain scenarios. I addressed these cases with further improvements specific to virtualizer in test environments within #9115.

Closes #3965, closes #7820
This enables virtualized components to be placed within ancestor scrolling elements, including the entire page, and still virtualize their children automatically, without needing a bounded height on the virtualizer itself. It works by intersecting the visible rectangle with the window viewport, accounting for the scroll position of all ancestor elements. We use
getBoundingClientRectto get the offset of the element relative to the viewport, and bound by the window size. We do not take the sizes of all ancestor elements into account for performance reasons, so there may be a few more elements rendered than absolutely necessary in some cases, but never more than the height of the window so this shouldn't be a big issue. This means it "just works" without any configuration of which element is scrollable.Test instructions
Test new S2 TableView stories