Skip to content
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

Fix virtualizer persisted keys with drag and drop #6644

Open
wants to merge 5 commits into
base: move-collections
Choose a base branch
from

Conversation

devongovett
Copy link
Member

Depends on #6640

This fixes the virtualizer persistedKeys during drag and drop so that keyboard navigation works correctly and focus isn't lost. Since we only render one of the drop positions (before or after), we need to normalize the key we persist accordingly so the correct one is in the DOM and able to get focus.

I've also changed the API on Virtualizer and collection renderers in general to accept persistedKeys as a prop rather than a single focusedKey. This enables multiple keys to be persisted and is more extensible for future use cases.

Finally, based on feedback in #6631 (comment), I've renamed the Layout validate method to update to make it easier to understand. This was originally pulled from v2 CollectionView but since we are doing a breaking change we can rename it now.

@rspbot
Copy link

rspbot commented Jun 28, 2024

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that within the 'Drag within table many items' story, if you mouse drag an item from the top of the list to the bottom, then try to keyboard drag an item back up, the list doesn't scroll as you move the drop indicator up the list.

@devongovett
Copy link
Member Author

Hmm yeah that seems to be an issue with persisted keys in TableLayout...

@devongovett
Copy link
Member Author

Ok fixed 3 issues:

  1. TableLayout was using the wrong indices to calculate persisted keys. When we perform an incremental layout, the index should be a count of the items that we actually have built, not all of the nodes in the collection. For example, if we start layout from the middle, there will be items before that we didn't layout. This matches how firstVisibleRow and lastVisibleRow are computed.
  2. In Spectrum, revert back to only persisting either the focused key or the drop target, not both. In RAC we need to persist both to avoid the dragged element going out of view, getting removed from the DOM, and triggering the end of the drag session due to this layout effect. This doesn't happen in Spectrum for some reason. On the flip side, we can't persist both in Spectrum because when dragging ends, there seems to be a race where the focusedKey isn't updated to the dropped item right away and we scroll back to the original focused item from before the drop. I can't reproduce this in RAC. Not quite sure what the difference is...
  3. @LFDanLu noticed that the async loading story was scrolling to the top, because we were placing the loading indicator in the wrong spot.

@rspbot
Copy link

rspbot commented Jun 28, 2024

@rspbot
Copy link

rspbot commented Jun 28, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/virtualizer

Virtualizer

 Virtualizer<O, T extends {}, V extends ReactNode> {
   children: (string, {}) => ReactNode
   collection: Collection<{}>
-  focusedKey?: Key
   isLoading?: boolean
   layout: Layout<{}, O>
   layoutOptions?: O
   onLoadMore?: () => void
+  persistedKeys?: Set<Key> | null
   renderWrapper?: RenderWrapper<{}, ReactNode>
   scrollDirection?: 'horizontal' | 'vertical' | 'both'
   sizeToFit?: 'width' | 'height'
 }

@react-stately/layout

GridLayout

 GridLayout<O = any, T> {
   constructor: (GridLayoutOptions) => void
   getContentSize: () => Size
   getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
   getDropTargetLayoutInfo: (ItemDropTarget) => LayoutInfo
   getLayoutInfo: (Key) => LayoutInfo | null
   getVisibleLayoutInfos: (Rect) => Array<LayoutInfo>
-  validate: () => void
+  update: () => void
 }

ListLayout

 ListLayout<O = any, T> {
   constructor: (ListLayoutOptions) => void
   getContentSize: () => void
   getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
   getDropTargetLayoutInfo: (ItemDropTarget) => LayoutInfo
   getLayoutInfo: (Key) => void
   getVisibleLayoutInfos: (Rect) => void
+  update: (InvalidationContext<O>) => void
   updateItemSize: (Key, Size) => void
-  validate: (InvalidationContext<O>) => void
 }

TableLayout

 TableLayout<O extends TableLayoutProps = TableLayoutProps, T> {
   constructor: (ListLayoutOptions) => void
   getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
   getDropTargetLayoutInfo: (ItemDropTarget) => LayoutInfo
   getVisibleLayoutInfos: (Rect) => void
-  validate: (InvalidationContext<TableLayoutProps>) => void
+  update: (InvalidationContext<TableLayoutProps>) => void
 }

@react-stately/virtualizer

