Skip to content

feat(RAC): Tree drag and drop #7692

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 48 commits into from
Apr 21, 2025
Merged

feat(RAC): Tree drag and drop #7692

merged 48 commits into from
Apr 21, 2025

Conversation

rob-clayburn
Copy link
Contributor

I'm looking to get some feedback on this PR to add drag and drop to the Tree.

I am really really new to this codebase, and there are probably a ton of things I've not understood and have messed up

I've mainly just copied the logic from the listbox and applied it to the tree component.

There's a storybook story which I've added at http://localhost:9003/?path=/story/treeview--tree-example-dynamic-drag-n-drop&providerSwitcher-express=false&strict=true

Some things that would be great to get some feedback/help on are:

  • I'm unclear as to whether the dragAndDropHooks should be created by the user each time (e.g. in the story). Some things i can see being useful, but it feels like the reorder function shouldn't have to be implemented here. Is there a better place to put this?

  • Re-ordering works, but at some point it stops working. I can't seem to find a repeatable pattern of drag / drops to consistently replicate this though.

  • In storybook the hot reload gives an error:
    image

  • Although I've added the keyboard code - I've not yet tested that - was hoping to get the other issues ironed out first.

  • The way I've implemented the drop insert indicator causes the tree items to move which in turn can cause the drop to switch from before to after. The droppable listbox story renders items with a gap in between but I'm not sure if that is what is wanted in a tree view.

@reidbarber
Copy link
Member

Thanks for the PR! I'm going to be taking a closer look when I have some time soon, just wanted to leave an update.

@rob-clayburn
Copy link
Contributor Author

thanks, have you managed to carve out some time to look at this?

@reidbarber
Copy link
Member

Sorry for the delay, we've been busy with the latest release! I just pushed a few small updates and a story that has DnD working in RAC Tree. I still need to debug RSP TreeView and fix some merge conflicts. The state management in the story should get simplified once we merge #7854.

@LFDanLu LFDanLu moved this from 📋 Waiting for Sprint to 🏗 In Progress in RSP Component Milestones Mar 11, 2025
@reidbarber
Copy link
Member

Pushed a few changes so it should be in a decent state to test out. Some things to note:

  • We're going to scope this to Tree in React Aria Components for now (adding to React Spectrum TreeView can be a follow-up). I made a new story for this and removed the old one.
  • We'll probably need a way expand/collapse items while keyboard navigating during a drop.

@@ -234,10 +235,29 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
}
}

