fix: DnD for moving child before parent with single root item

[rothsandro]

Closes

See discussion #9796

If a tree contains a single root item with a child, the child can't be moved before the parent element via drag & drop, the drag indicator doesn't appear and dropping it calls onMove with an invalid id.

I made a fix by checking the node type (as suggested by snowystinger in the discussion), because the bug was caused by a content node which resulted in the invalid id. While checking the type likely makes sense (it's also done further up in the code for other cases), I'm not really confident with the change. I'm not sure what use case the whole Handle converting "after" to "before next" block handles and if the change breaks anything else. The if statement also doesn't check whether the target actually has after as drop position, so maybe this would be another way to fix it.

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

1. Start Storybook
2. Open the Tree With Drag And Drop Story from React Aria Components
3. Adjust the Story to only have a single root item (with children)
4. Move a child before its parent item via drag & drop

🧢 Your Project:

[snowystinger]

Thanks for the PR and especially the test, I think we may have a bug in our story. I can see the drop indicator now, but if I drop it, I get this error

RangeError: Invalid array length
    at Array.push (<anonymous>)
    at useCachedChildren.ts:63:13
    at updateMemo (react-dom-client.development.js:8795:19)
    at Object.useMemo (react-dom-client.development.js:26484:18)
    at exports.useMemo (react.development.js:1251:34)
    at useCachedChildren (useCachedChildren.ts:39:16)
    at useCollectionRender (Collection.tsx:167:26)
    at CollectionRoot (Collection.tsx:155:12)
    at Object.react_stack_bottom_frame (react-dom-client.development.js:25904:20)
    at renderWithHooks (react-dom-client.development.js:7662:22)
    at updateFunctionComponent (react-dom-client.development.js:10166:19)
    at beginWork (react-dom-client.development.js:11778:18)
    at runWithFiberInDEV (react-dom-client.development.js:871:30)
    at performUnitOfWork (react-dom-client.development.js:17641:22)
    at workLoopSync (react-dom-client.development.js:17469:41)
    at renderRootSync (react-dom-client.development.js:17450:11)
    at performWorkOnRoot (react-dom-client.development.js:16583:35)
    at performSyncWorkOnRoot (react-dom-client.development.js:18972:7)
    at flushSyncWorkAcrossRoots_impl (react-dom-client.development.js:18814:21)
    at processRootScheduleInMicrotask (react-dom-client.development.js:18853:9)
    at react-dom-client.development.js:18991:13

[rothsandro]

Hm, I can't reproduce this error, I properly get the "moving [project-1] before projects" log. Are you using the same Story and updated rows?

http://localhost:9003/?path=/story/react-aria-components-tree--tree-with-drag-and-drop

diff --git a/packages/react-aria-components/stories/Tree.stories.tsx b/packages/react-aria-components/stories/Tree.stories.tsx
index cf04d5eed..75036d948 100644
--- a/packages/react-aria-components/stories/Tree.stories.tsx
+++ b/packages/react-aria-components/stories/Tree.stories.tsx
@@ -399,31 +399,7 @@ export const TreeExampleStaticNoActions: StoryObj<typeof TreeExampleStaticNoActi
 
 let rows = [
   {id: 'projects', name: 'Projects', childItems: [
-    {id: 'project-1', name: 'Project 1'},
-    {id: 'project-2', name: 'Project 2', childItems: [
-      {id: 'project-2A', name: 'Project 2A'},
-      {id: 'project-2B', name: 'Project 2B'},
-      {id: 'project-2C', name: 'Project 2C'}
-    ]},
-    {id: 'project-3', name: 'Project 3'},
-    {id: 'project-4', name: 'Project 4'},
-    {id: 'project-5', name: 'Project 5', childItems: [
-      {id: 'project-5A', name: 'Project 5A'},
-      {id: 'project-5B', name: 'Project 5B'},
-      {id: 'project-5C', name: 'Project 5C'}
-    ]}
-  ]},
-  {id: 'reports', name: 'Reports', childItems: [
-    {id: 'reports-1', name: 'Reports 1', childItems: [
-      {id: 'reports-1A', name: 'Reports 1A', childItems: [
-        {id: 'reports-1AB', name: 'Reports 1AB', childItems: [
-          {id: 'reports-1ABC', name: 'Reports 1ABC'}
-        ]}
-      ]},
-      {id: 'reports-1B', name: 'Reports 1B'},
-      {id: 'reports-1C', name: 'Reports 1C'}
-    ]},
-    {id: 'reports-2', name: 'Reports 2'}
+    {id: 'project-1', name: 'Project 1'}
   ]}
 ];

[snowystinger]

Ah, I did not adjust the story. I went to "Tree With Drag And Drop", dragged one of the top level items over to the second tree, then opened it, and dragged its child before it.

[rothsandro]

I can reproduce it but doesn't seem to be related to the other problem / fix. It's also reproducible when moving the child item after the root item (which didn't have the missing drop indicator problem).

It seems somehow related / caused by the buildTree function which mutates the originalMap it receives. Passing new Map(originalMap) solves the problem but this definitely needs further analyzes why this exactly is happening and how to fix it (the fix should probably be within the buildTree function but cloning the map there resulted in other problems). I would suggest to create a new bug issue for this if you agree.

react-spectrum/packages/react-stately/src/data/useTreeData.ts

Lines 262 to 267 in 1bd82a0

	insert(parentKey: Key | null, index: number, ...values: T[]) {
	setItems(({items, nodeMap: originalMap}) => {
	let {items: newNodes, nodeMap: newMap} = buildTree(values, originalMap, parentKey);
	// If parentKey is null, insert into the root.
	if (parentKey == null) {

Edit: It also only happens in strict mode.

[snowystinger]

Ah, that's great! it'll be fixed by this PR then fix(useTreeData): insert items into a child node with StrictMode Though what were the other issues you found when you passed in a new Map? So we can address those issues here.

Thanks for looking into that.

[rothsandro]

There was only an issue when creating a new map inside buildTree. Looking again, I realized that this is because of the recursive call inside buildTree (which then shouldn't create a new copy). Creating a new map outside and passing it to buildTree as the linked PR does it works fine.