Refactor virtualizer layouts to remove Spectrum-specific logic and improve backward compatibility #6613
Conversation
accessibility issue seems specific to Firefox VoiceOver
| rows = getAllByRole('row'); | ||
| expect(rows).toHaveLength(8); | ||
| expect(rows.map(r => r.textContent)).toEqual(['FooBar', 'Foo 7Bar 7', 'Foo 8Bar 8', 'Foo 9Bar 9', 'Foo 10Bar 10', 'Foo 11Bar 11', 'Foo 12Bar 12', 'Foo 13Bar 13']); | ||
| expect(rows.map(r => r.textContent)).toEqual(['FooBar', 'Foo 8Bar 8', 'Foo 9Bar 9', 'Foo 10Bar 10', 'Foo 11Bar 11', 'Foo 12Bar 12', 'Foo 13Bar 13', 'Foo 14Bar 14']); |
There was a problem hiding this comment.
RAC table doesn't add + 1 for bottom row borders anymore as Spectrum does.
|
Based on your description, i guess #6611 (comment) is unneeded? |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Did a quick sweep of ListView and TableView as well as RAC virtualized Table and List, looks good, no crashes or anything dire
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
LGTM, chromatic ran clean and the virtualized stories for RAC/RSP seem to work as expected. Didn't find any other Spectrum specific logic other than maybe the +/- 10 thresholds for DnD in getDropTargetFromPoint that a user might want to be able to customize/specify but that one doesn't seem like a huge deal unless a user has much larger or smaller rows than our own I guess.
If a non-RSP user was previously relying on the Spectrum specific calculations in the layouts before, would the recommendation be that they essentially copy TableViewLayout/ListBoxLayout/etc aka extend from Table/ListLayout and add the same logic? I don't know if anyone was relying on those tbh so maybe it is fine
|
Yeah looks like the nubbin isn't clipped anymore on this PR. |
|
## API Changes
unknown top level export { type: 'any' } @react-spectrum/listboxuseListBoxLayout ListBoxBase<T> {
autoFocus?: boolean | FocusStrategy
disallowEmptySelection?: boolean
domProps?: HTMLAttributes<HTMLElement>
focusOnPointerEnter?: boolean
isLoading?: boolean
- layout: ListLayout<T>
+ layout: ListBoxLayout<T>
onLoadMore?: () => void
onScroll?: () => void
renderEmptyState?: () => ReactNode
shouldFocusWrap?: boolean
shouldUseVirtualFocus?: boolean
showLoadingSpinner?: boolean
state: ListState<T>
}
@react-stately/layoutListLayoutOptions ListLayoutOptions {
- L: undefined
+ estimatedHeadingHeight?: number
+ estimatedRowHeight?: number
+ headingHeight?: number
+ rowHeight?: number
}it changed:
ListLayoutProps-ListLayoutProps {
- isLoading?: boolean
-}
+it changed:
LayoutNodechanged by:
LayoutNode {
children?: Array<LayoutNode>
- header?: LayoutInfo
index?: number
layoutInfo: LayoutInfo
node?: Node<unknown>
validRect: Rect
it changed:
TableLayoutOptions-TableLayoutOptions<T> {
- scrollContainer?: 'table' | 'body'
-}
+it changed:
ListLayoutchanged by:
-ListLayout<T> {
- constructor: (ListLayoutOptions<T>) => void
- getContentSize: () => void
- getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
- getLayoutInfo: (Key) => void
- getVisibleLayoutInfos: (Rect) => void
- updateItemSize: (Key, Size) => void
- validate: (InvalidationContext<ListLayoutProps>) => void
-}
+TableLayoutchanged by:
-TableLayout<T> {
- collection: TableCollection<T>
- columnWidths: Map<Key, number>
- constructor: (TableLayoutOptions<T>) => void
- getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
- getVisibleLayoutInfos: (Rect) => void
- isLoading: any
- lastCollection: TableCollection<T>
- lastPersistedKeys: Set<Key>
- persistedIndices: Map<Key, Array<number>>
- scrollContainer: 'table' | 'body'
- stickyColumnIndices: Array<number>
- validate: (InvalidationContext<TableLayoutProps>) => void
-}
+undefined-
+ListLayout<O = any, T> {
+ constructor: (ListLayoutOptions) => void
+ getContentSize: () => void
+ getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
+ getLayoutInfo: (Key) => void
+ getVisibleLayoutInfos: (Rect) => void
+ updateItemSize: (Key, Size) => void
+ validate: (InvalidationContext<O>) => void
+}undefined-
+TableLayout<O extends TableLayoutProps = TableLayoutProps, T> {
+ constructor: (ListLayoutOptions) => void
+ getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
+ getVisibleLayoutInfos: (Rect) => void
+ validate: (InvalidationContext<TableLayoutProps>) => void
+}@react-stately/virtualizerLayout Layout<O = any, T extends {}> {
getContentSize: () => Size
getItemRect: (Key) => Rect
- getLayoutInfo: (Key) => LayoutInfo
+ getLayoutInfo: (Key) => LayoutInfo | null
getVisibleLayoutInfos: (Rect) => Array<LayoutInfo>
getVisibleRect: () => Rect
shouldInvalidate: (Rect, Rect) => boolean
updateItemSize: (Key, Size) => boolean
virtualizer: Virtualizer<{}, any, any>
}
|
This removes Spectrum-specific logic from the layouts in
@react-stately/layout, and moves that into subclasses within each Spectrum component. These include:It also adds backward compatibility for the
layoutprop ofuseTablewhich I previously removed. This is done by shimming theLayoutDelegateinterface using a copied interface that matches the old Virtualizer Layout class so we don't need a dependency on it.I also reverted the change to use
overflow: clipbecause it resulted in incorrect height measurements (caught by Chromatic). This was originally because I thought it introduced additional generic containers in Firefox accessibility tree. I later realized that these only seem to affect VoiceOver which is not a supported combination. Firefox NVDA seems fine.Finally I removed the workaround added 2 years ago for sticky positioning crashing in Chrome 105 (#3504). I think it's been fixed for long enough that we don't need this anymore.