function getDescendantKeys(node?: TreeNode<T>): Key[] {
Copy link
Member

Choose a reason for hiding this comment

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

any reason to have node be optional? can we make it required?

if (!node) {
return descendantKeys;
}
function recurse(currentNode: TreeNode<T>) {
Copy link
Member

Choose a reason for hiding this comment

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

does the traversal order matter? preorder, in order, postorder

should descendantKeys be a Set if it doesn't matter?

disabledKeys: state.selectionManager.disabledKeys,
disabledBehavior: state.selectionManager.disabledBehavior,
layout: 'stack',
direction,
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that Table in RAC doesn't specify direction is that a bug? or what does this affect?

@reidbarber reidbarber moved this from 🏗 In Progress to 👀 In Review in RSP Component Milestones Apr 8, 2025
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Approving for further testing. Issues can be investigated separately in a followup PR (I'll need to dive into the useTreeData for one of the aforementioned cases, unsure if it is a drop hooks issue or if the tree data is getting malformed)

Copy link
Member

Choose a reason for hiding this comment

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

I'll have to track down what exactly is happening but I noticed the following bug:

  1. Using http://localhost:9003/?path=/story/react-aria-components--tree-with-drag-and-drop&args=selectionMode:multiple&providerSwitcher-express=false&strict=true, expand both top level folders and and drag "Project 1" above "Projects"
  2. Drag "Project 1" back somewhere under "Projects" again (e.g. between "Project 3" and "Project 4")
    Note that "Project 1" disappears at this point

Copy link
Member

Choose a reason for hiding this comment

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

Another odd case that I discovered:

  1. Drag "Reports 1" into "Projects 2"
  2. Drag "Project 2" down below "Project 3"
  3. Drag "Project 2" down into "Reports" (Aka between "Reports" and "Reports 1" so that it becomes a child of "Reports"

Project 2 actually becomes a root level folder after this

Copy link
Member

Choose a reason for hiding this comment

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

It might be that the drop operation thinks the drop position is "After Reports" which technically isn't wrong but it should really be "Before Reports 1" or have a greater degree of granularity (aka "between Reports and Reports 1"). This actually works with keyboard DnD (and has the proper "insert between Reports and Reports 1" drop target) but for some reason is targeting the wrong drop insertion target I think for mouse DnD

Comment on lines +263 to +272
onDropActivate: (e) => {
// Expand collapsed item when dragging over
if (e.target.type === 'item') {
let key = e.target.key;
let item = state.collection.getItem(key);
if (item && item.hasChildNodes && expandedKeys !== 'all' && !expandedKeys.has(key)) {
state.toggleKey(key);
}
}
},
Copy link
Member

Choose a reason for hiding this comment

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

I can't seem to get this to trigger, not sure if regression or known?

Comment on lines +274 to +277
if (e.key === 'ArrowRight' || e.key === 'ArrowLeft') {
// Toggle expansion when drop target is after expandable item
let target = dropState?.target;
if (target && target.type === 'item') {
Copy link
Member

Choose a reason for hiding this comment

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

We may want to update this behavior in the future and make it possible for the user to keyboard navigate onto the row when drag and dropping and then accept "ArrowRight/ArrowLeft". Feels a bit confusing at the moment

let dragButtonRef = useRef<HTMLButtonElement>(null);
useEffect(() => {
if (dragState && !dragButtonRef.current && process.env.NODE_ENV !== 'production') {
console.warn('Draggable items in a Table must contain a <Button slot="drag"> element so that keyboard and screen reader users can drag them.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.warn('Draggable items in a Table must contain a <Button slot="drag"> element so that keyboard and screen reader users can drag them.');
console.warn('Draggable items in a Tree must contain a <Button slot="drag"> element so that keyboard and screen reader users can drag them.');

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

let's get this in for testing

@@ -741,6 +743,7 @@ export function useDroppableCollection(props: DroppableCollectionOptions, state:
break;
}
}
localState.props.onKeyDown?.(e as any);
Copy link
Member

Choose a reason for hiding this comment

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

e won't be a keyboard event here. What was this intended for?

@@ -36,7 +36,6 @@ export interface DropOptions {
/**
* Handler that is called after a valid drag is held over the drop target for a period of time.
* This typically opens the item so that the user can drop within it.
* @private
Copy link
Member

Choose a reason for hiding this comment

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

Did we find a way to make this accessible on keyboard/screen reader? That's why it was private before.

* An optional keyboard delegate implementation for type to select,
* to override the default.
*/
keyboardDelegate?: KeyboardDelegate
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to expose this as a public prop? Doesn't look like we do in other collection components e.g. GridList

@reidbarber reidbarber added this pull request to the merge queue Apr 21, 2025
Merged via the queue into adobe:main with commit e4ec6b4 Apr 21, 2025
30 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in RSP Component Milestones Apr 21, 2025
@marchingband
Copy link

At this stage, is this feature available in nightly builds via NPM?
I'd be happy to help test...
Is there a way I can get an alert when it makes it into "latest" ?
thanks so much.

@snowystinger
Copy link
Member

Yes, it's on main, so it's now in nightly. There are some follow ups from our own testing here fix: refactor useTreeData
It should land in our next release. We don't have an RSS feed yet, but you could use one of the options outlined here https://www.quora.com/Is-there-a-way-to-subscribe-to-sites-that-dont-have-RSS-feed-functionality to watch our Rleases page https://react-spectrum.adobe.com/releases/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants