-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Automatically bound virtualizer visible rectangle to window viewport #9784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
| */ | ||
|
|
||
| import {getScrollParents} from './getScrollParents'; | ||
| import {isChrome, isIOS} from './platform'; | ||
| import {isIOS} from './platform'; | ||
|
|
||
| interface ScrollIntoViewOpts { | ||
| /** The position to align items along the block axis in. */ | ||
|
|
@@ -133,9 +133,7 @@ export function scrollIntoViewport(targetElement: Element | null, opts: ScrollIn | |
| if (targetElement && targetElement.isConnected) { | ||
| let root = document.scrollingElement || document.documentElement; | ||
| let isScrollPrevented = window.getComputedStyle(root).overflow === 'hidden'; | ||
| // 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) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixes scroll into view when table is taller than the viewport. Underlying chrome bug is now fixed: https://issues.chromium.org/issues/40074749
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, why did the shim not work correctly there? Could maybe be addressed in #9780?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't dive too deep into it, but I think it's the same issue Daniel was addressing. |
||
| let {left: originalLeft, top: originalTop} = targetElement.getBoundingClientRect(); | ||
|
|
||
| // use scrollIntoView({block: 'nearest'}) instead of .focus to check if the element is fully in view or not since .focus() | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, I was creating a |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,9 @@ | |
|
|
||
| // @ts-ignore | ||
| import {flushSync} from 'react-dom'; | ||
| import {getEventTarget, useEffectEvent, useEvent, useLayoutEffect, useObjectRef, useResizeObserver} from '@react-aria/utils'; | ||
| import {getEventTarget, nodeContains, useEffectEvent, useLayoutEffect, useObjectRef, useResizeObserver} from '@react-aria/utils'; | ||
| import {getScrollLeft} from './utils'; | ||
| import {Point, Rect, Size} from '@react-stately/virtualizer'; | ||
| import React, { | ||
| CSSProperties, | ||
| ForwardedRef, | ||
|
|
@@ -25,17 +26,17 @@ import React, { | |
| useRef, | ||
| useState | ||
| } from 'react'; | ||
| import {Rect, Size} from '@react-stately/virtualizer'; | ||
| import {useLocale} from '@react-aria/i18n'; | ||
|
|
||
| interface ScrollViewProps extends HTMLAttributes<HTMLElement> { | ||
| interface ScrollViewProps extends Omit<HTMLAttributes<HTMLElement>, 'onScroll'> { | ||
| contentSize: Size, | ||
| onVisibleRectChange: (rect: Rect) => void, | ||
| children?: ReactNode, | ||
| innerStyle?: CSSProperties, | ||
| onScrollStart?: () => void, | ||
| onScrollEnd?: () => void, | ||
| scrollDirection?: 'horizontal' | 'vertical' | 'both' | ||
| scrollDirection?: 'horizontal' | 'vertical' | 'both', | ||
| onScroll?: (e: Event) => void | ||
| } | ||
|
|
||
| function ScrollView(props: ScrollViewProps, ref: ForwardedRef<HTMLDivElement | null>) { | ||
|
|
@@ -70,39 +71,76 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |
| onScrollStart, | ||
| onScrollEnd, | ||
| scrollDirection = 'both', | ||
| onScroll: onScrollProp, | ||
| ...otherProps | ||
| } = props; | ||
|
|
||
| let state = useRef({ | ||
| scrollTop: 0, | ||
| scrollLeft: 0, | ||
| // Internal scroll position of the scroll view. | ||
| scrollPosition: new Point(), | ||
| // Size of the scroll view. | ||
| size: new Size(), | ||
| // Offset of the scroll view relative to the window viewport. | ||
| viewportOffset: new Point(), | ||
| // Size of the window viewport. | ||
| viewportSize: new Size(), | ||
| scrollEndTime: 0, | ||
| scrollTimeout: null as ReturnType<typeof setTimeout> | null, | ||
| width: 0, | ||
| height: 0, | ||
| isScrolling: false | ||
| }).current; | ||
| let {direction} = useLocale(); | ||
|
|
||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oo, no more required explicit height, nice!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LFDanLu I think this should simplify our tests as well, we can more easily mock the virtualizer size vs the item sizes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, the |
||
| // While there may be other scrollable elements between the <body> and the scroll view, we do not take | ||
| // their sizes into account for performance reasons. Their scroll positions are accounted for in viewportOffset | ||
| // though (due to getBoundingClientRect). This may result in more rows than absolutely necessary being rendered, | ||
| // but no more than the entire height of the viewport which is good enough for virtualization use cases. | ||
| 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)), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when is this negative?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. viewportOffset may be greater than the size of the scrollview (when it is entirely scrolled out of view) |
||
| Math.max(0, Math.min(state.size.height - state.viewportOffset.y, state.viewportSize.height)) | ||
| ); | ||
| onVisibleRectChange(visibleRect); | ||
| }, [state, onVisibleRectChange]); | ||
|
|
||
| let [isScrolling, setScrolling] = useState(false); | ||
|
|
||
| let onScroll = useCallback((e) => { | ||
| if (getEventTarget(e) !== e.currentTarget) { | ||
| let onScroll = useCallback((e: Event) => { | ||
| let target = getEventTarget(e) as Element; | ||
| if (!nodeContains(target, ref.current!)) { | ||
| return; | ||
| } | ||
|
|
||
| if (props.onScroll) { | ||
| props.onScroll(e); | ||
| if (onScrollProp && target === ref.current) { | ||
| onScrollProp(e); | ||
| } | ||
|
|
||
| flushSync(() => { | ||
| let scrollTop = e.currentTarget.scrollTop; | ||
| let scrollLeft = getScrollLeft(e.currentTarget, direction); | ||
| if (target !== ref.current) { | ||
| // An ancestor element or the window was scrolled. Update the position of the scroll view relative to the viewport. | ||
| let boundingRect = ref.current!.getBoundingClientRect(); | ||
| let x = boundingRect.x < 0 ? -boundingRect.x : 0; | ||
| let y = boundingRect.y < 0 ? -boundingRect.y : 0; | ||
| if (x === state.viewportOffset.x && y === state.viewportOffset.y) { | ||
| return; | ||
| } | ||
|
|
||
| state.viewportOffset = new Point(x, y); | ||
| } else { | ||
| // The scroll view itself was scrolled. Update the local scroll position. | ||
| // Prevent rubber band scrolling from shaking when scrolling out of bounds | ||
| state.scrollTop = Math.max(0, Math.min(scrollTop, contentSize.height - state.height)); | ||
| state.scrollLeft = Math.max(0, Math.min(scrollLeft, contentSize.width - state.width)); | ||
| onVisibleRectChange(new Rect(state.scrollLeft, state.scrollTop, state.width, state.height)); | ||
| let scrollTop = target.scrollTop; | ||
| 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)) | ||
| ); | ||
|
Comment on lines
+135
to
+139
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, this logic does not work in various edge cases. See #9115 for changes done to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the logic has not changed in this PR, but what are the cases? I couldn't easily tell from the PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clamping needs to be aware of the offset type, due to overscroll. I moved clamping into 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 😉 |
||
| } | ||
|
|
||
| flushSync(() => { | ||
| updateVisibleRect(); | ||
|
|
||
| if (!state.isScrolling) { | ||
| state.isScrolling = true; | ||
|
|
@@ -138,10 +176,13 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |
| }, 300); | ||
| } | ||
| }); | ||
| }, [props, direction, state, contentSize, onVisibleRectChange, onScrollStart, onScrollEnd]); | ||
| }, [onScrollProp, ref, direction, state, contentSize, updateVisibleRect, onScrollStart, onScrollEnd]); | ||
|
|
||
| // Attach event directly to ref so RAC Virtualizer doesn't need to send props upward. | ||
| useEvent(ref, 'scroll', onScroll); | ||
| // 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); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| }, [onScroll]); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
|
|
@@ -175,11 +216,18 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |
| let w = isTestEnv && !isClientWidthMocked ? Infinity : clientWidth; | ||
| let h = isTestEnv && !isClientHeightMocked ? Infinity : clientHeight; | ||
|
|
||
| if (state.width !== w || state.height !== h) { | ||
| state.width = w; | ||
| state.height = h; | ||
| // Update the window viewport size. | ||
| let viewportWidth = window.innerWidth; | ||
| let viewportHeight = window.innerHeight; | ||
| let viewportSizeChanged = state.viewportSize.width !== viewportWidth || state.viewportSize.height !== viewportHeight; | ||
| if (viewportSizeChanged) { | ||
| state.viewportSize = new Size(viewportWidth, viewportHeight); | ||
| } | ||
|
|
||
| if (state.size.width !== w || state.size.height !== h || viewportSizeChanged) { | ||
| state.size = new Size(w, h); | ||
| flush(() => { | ||
| onVisibleRectChange(new Rect(state.scrollLeft, state.scrollTop, w, h)); | ||
| updateVisibleRect(); | ||
| }); | ||
|
|
||
| // If the clientWidth or clientHeight changed, scrollbars appeared or disappeared as | ||
|
|
@@ -188,18 +236,30 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |
| // again, resulting in extra padding. We stop after a maximum of two layout passes to avoid | ||
| // an infinite loop. This matches how browsers behavior with native CSS grid layout. | ||
| if (!isTestEnv && clientWidth !== dom.clientWidth || clientHeight !== dom.clientHeight) { | ||
| state.width = dom.clientWidth; | ||
| state.height = dom.clientHeight; | ||
| state.size = new Size(dom.clientWidth, dom.clientHeight); | ||
| flush(() => { | ||
| onVisibleRectChange(new Rect(state.scrollLeft, state.scrollTop, state.width, state.height)); | ||
| updateVisibleRect(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| isUpdatingSize.current = false; | ||
| }, [ref, state, onVisibleRectChange]); | ||
| }, [ref, state, updateVisibleRect]); | ||
| let updateSizeEvent = useEffectEvent(updateSize); | ||
|
|
||
| // 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This includes scrollbars. We might want to use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| let onWindowResize = () => { | ||
| updateSizeEvent(flushSync); | ||
| }; | ||
|
|
||
| window.addEventListener('resize', onWindowResize); | ||
| return () => window.removeEventListener('resize', onWindowResize); | ||
| }, [state]); | ||
|
|
||
| // Update visible rect when the content size changes, in case scrollbars need to appear or disappear. | ||
| let lastContentSize = useRef<Size | null>(null); | ||
| let [update, setUpdate] = useState({}); | ||
|
|
@@ -250,7 +310,7 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |
| if (scrollDirection === 'horizontal') { | ||
| style.overflowX = 'auto'; | ||
| style.overflowY = 'hidden'; | ||
| } else if (scrollDirection === 'vertical' || contentSize.width === state.width) { | ||
| } else if (scrollDirection === 'vertical' || contentSize.width === state.size.width) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like it was added in #5380
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oof, I do not unfortunately, but if memory serves it would happen when stepping through various browser zoom levels. |
||
| // Set overflow-x: hidden if content size is equal to the width of the scroll view. | ||
| // This prevents horizontal scrollbars from flickering during resizing due to resize observer | ||
| // firing slower than the frame rate, which may cause an infinite re-render loop. | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need these? At a glance the Virtualizer type has this as optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a type error because
collectionPropsandlistBoxPropshaveHTMLAttributes, which uses React'sonScrolltype, which is aUIEvent. ButVirtualizerneeds the native scroll event, which is justEvent(idk why React is different). This was not caught before because theonScrolllistener insideScrollViewwas untyped (ewas implicitlyany).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
UIEventinstance anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was already true but was not correctly reflected in the types
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what I meant. Your change to
useScrollViewL74 pulls this prop out fromotherProps, where as before it was both passthrough'd AND invoked via props.onScroll. This means a user relying on receiving aUIEventwont do so anymore after this change.I specifically excluded
onScrollfrom being passed touseScrollViewin #9115 because of that. Or what am I missing? Why was this already true?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh that's true but I checked that
collectionPropsandlistBoxPropsdo not have anonScrollhandlerUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.