Skip to content

fix: Tree Dnd updates #8229

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

Merged
merged 10 commits into from
May 15, 2025
Merged

fix: Tree Dnd updates #8229

merged 10 commits into from
May 15, 2025

Conversation

devongovett
Copy link
Member

  • Support onMove event, which allows moving between parents. onReorder now only allows reordering within the same parent.
  • Fix accessibility of drop indicators (they should have role=row/gridcell instead of role=option)
  • Fix dropping between the parent item and its first child
  • Made ArrowLeft/ArrowRight to expand while dragging work while focused on the item rather than the drop indicator after it. Also fixed RTL
  • Made the chevron visible to screen readers so mobile users can expand/collapse an item while dragging. For now, this is not tabbable (since keyboard users can use arrow keys to expand/collapse). This will be needed for other components, but needs further work on propagating the focus event so the focus ring becomes visible.

@rspbot
Copy link

rspbot commented May 13, 2025

Comment on lines +278 to +285
if (nextKey != null) {
target = {
type: 'item',
key: nextKey,
dropPosition: 'before'
};
}
}
Copy link
Member

Choose a reason for hiding this comment

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

could we do something like:

              let beforeTarget =  {
                type: 'item',
                key: nextKey,
                dropPosition: 'before'
              };

              if (nextKey != null && isValidDropTarget(beforeTarget) && isValidDropTarget(target)) {
                return beforeTarget;
              }

This would allow us to drop after the last item in your current parent scope even if the next item after that is a item outside your parent scope. Maybe doesn't even need the isValidDropTarget(target) check.

This doesn't fix the onMove case where you can't target the after position for the same case above, but maybe the x,y can be utilized here to favor one drop position over the other (aka if you are closer to dropping outside the left edge of the parent). The tolerance is a bit tight though

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I wonder if we could use the x coordinate to determine if a root drop should happen instead of placing it after the last item of the tree. As is is now, a user can't move something immediately after "Reports" without collapsing it, something they can't afford to do via mouse/touch if they are trying to move a child of "Report" up and out to the root level.

@rspbot
Copy link

rspbot commented May 13, 2025

Comment on lines 323 to 326
if (this.getCurrentActivateButton()?.contains(e.target as Node)) {
this.activate();
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

In Android talkback, when I double tap when focused on the expand chevron while a drag session is active, the drop executes instead of expanding the target row. Not sure why, will need to dig when I can actually connect to a my locally hosted storybook rather than the built one

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, for VO on iPhone it was the opposite.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, it turns out that its due to talkback not moving DOM focus as your move the cursor. Does it still work in iPhone after the changes?

@rspbot
Copy link

rspbot commented May 13, 2025

@rspbot
Copy link

rspbot commented May 14, 2025

@rspbot
Copy link

rspbot commented May 14, 2025

@rspbot
Copy link

rspbot commented May 14, 2025

@rspbot
Copy link

rspbot commented May 14, 2025

@rspbot
Copy link

rspbot commented May 14, 2025

## API Changes

react-aria-components

/react-aria-components:DropIndicator

 DropIndicator {
+  activateButtonRef?: RefObject<FocusableElement | null>
   children?: ReactNode | ((DropIndicatorRenderProps & {
     defaultChildren: ReactNode | undefined
 })) => ReactNode
   className?: string | ((DropIndicatorRenderProps & {
 })) => string
   style?: CSSProperties | ((DropIndicatorRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   target: DropTarget
 }

/react-aria-components:TreeItemRenderProps

 TreeItemRenderProps {
+  allowsDragging?: boolean
   hasChildItems: boolean
   id: Key
   isDisabled: boolean
+  isDragging?: boolean
+  isDropTarget?: boolean
   isExpanded: boolean
   isFocusVisible: boolean
   isFocusVisibleWithin: boolean
   isFocused: boolean
   isPressed: boolean
   isSelected: boolean
   level: number
   selectionBehavior: SelectionBehavior
   selectionMode: SelectionMode
   state: TreeState<unknown>
 }

/react-aria-components:TreeItemContentRenderProps

 TreeItemContentRenderProps {
+  allowsDragging?: boolean
   hasChildItems: boolean
   id: Key
   isDisabled: boolean
+  isDragging?: boolean
+  isDropTarget?: boolean
   isExpanded: boolean
   isFocusVisible: boolean
   isFocusVisibleWithin: boolean
   isFocused: boolean
   isPressed: boolean
   isSelected: boolean
   level: number
   selectionBehavior: SelectionBehavior
   selectionMode: SelectionMode
   state: TreeState<unknown>
 }

/react-aria-components:DragAndDropOptions

 DragAndDropOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   dropTargetDelegate?: DropTargetDelegate
   getAllowedDropOperations?: () => Array<DropOperation>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   getItems?: (Set<Key>) => Array<DragItem> = () => []
   isDisabled?: boolean
   onDragEnd?: (DraggableCollectionEndEvent) => void
   onDragMove?: (DraggableCollectionMoveEvent) => void
   onDragStart?: (DraggableCollectionStartEvent) => void
   onDrop?: (DroppableCollectionDropEvent) => void
+  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
+  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   renderDragPreview?: (Array<DragItem>) => JSX.Element
   renderDropIndicator?: (DropTarget) => JSX.Element
 }

/react-aria-components:DropIndicatorProps

 DropIndicatorProps {
+  activateButtonRef?: RefObject<FocusableElement | null>
   children?: ReactNode | ((DropIndicatorRenderProps & {
     defaultChildren: ReactNode | undefined
 })) => ReactNode
   className?: string | ((DropIndicatorRenderProps & {
 })) => string
   style?: CSSProperties | ((DropIndicatorRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   target: DropTarget
 }

@react-aria/dnd

/@react-aria/dnd:DroppableCollectionOptions

 DroppableCollectionOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   dropTargetDelegate: DropTargetDelegate
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   keyboardDelegate: KeyboardDelegate
   onDrop?: (DroppableCollectionDropEvent) => void
+  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
-  onKeyDown?: KeyboardEvents['onKeyDown']
+  onKeyDown?: (KeyboardEvent) => void
+  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean
 }

/@react-aria/dnd:DroppableItemOptions

 DroppableItemOptions {
+  activateButtonRef?: RefObject<FocusableElement | null>
   target: DropTarget
 }

/@react-aria/dnd:DropIndicatorProps

 DropIndicatorProps {
+  activateButtonRef?: RefObject<FocusableElement | null>
   target: DropTarget
 }

@react-spectrum/dnd

/@react-spectrum/dnd:DragAndDropOptions

 DragAndDropOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   getAllowedDropOperations?: () => Array<DropOperation>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   getItems?: (Set<Key>) => Array<DragItem> = () => []
   onDragEnd?: (DraggableCollectionEndEvent) => void
   onDragMove?: (DraggableCollectionMoveEvent) => void
   onDragStart?: (DraggableCollectionStartEvent) => void
   onDrop?: (DroppableCollectionDropEvent) => void
+  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
+  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   renderPreview?: (Set<Key>, Key) => JSX.Element
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean

@react-stately/dnd

/@react-stately/dnd:DroppableCollectionStateOptions

 DroppableCollectionStateOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   collection: Collection<Node<unknown>>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   isDisabled?: boolean
   onDrop?: (DroppableCollectionDropEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
+  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   selectionManager: MultipleSelectionManager
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean

@devongovett devongovett added this pull request to the merge queue May 15, 2025
Merged via the queue into main with commit 206ebd5 May 15, 2025
30 checks passed
@devongovett devongovett deleted the tree-dnd-fixes branch May 15, 2025 17:17
reidbarber added a commit that referenced this pull request May 15, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 15, 2025
* Revert "feat(RAC): Tree drag and drop (#7692)"

This reverts commit e4ec6b4.

* Revert "fix: Tree Dnd updates (#8229)"

This reverts commit 206ebd5.

* lint

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

Successfully merging this pull request may close these issues.

4 participants