feat: nested collection support

[nwidynski]

Closes #7277 (comment)

As discussed, this change focuses only on the bare minimum to get nested collections to work.

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

[LFDanLu]

@nwidynski Heya, sorry about the delay, we were pretty busy with getting the release out. As mentioned in #7277 (comment) we are leaning towards supporting inputs in Gridlist first via something like #7360, which you had already noted doesn't solve the typeahead collision problem.

However, instead of making typeahead disable-able via a prop, we tossed around an idea of perhaps just doing some detection in the useTypeselect hook to see if the user was typing in an input and simply early return at that point. Any thoughts on that approach?

[nwidynski]

@LFDanLu No worries about the timeline 👍

I suppose you were exploring that idea to lighten the burden on the developer? If so, I don't see why these necessarily have to exclude each other. Preventing type select only when focus is currently on an input, will still cause issues with nested collections. Was there other reasoning for prohibiting the exposure of disallowTypeAhead?

As mentioned (see comment), this PR focuses only on the essentials of supporting nested collections and can be seen as independent of #7360. Only the Document key and type select are things we have to fix upstream to implement our use-case. Everything else we proposed, including #7360, we can temporarily implement ourselves through a custom virtualizer.

On that note, any feedback from the team on #7376?

[LFDanLu]

There was also a concern about exposing disallowTypeAhead since it may not be clear to a user when/when not to disable it. That being said, at a glance it doesn't appear in the grid spec so maybe not as core of an interaction pattern as previously thought. No feedback on #7376 just yet, I hope to take a look at it soon as soon as I get to a good place with the SearchableMenu/Autocomplete support myself.

[LFDanLu]

One thing I just noticed is that this unscoping doesn't work with the Virtualizer wrapped GridList. It seems like the GridList's collection gets built with one instance of the autogenerated id, but then another render happens where the GridList generates another scope id but the collection itself doesn't get regenerated. This results in the above props.id mismatching with the collection's node key prefix. Poking around to see if I can figure out why that is

EDIT: possible a bug in our useId, it seems to be discarding the user provided defaultId somehow. Not entirely sure why that happens only for the Virtualizer wrapped GridList...

[nwidynski]

Good catch 👍 I just got around to take a proper look and pushed a fix to remote.

The issue wasn't with useId but rather with the drag and drop hooks generating an id without taking the defaultId into account. That also explains why you only observed it with GridList as all other virtualized examples simply didn't have drag and drop implemented 😄

I opted to override the id in mergeProps of the component, but longterm might be wise to think about forwarding the defaultId to the drag and drop hooks somehow...

[LFDanLu]

Ah, nice catch! I completely forgot that the drop hooks generate an id of their own, was so confused how some autogenerated id was making it though. Unfortunately this means I've found even more problematic areas in the code 😅.

1. I think it is a good idea to forward the GridList's id to GridList's call o useDroppableCollection so the weak map id here is consistent. It actually already works at the moment due to mergeProps deduping the ids and causing all other instances of useId to use the last one provided (aka via this call of mergeProps), but it would be best not to rely on this IMO.

2. I noticed that the reordering in the Virtualized Gridlist story is broken because the selectionManager/collection has the prefixed id for the items (which is expected) and will use those prefixed id as the dragged keys when drag starts (see here for how it is pulling them out of the keyMap). These are stored globally and used in useDroppableCollection in things like onReorder/etc, resulting in logic like

   react-spectrum/packages/react-aria-components/stories/GridList.stories.tsx

   Lines 130 to 136 in 4b97149

   	onReorder(e) {
   	if (e.target.dropPosition === 'before') {
   	list.moveBefore(e.target.key, e.keys);
   	} else if (e.target.dropPosition === 'after') {
   	list.moveAfter(e.target.key, e.keys);
   	}
   	},

   no longer working because the list has the user's original non prefixed id but e.keys has the prefixed ids and thus we have a mismatch. If we store the prefix and as part of the collection/SelectionManager via BaseCollection like you mentioned before, we could perhaps strip off the prefix and hopefully fix this

However, I'm now a bit more worried since this class of issue is due to our original implemented assumption that the ids stored in the collection/SelectionManager matches the original ids format provided by the user. This assumption is likely to exist in many other hooks, and I'm quite troubled by the fact that the tests ran clean...

[LFDanLu]

Perhaps we should step back and instead have the collection id scoping only applied to the generated DOM data-key rather than affecting the keys at collection/SelectionManger level.

[nwidynski]

haha, that's exactly what i worked on today 👍

[nwidynski]

Btw, funnily enough your call about useId caused me to unearth a tricky little bug in it.

When a component has 2 ids for some reason (e.g. because of drag & drop hooks), merging these ids in the order id1 -> id2 -> id1 causes the component to infinitely re-render when in strict mode 😄.

Not sure whether it's worth to fix that, but could be untangled by mergeProps perhaps.

[nwidynski]

Not really happy with this, but passing idScope down here will be a mess.

The cleanest option I can think of would be to setup an apply trap for layoutDelegate.getItemRect() inside useGridList() to inject the idScope into its arguments.

[LFDanLu]

what if you apply the collection's idScope to the collection node as its own data-key and look that up via the container ref? That way you can construct the expected node data key here. It might be good to make a util function that generalizes the construction of the nodeKey just so we can make sure we are consistent with the ${idScope}-${manager.focusedKey.toString()} structure.

[nwidynski]

Im a bit worried there might also be assumptions in place for what elements have a data-key attribute, but for now I can only find ListDropTargetDelegate doing that. Can you ask the team?

I guess you would be okay with @react-aria/dnd receiving a @react-aria/selection dependency? Otherwise I'm not sure where to host this util function, since housing it in @react-aria/utils doesn't feel right to me.

[LFDanLu]

sure, I'll ask if anyone one remembers any other areas where we might have assumptions for the application of data-key. Does feel a bit iffy to pull in the @react-aria/selection package into the @react-aria/dnd package just for this util, I'll let others weigh in there. If you'd like you could skip using that function for the dnd package for now, can be followup

[nwidynski]

Ideally I would like to house the getNodeKey util inside @react-aria/collections. I feel like it fits best there and I would like to leverage it for the idScope of useCachedChildren too, since there are discrepancies currently.

I would argue the impact of that to be negligible as the vast majority of users will already have an implicit import of @react-aria/collections when consuming either @react-aria/dnd or @react-aria/selection, especially now that S2 is streamlining to this collection implementation as well.

Let me know what the team thinks of that though 👍

[LFDanLu]

One other place I think you may need to apply this change as well is in this function:

react-spectrum/packages/@react-aria/dnd/src/ListDropTargetDelegate.ts

Lines 102 to 108 in 50c7ada

	let items = [...this.collection].filter(item => item.type === 'item');
	let low = 0;
	let high = items.length;
	while (low < high) {
	let mid = Math.floor((low + high) / 2);
	let item = items[mid];
	let element = elementMap.get(String(item.key));

. The elementMap uses the data-keys on the nodes which are now prefixed:

react-spectrum/packages/@react-aria/dnd/src/ListDropTargetDelegate.ts

Lines 90 to 97 in 50c7ada

	let elements = this.ref.current.querySelectorAll('[data-key]');
	let elementMap = new Map<string, HTMLElement>();
	for (let item of elements) {
	if (item instanceof HTMLElement && item.dataset.key != null) {
	elementMap.set(item.dataset.key, item);
	}
	}

but then we use the collection keys (aka non prefixed keys) to access the items in the elementMap on line 108.

Noticed this break when testing the RAC GridList drag and drop doc examples via mouse drag

[nwidynski]

Thanks, sometimes I wish test suite would cover these 😓

As mentioned above, with the proposed collection data-key there is an edge case in here where a nested collection could cause the drop target to be incorrectly determined as the nested collection.

<GridList id="grid" data-key="grid">
  <GridListItem id="dog" data-key="grid:dog"/>
  <GridListItem id="cat" data-key="grid:cat">
    <GridList id="grid:dog" data-key="grid:dog">
      <GridListItem id="dog" data-key="grid:dog:dog" />
    </GridList>
  </GridListItem>
</GridList>

I suppose we could ship around this with additional query selectors (probably for role?) but still wanted to ask upfront whether this is the path we want to head down.

[LFDanLu]

Yeahhhh, I think we are going to need to expand the coverage of the RAC tests, I think we were hoping the tests in RSP would be redundant enough since they leverage the same hooks, but the different Collections API is proving that wrong.

As for the edge case checking for role could certainly help. I think in general that the ListDropTarget delegate and other drag and drop logic will need to be updated for nested collection support anyways. Would changing the idScope from matching the user provided id to an autogenerated one help reduce the likelihood of this edge case as well? Reusing the id as the idScope feels more of a convenience and is certainly helpful for the user if they wanted to use data-key as a targeting strategy, but I'd say its not really meant as public API.

[nwidynski]

Excuse the downtime, I hope you guys had a great time off as well 🎉

Based on what we last discussed, I figured storing the scope in a dedicated data-scope attribute should be the safest for now. I will try to preserve the targeting strategy as long as possible, but will keep in mind that removing it is an option.

With this, dnd behavior should be restored - not sure whether hooks need additional adjustments tbh. A lot of the node key dependent behavior appears to be happening in the delegate. As mentioned above, I added the util in @react-aria/collections for now, but will adjust based on feedback.

I also added coverage of the work done here, but I will try and see what I can do about larger test suite enhancement in the coming weeks. Ideally those as well as the fix for useId could be deferred into a follow up.

[LFDanLu]

Thanks for the updates and hope you had an enjoyable break as well! I will try to take a look at the changes soon, but just wanted to let you know that we are gearing up for another release so responses may be slow. Due to the nature of the changes here as well as the revelation that our test suites didn't catch as much as I would've liked, I'm leaning towards getting this into a testing session next week and having it bake a bit so that we can be sure that no other regressions get introduced. Unfortunately, this probably means it will miss the coming release window, apologies about that.

As for the test suite enhancement/useId fix, I'd personally be happy for that to be a follow up. I'll see if I can get some resourcing/time to help out with that work as well.