fix(Table): Fix focus behavior after moving items to a different level via keyboard DnD

[chirokas]

Closes

Video.Project.mp4

When a row moves to a different level, it is removed and recreated during the TableCollection rebuild (due to the Fiber child reconciliation algorithm). The previously focused cell is recreated with a new key that no longer matches prevFocusedKey, resulting in incorrect focus behavior.

✅ 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:

🧢 Your Project:

[snowystinger]

We should probably stabilise cell ids (keys) based on the row if possible.

I checked, all the behaviour works if you supply an id to the Cell for the drag and drop button, because then it is stable. So it's only for the case where we generate the id instead

react-spectrum/packages/react-aria/src/collections/Document.ts

Line 336 in 86c8a3b

node = new CollectionNodeClass(id ?? `react-aria-${++this.ownerDocument.nodeId}`);

@LFDanLu it looks like you created this line, I vaguely recall some discussion around this, was there a reason we didn't use the parent key in this?

[chirokas]

@snowystinger In React, the child refCallback is called before the parent refCallback.
When Cell.setProps is called, Row.node hasn’t been instantiated yet. How can I access the Cell’s parent key at this point?

[snowystinger]

Ah, that's a good point, hadn't gone that far yet. Hmmm... will keep thinking

[nwidynski]

@chirokas We can resolve the key earlier in the lifecycle. I remember having explored using the is attribute to forward the key at createElement time, which, iirc, flows top-down instead of bottom-up. The key would be stored on the ElementNode so the parent key would be available by the time we set props on the child. Alternatively we could also just always auto-generate and then override the node key.

[nwidynski]

@chirokas Browser support doesn't matter, since we are rendering into a fake document. React supports the is attribute since v16, which coincides with the compatibility range of react aria. It won't bail out in Webkit environments if that's what you are worried about - that's all left up to userland.

[nwidynski]

@chirokas The comment below the line you linked refers to "is" having been passed to createElement by the time this is reached. This is done via the second argument of the createElement API (see MDN), which was working last time I explored.

Once we extract the key there, we can store it on the element node ourselves, ideally in a nodeId field. Just be aware that the is attribute only supports strings so you will have to serialize numeric keys.

I will see whether I can pull up a small code sample from the exploration once im back at the keys.

Edit: Added pseudo diff of react-aria/collections/Document.ts

BaseNode

+  nodeId: Key;

-  constructor(ownerDocument: Document<T, any>) {
-    this.ownerDocument = ownerDocument;
-  }
+  constructor(key: Key, ownerDocument: Document<T, any>) {
+    this.ownerDocument = ownerDocument;
+    this.nodeId = key; 
+  }

Document

-  nodeId: number;
+  nodeCount: number;

-  constructor(collection: C) {
-    // @ts-ignore
-    super(null);
-    this.collection = collection;
-    this.nextCollection = collection;
-  }
+  constructor(collection: C) {
+    // @ts-ignore
+    super('react-aria', null);
+    this.collection = collection;
+    this.nextCollection = collection;
+  }

-  createElement(type: string): ElementNode<T> {
-    return new ElementNode(type, this);
-  }
+  createElement(type: string, opts: ElementCreationOptions): ElementNode<T> {
+    let key = opts.is != null ? JSON.parse(opts.is) : `react-aria-${++this.nodeCount}`;
+    return new ElementNode(type, key, this);
+  }

ElementNode

-  constructor(type: string, ownerDocument: Document<T, any>) {
-    super(ownerDocument);
-    this.node = null;
-  }
+    constructor(type: string, key: Key, ownerDocument: Document<T, any>) {
+    super(key, ownerDocument);
+    this.node = null;
+  }

-  new CollectionNodeClass(id ?? `react-aria-${++this.ownerDocument.nodeId}`)
+  new CollectionNodeClass(id ?? `${this.parentNode.nodeId}-${this.index}`)

[chirokas]

@nwidynski The approach I took is slightly different let me know what you think

[nwidynski]

@chirokas What was the reason for only doing this client-side? I'm kind of concerned about the change in key strategy after the document has been built during hydration. From the top of my head, I think this could become a problem anytime a ref is re-mounted within the same document, e.g. inside an Activity.

[nwidynski]

LGTM, thanks for the update 🚀

[snowystinger]

I think this is a pretty good solution. Thanks for the help @nwidynski !

I have a feeling @devongovett will have some thoughts as well, specifically around performance. The nodes are meant to be as light as possible and now we're adding a map of attributes for each one. It may make more sense to just make it one known attribute for now so it's less of a memory footprint. If we need to add more down the line, we can expand on it.

[nwidynski]

Suggested change

	if (node.parentNode instanceof ElementNode) {
	if (node.parentNode !== node.ownerDocument) {

Regarding performance, I think this is faster than traversing the prototype chain. Will need type assertions though.

[LFDanLu]

Sorry about the delay. Verified the code path before and after the fix, this change looks reasonable to me. @snowystinger w/ regards to the line you referenced, that was actually part of the original implementation: https://github.com/adobe/react-spectrum/blame/9b48ec5d528adeba1a96592945459726def7ba56/packages/%40react-aria/collections/src/Document.ts#L229, I'm not sure if there was a particular reason not including the parent key in the first place but perhaps @devongovett can speak to that.

[snowystinger]

re-approving after merge conflicts