Refactor Virtualizer to improve performance, stability, and complexity #6451
Conversation
| // Dispatch a custom event that parent elements can intercept to customize focus restoration. | ||
| // For example, virtualized collection components reuse DOM elements, so the original element | ||
| // might still exist in the DOM but representing a different item. | ||
| if (node.dispatchEvent(new CustomEvent(RESTORE_FOCUS_EVENT, {bubbles: true, cancelable: true}))) { |
There was a problem hiding this comment.
This custom event fixes restoring focus when closing an ActionBar, which previously only worked because of the animation removing rows in TableView. Without the animation, row elements are reused to represent a different object immediately, and focus appears to be restored to the wrong item. Dispatching a custom event allows useSelectableCollection to pick this up and apply its own logic instead of the default provided by FocusScope.
| @@ -293,6 +293,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions | |||
| }; | |||
|
|
|||
| // Store the scroll position so we can restore it later. | |||
| /// TODO: should this happen all the time?? | |||
There was a problem hiding this comment.
I can't remember why we did this or what it fixed. Maybe it isn't needed anymore??
There was a problem hiding this comment.
Well, I thought it was for this flow:
Go to collection and focus some item
Tab out of the collection
Scroll the collection (scroll wheel or track pad, anything that doesn't put focus back into the collection)
Tab back to the collection
Focus should go to the last focused Item
It looks like it's still working if I remove it though. I thought maybe it wasn't needed anymore because persistedKeys took care of it. This code was added before persistedKeys, however, it appeared to work before as well.
Maybe @LFDanLu will remember, the only note was a comment from him referencing an issue he tried to solve previously #2233
There was a problem hiding this comment.
So you can reproduce by getting rid of the scrollPos references completely and by using the "useTable -> Scroll testing" story. Keyboard nav into the table and focus any cell that isn't in the first row, then tab out and shift tab back into the table:
Screen.Recording.2024-05-29.at.5.05.56.PM.mov
If my memory + notes on the PR serves, this is due to how focus lands on the last checkbox in the table before being marshalled to the proper tracked focused key -> causes a scroll position change in the scrollable body -> messes up scrollIntoView calculations. Think the same issue happens if shift tabbing to a previously focused table column as well
| @@ -130,41 +130,6 @@ export class GridLayout<T> extends BaseLayout<T> { | |||
| ); | |||
| } | |||
|
|
|||
| getVisibleLayoutInfos(rect) { | |||
There was a problem hiding this comment.
Handled by base class. Previously this implementation did not account for persisted keys.
| if (type === 'item') { | ||
| return ( | ||
| <> | ||
| {isListDroppable && collection.getKeyBefore(item.key) == null && |
There was a problem hiding this comment.
Splitting lots of this out into separate components. The callback passed to virtualizer is memoized, so we can't rely on the values accessed from closure scope being correct here. Moving it into separate components and accessing what we need via context fixes this.
| @@ -293,6 +293,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions | |||
| }; | |||
|
|
|||
| // Store the scroll position so we can restore it later. | |||
| /// TODO: should this happen all the time?? | |||
There was a problem hiding this comment.
Well, I thought it was for this flow:
Go to collection and focus some item
Tab out of the collection
Scroll the collection (scroll wheel or track pad, anything that doesn't put focus back into the collection)
Tab back to the collection
Focus should go to the last focused Item
It looks like it's still working if I remove it though. I thought maybe it wasn't needed anymore because persistedKeys took care of it. This code was added before persistedKeys, however, it appeared to work before as well.
Maybe @LFDanLu will remember, the only note was a comment from him referencing an issue he tried to solve previously #2233
| onFocusedResizer, | ||
| headerMenuOpen, | ||
| setHeaderMenuOpen, | ||
| renderEmptyState: props.renderEmptyState |
There was a problem hiding this comment.
this is a huge context that will update a lot, should we split it up?
maybe one for drag and drop stuff, one for virtualizer stuff, one for state, and one for resizing?
There was a problem hiding this comment.
yeah we should. I think I only added one key to it though, was just on a single line before 😉
There was a problem hiding this comment.
o yeah, i know you aren't responsible for this ridiculous context, haha. I'm fairly certain that would be me ;)
|
Can we fix #4382 as well? At present, we do not support sections in TableView; The rendered content within the role="presentation" on the ScrollView at
with role="rowgroup" at (
. We can add a comment explaining this:
/*
Firefox and Chrome make generic elements using CSS overflow 'scroll' or 'auto' tabbable,
including them within the accessibility tree, which breaks the table structure in Firefox,
Using tabIndex={-1} prevents the ScrollView from being tabbable, and using role="rowgroup"
and role="presentation" on the table body content fixes the table structure.
*/
role="rowgroup"
tabIndex={isVirtualDragging ? null : -1} |
|
Added some additional performance improvements found while profiling an example project from Workfront.
This together with the previous optimizations resulted in a huge improvement on the added storybook example: average per-frame (scroll event) render time went from ~70ms to ~4ms. The example has 10,000 rows and 50 columns, with complex cell content such as tags, status lights, links, buttons, etc., and it now scrolls smoothly with no dropped frames. |
|
Found a little more odd behavior, as I keyboard navigate rows, the tag group seems to also navigate? |
I don't think that's new right? |
| let renderWrapper = (parent: View, reusableView: View, children: View[], renderChildren: (views: View[]) => ReactElement[]) => { | ||
| let style = layoutInfoToStyle(reusableView.layoutInfo, direction, parent && parent.layoutInfo); | ||
| if (style.overflow === 'hidden') { | ||
| style.overflow = 'visible'; // needed to support position: sticky | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks like the cellWrapper doesn't get overflow: visible anymore, it is cutting off the resizer's right half:
Maybe the cellwrapper needs useStyle?
There was a problem hiding this comment.
Another thing I found was the focus rings in the hide header are cut off now, doesn't happen on main:
Same root issue with the cellWrapper not having overflow: 'visible'
| let overscanY = this.visibleRect.height / 3; | ||
| overscanned.height += overscanY; |
There was a problem hiding this comment.
Curious about the math here, is this essentially signalling to the virtualizer that the available rect is an additional 1/3 of the visible rect bounds in the direction of the scrolling Just saw you additional comment. Was the 1/3 just a number that felt reasonable performance wise? Is there any downside to returning 0 for overscanY if the velocity is 0 (aka the user isn't scrolling)? I suppose its nice to always have the extra rows rendered at all times and to avoid changing this overscan value even more
|
Merging for testing. |
|
## API Changes
unknown top level export { type: 'any' } @react-aria/utilsuseDescription-isMac {
- returnVal: undefined
-}
+isMac-isIPhone {
- returnVal: undefined
-}
+isIPhone-isIPad {
- returnVal: undefined
-}
+isIPad-isIOS {
- returnVal: undefined
-}
+isIOS-isAppleDevice {
- returnVal: undefined
-}
+isAppleDevice-isWebKit {
- returnVal: undefined
-}
+isWebKit-isChrome {
- returnVal: undefined
-}
+isChrome-isAndroid {
- returnVal: undefined
-}
+isAndroid-isFirefox {
- returnVal: undefined
-}
+isFirefox useEvent<K extends keyof GlobalEventHandlersEventMap> {
ref: RefObject<EventTarget>
- event: K
+ event: K | (string & {
+
+})
handler?: (Document, GlobalEventHandlersEventMap[K]) => any
options?: boolean | AddEventListenerOptions
returnVal: undefined
}@react-aria/virtualizerVirtualizer-Virtualizer<T extends {}, V extends ReactNode> {
- autoFocus?: boolean
- children: (string, {}) => ReactNode
- collection: Collection<{}>
- focusedKey?: Key
- isLoading?: boolean
- layout: Layout<{}>
- onLoadMore?: () => void
- renderWrapper?: (ReusableView<{}, ReactNode> | null, ReusableView<{}, ReactNode>, Array<ReusableView<{}, ReactNode>>, (Array<ReusableView<{}, ReactNode>>) => Array<ReactElement>) => ReactElement
- scrollDirection?: 'horizontal' | 'vertical' | 'both'
- scrollToItem?: (Key) => void
- shouldUseVirtualFocus?: boolean
- sizeToFit?: 'width' | 'height'
- transitionDuration?: number
-}
+setScrollLeft-
+Virtualizer<O, T extends {}, V extends ReactNode> {
+ children: (string, {}) => ReactNode
+ collection: Collection<{}>
+ focusedKey?: Key
+ isLoading?: boolean
+ layout: Layout<{}, O>
+ layoutOptions?: O
+ onLoadMore?: () => void
+ renderWrapper?: (ReusableView<{}, ReactNode> | null, ReusableView<{}, ReactNode>, Array<ReusableView<{}, ReactNode>>, (Array<ReusableView<{}, ReactNode>>) => Array<ReactElement>) => ReactElement
+ scrollDirection?: 'horizontal' | 'vertical' | 'both'
+ sizeToFit?: 'width' | 'height'
+}@react-spectrum/cardGalleryLayout GridLayout<T> {
buildChild: (Node<T>, number, number) => LayoutInfo
buildCollection: () => void
cardOrientation: Orientation
constructor: (GridLayoutOptions) => void
getIndexAtPoint: (any, any, any) => void
getKeyAbove: (Key) => void
getKeyBelow: (Key) => void
- getVisibleLayoutInfos: (any) => void
itemPadding: number
layoutType: any
}GridLayout WaterfallLayout<T> {
- buildCollection: (InvalidationContext<Node<T>, unknown>) => void
+ buildCollection: (InvalidationContext) => void
constructor: (WaterfallLayoutOptions) => void
getClosestLeft: (Key) => void
getClosestRight: (Key) => void
getKeyLeftOf: (Key) => void
getNextColumnIndex: (any) => void
layoutType: any
updateItemSize: (Key, Size) => void
}
@react-spectrum/listboxListBox useListBoxLayout<T> {
state: ListState<T>
- isLoading: boolean
returnVal: undefined
}useListBoxLayout ListBoxBase<T> {
autoFocus?: boolean | FocusStrategy
disallowEmptySelection?: boolean
domProps?: HTMLAttributes<HTMLElement>
focusOnPointerEnter?: boolean
isLoading?: boolean
layout: ListLayout<T>
onLoadMore?: () => void
onScroll?: () => void
renderEmptyState?: () => ReactNode
shouldFocusWrap?: boolean
shouldSelectOnPressUp?: boolean
shouldUseVirtualFocus?: boolean
+ showLoadingSpinner?: boolean
state: ListState<T>
- transitionDuration?: number
}@react-stately/layoutListLayout ListLayout<T> {
allowDisabledKeyFocus: boolean
buildChild: (Node<T>, number, number) => LayoutNode
buildCollection: () => Array<LayoutNode>
buildHeader: (Node<T>, number, number) => LayoutNode
buildItem: (Node<T>, number, number) => LayoutNode
buildNode: (Node<T>, number, number) => LayoutNode
buildSection: (Node<T>, number, number) => LayoutNode
collection: Collection<Node<T>>
constructor: (ListLayoutOptions<T>) => void
disabledKeys: Set<Key>
+ ensureLayoutInfo: (Key) => void
getContentSize: () => void
getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
- getFinalLayoutInfo: (LayoutInfo) => void
getFirstKey: () => Key | null
- getInitialLayoutInfo: (LayoutInfo) => void
getKeyAbove: (Key) => Key | null
getKeyBelow: (Key) => Key | null
getKeyForSearch: (string, Key) => Key | null
getKeyPageAbove: (Key) => Key | null
getKeyPageBelow: (Key) => Key | null
getLastKey: () => Key | null
getLayoutInfo: (Key) => void
getVisibleLayoutInfos: (Rect) => void
isLoading: boolean
isValid: (Node<T>, number) => void
isVisible: (LayoutNode, Rect) => void
+ layoutIfNeeded: (Rect) => void
updateItemSize: (Key, Size) => void
updateLayoutNode: (Key, LayoutInfo, LayoutInfo) => void
- validate: (InvalidationContext<Node<T>, unknown>) => void
+ validate: (InvalidationContext<ListLayoutProps>) => void
}TableLayout TableLayout<T> {
addVisibleLayoutInfos: (Array<LayoutInfo>, LayoutNode, Rect) => void
binarySearch: (Array<LayoutNode>, Point, 'x' | 'y') => void
buildBody: (number) => LayoutNode
buildCell: (GridNode<T>, number, number) => LayoutNode
buildCollection: () => Array<LayoutNode>
buildColumn: (GridNode<T>, number, number) => LayoutNode
buildHeader: () => LayoutNode
buildHeaderRow: (GridNode<T>, number, number) => LayoutNode
buildNode: (GridNode<T>, number, number) => LayoutNode
buildPersistedIndices: () => void
buildRow: (GridNode<T>, number, number) => LayoutNode
collection: TableCollection<T>
columnLayout: TableColumnLayout<T>
columnWidths: Map<Key, number>
constructor: (TableLayoutOptions<T>) => void
controlledColumns: Map<Key, GridNode<unknown>>
endResize: () => void
getColumnMaxWidth: (Key) => number
getColumnMinWidth: (Key) => number
getColumnWidth: (Key) => number
getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
getEstimatedHeight: (GridNode<T>, number, number, number) => void
- getFinalLayoutInfo: (LayoutInfo) => void
- getInitialLayoutInfo: (LayoutInfo) => void
getRenderedColumnWidth: (GridNode<T>) => void
getResizerPosition: () => Key
getVisibleLayoutInfos: (Rect) => void
isLoading: any
lastPersistedKeys: Set<Key>
persistedIndices: Map<Key, Array<number>>
resizingColumn: Key | null
setChildHeights: (Array<LayoutNode>, number) => void
startResize: (Key) => void
stickyColumnIndices: Array<number>
uncontrolledColumns: Map<Key, GridNode<unknown>>
uncontrolledWidths: Map<Key, ColumnSize>
updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
wasLoading: any
}
@react-stately/virtualizerInvalidationContext-InvalidationContext<T extends {}, V> {
- afterAnimation: () => void
- afterLayout: () => void
- animated?: boolean
- beforeLayout: () => void
- contentChanged?: boolean
- offsetChanged?: boolean
- sizeChanged?: boolean
- transaction?: Transaction<{}, V>
-}
+VirtualizerState VirtualizerState<T extends {}, V, W> {
contentSize: Size
endScrolling: () => void
- isAnimating: boolean
isScrolling: boolean
setVisibleRect: (Rect) => void
startScrolling: () => void
virtualizer: Virtualizer<{}, V, W>
}
it changed:
Layout-Layout<T extends {}> {
- getContentSize: () => Size
- getFinalLayoutInfo: (LayoutInfo) => LayoutInfo
- getInitialLayoutInfo: (LayoutInfo) => LayoutInfo
- getLayoutInfo: (Key) => LayoutInfo
- getVisibleLayoutInfos: (Rect) => Array<LayoutInfo>
- shouldInvalidate: (Rect, Rect) => boolean
- validate: (InvalidationContext<{}, any>) => void
- virtualizer: Virtualizer<{}, any, any>
-}
+ReusableViewchanged by:
ReusableView<T extends {}, V> {
+ children: Set<ReusableView<{}, V>>
constructor: (Virtualizer<{}, V, unknown>) => void
content: {}
+ getReusableView: (string) => void
key: Key
layoutInfo: LayoutInfo | null
+ parent: ReusableView<{}, V> | null
prepareForReuse: () => void
rendered: V
+ reusableViews: Map<string, Array<ReusableView<{}, V>>>
+ reuseChild: (ReusableView<{}, V>) => void
viewType: string
virtualizer: Virtualizer<{}, V, unknown>
}it changed:
useVirtualizerStatechanged by:
-useVirtualizerState<T extends {}, V, W> {
- opts: VirtualizerProps<T, V, W>
- returnVal: undefined
-}
+undefined-
+InvalidationContext<O = any> {
+ contentChanged?: boolean
+ itemSizeChanged?: boolean
+ layoutOptions?: O
+ offsetChanged?: boolean
+ sizeChanged?: boolean
+}undefined-
+Layout<O = any, T extends {}> {
+ getContentSize: () => Size
+ getLayoutInfo: (Key) => LayoutInfo
+ getVisibleLayoutInfos: (Rect) => Array<LayoutInfo>
+ shouldInvalidate: (Rect, Rect) => boolean
+ updateItemSize: (Key, Size) => boolean
+ validate: (InvalidationContext<O>) => void
+ virtualizer: Virtualizer<{}, any, any>
+}undefined-
+useVirtualizerState<O = any, T extends {}, V, W> {
+ opts: VirtualizerProps<T, V, W, O>
+ returnVal: undefined
+} |
| @@ -342,7 +343,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions | |||
| scrollRef.current.scrollLeft = scrollPos.current.left; | |||
| } | |||
|
|
|||
| if (!isVirtualized && manager.focusedKey != null) { | |||
| if (manager.focusedKey != null) { | |||
There was a problem hiding this comment.
Was removing !isVirtualized intentional here?
There was a problem hiding this comment.
yeah we centralized the logic between both virtualized and non-virtualized collections so now they are both handled here. was there a concern or just curious?
Fixes #6181, fixes #6074, fixes #5357, closes #5913, closes #1747
This is a significant refactor to
Virtualizerto reduce its complexity, while improving performance and stability.Complexity
The current Virtualizer code originated over 9 years ago, before we even used React. While some of it has been updated, much of the original code survived. It supports a lot of features that we don't currently use, such as scroll anchoring, and has significant complexity for things like transactional animations. Things like scroll into view and focus management were also duplicated both in virtualizer and in other React Aria hooks, creating a tangled web of hard to maintain code.
This PR removes a lot of that complexity, reducing the main Virtualizer.ts from over 1200 lines down to 350. This is accomplished by removing support for animated updates, scroll anchoring, and other features we didn't use. It also removes scroll into view and focus management from Virtualizer, which can now simply use the same code we use for non-virtualized collections. This is possible because of the support for
persistedKeyswe added a while back, which ensures that the focused item is always persisted in the DOM even if it is scrolled out of view. This means that we can use the regular DOM scroll into view and focus management code instead of needing custom virtualizer-specific logic.Animations and scroll anchoring may return in the future, but probably in a different form that is less virtualizer specific.
Performance
This refactor also improves the performance of Virtualizer during scrolling. When it was initially designed, Virtualizer only handled single-level collections (e.g. lists). Over time it gained support for hierarchy, e.g. sections, or cells within rows. However, the data structures in the core virtualizer still operated on flattened collections. When reusing DOM nodes, this meant that a node representing a cell that was removed from view might be reused to render a cell in a different row. This "re-parenting" caused React to unmount and remount elements much more frequently than necessary, resulting in performance issues. We now maintain separate reuse queues per parent element, so that when a node is reused it is always placed within the same parent. This means that while scrolling, the React children do not change very much – they maintain the same keys and order, and only their contents change – which results in fewer DOM updates, browser layout and style invalidations, etc.
Stability
Since the core of Virtualizer was originally written prior to React, it didn't follow React's rules in some places. For example, updating the collection or layout mutated the virtualizer object during render. This resulted in a side effect being called, triggering an additional render to update the visible views. In addition there were race conditions caused by some updates being triggered inside
requestAnimationFrameto achieve batching. These caused some issues with React's StrictMode.This PR refactors Virtualizer to run layout and updates during React's render lifecycle, rather than triggering additional re-renders as side effects. Updates to the visible rect, collection, layout, persisted keys, etc. all occur as regular React state updates and are passed into Virtualizer which performs some memoized computations and returns an array of visible views.
Virtualizer also now accepts a
layoutOptionsprop, which is passed into Layout objects to trigger updates. For example, we previously mutated layouts to update state likeisLoading, which broke react's rules. Now, this can be passed vialayoutOptions, and virtualizer handles triggering relayout during render when necessary.