Layout

 Layout<O = any, T extends {}> {
   getContentSize: () => Size
   getDropTargetLayoutInfo: (ItemDropTarget) => LayoutInfo
   getItemRect: (Key) => Rect
   getLayoutInfo: (Key) => LayoutInfo | null
   getVisibleLayoutInfos: (Rect) => Array<LayoutInfo>
   getVisibleRect: () => Rect
   shouldInvalidate: (Rect, Rect) => boolean
+  update: (InvalidationContext<O>) => void
   updateItemSize: (Key, Size) => boolean
-  validate: (InvalidationContext<O>) => void
   virtualizer: Virtualizer<{}, any>
 }

@react-aria/collections

CollectionBuilder

-
+CollectionBuilder<C extends BaseCollection<{}>> {
+  children: (BaseCollection<{}>) => ReactNode
+  content: ReactNode
+  createCollection?: () => BaseCollection<{}>
+}

Collection

+Collection<T extends {}> {
 
+}

createLeafComponent

-
+createLeafComponent<E extends Element, P extends {}> {
+  type: string
+  render: (P, ForwardedRef<E>, any) => ReactNode
+  returnVal: undefined
+}

createBranchComponent

-
+createBranchComponent<E extends Element, P extends {
+    children?: any
+}, T extends {}> {
+  type: string
+  render: (P, ForwardedRef<E>, Node<T>) => ReactNode
+  useChildren: (P) => ReactNode
+  returnVal: undefined
+}

createHideableComponent

-
+createHideableComponent<P = {
+  
+}, T> {
+  fn: (P, React.Ref<T>) => ReactNode | null
+  returnVal: undefined
+}

useIsHidden

-
+useIsHidden {
+  returnVal: undefined
+}

useCachedChildren

changed by:

  • CachedChildrenOptions
-
+useCachedChildren<T extends {}> {
+  props: CachedChildrenOptions<T>
+  returnVal: undefined
+}

BaseCollection

changed by:

  • NodeValue
  • NodeValue
-
+BaseCollection<T> {
+  addNode: (NodeValue<T>) => void
+  at: () => Node<T>
+  clone: () => this
+  commit: (Key | null, Key | null, any) => void
+  getChildren: (Key) => Iterable<Node<T>>
+  getFirstKey: () => void
+  getItem: (Key) => Node<T> | null
+  getKeyAfter: (Key) => void
+  getKeyBefore: (Key) => void
+  getKeys: () => void
+  getLastKey: () => void
+  removeNode: (Key) => void
+  size: any
+  undefined: () => void
+}

NodeValue

changed by:

  • NodeValue
-
+NodeValue<T> {
+  aria-label?: string
+  childNodes: Iterable<Node<T>>
+  clone: () => NodeValue<T>
+  constructor: (string, Key) => void
+  firstChildKey: Key | null
+  hasChildNodes: boolean
+  index: number
+  key: Key
+  lastChildKey: Key | null
+  level: number
+  nextKey: Key | null
+  parentKey: Key | null
+  prevKey: Key | null
+  props: any
+  render?: (Node<any>) => ReactElement
+  rendered: ReactNode
+  textValue: string
+  type: string
+  value: T | null
+}

it changed:

  • BaseCollection
  • NodeValue

CollectionBuilderProps

-
+CollectionBuilderProps<C extends BaseCollection<{}>> {
+  children: (BaseCollection<{}>) => ReactNode
+  content: ReactNode
+  createCollection?: () => BaseCollection<{}>
+}

CollectionProps

+CollectionProps<T> {
 
+}

CachedChildrenOptions

-
+CachedChildrenOptions<T> {
+  addIdAndValue?: boolean
+  children?: ReactNode | (T) => ReactNode
+  dependencies?: Array<any>
+  idScope?: Key
+  items?: Iterable<T>
+}

it changed:

  • useCachedChildren

@devongovett
Copy link
Member Author

Brain dump before break:

Actually I did manage to reproduce (2) in RSP as well, so we should persist the drag target as well as the drop target. It only occurs when the drag target element is removed from the dom and not reused by another view, most commonly with variable row heights.

The other issue with scrolling to the top on drop also only seems to occur in the non-util handlers stories, I think because the onDrop is async there and we refocus in DragManager's end method prior to the items being moved therefore scrolling to the wrong position. If you delay that by a micro task it works, but this breaks a lot of tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants