Feat: Add lifecycle callback to useId

[nwidynski]

This PR adds support for lifecycle hooks to a useId. This can be useful to build global state with unique ids as the key, in scenarios where a common state object isn't accessible.

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

change seems fine, though having a more concrete use case would be good

I know GC doesn't get called in jsdom (at least not easily) wonder if we should start mocking the FinalizationRegistry so we can get some tests around this as it's growing in complexity

[nwidynski]

@snowystinger Let me elaborate on a specific use case - I began work on #7240, starting with unifying the labeling across all collection components. This PR will unify the id generation of nodes, in order to make them predictable from a parent collection builder. The result will be the id option on collection item hooks becoming misleading in the sense that it wont affect the DOM anymore - we therefore ideally remove it.

For backwards compatibility, this is a problem for useMenuItem, which is relying on its id option to receive the id generated by useSubmenuTrigger. Now to share the submenu trigger id, we need a way to hold global state in a Map that can be accessed by both menu and trigger hooks - meaning we will use the root menu id as our Map index.

To tidy it all up, we need this additional callback on useId to cleanup our Map 😄

PS: Having a predictable id generation for nodes will allow us to connect adjecent collections, such as a tablist's tabs, with presentational nodes that are rendered around listbox items in a tabbed Carousel pattern:

// => Carousel's renderer wraps items with <div role="presentation" id="<tabpanel-id>" />

<Carousel>
  <Button slot="previous" />
  <Button slot="next" />
  <ListBox>
    <ListBoxItem id="1" />
  </ListBox>
  <TabList>
    <Tab id="1" />
  </TabList>
</Carousel>

[snowystinger]

Thanks for the explanation. I have a feeling the team will want to discuss this Carousel API, it's a bit unusual. If you're planning on trying to contribute it, it'd be best to outline your plan in an RFC so we can discuss it there and reach agreement.

[nwidynski]

@snowystinger Got it, for now I‘m only focusing on making non breaking changes to the primitives, aka hooks and state, to get them where they would need to be in order to support building a component like this on top. Some of these changes address open issues or make sense regardless of whether the carousel makes it or not, so I’m spinning these out into separate PRs, to be reviewed independently.

For the component layer or implementation specific changes, I will keep in mind to open an RFC before PRs 👍 Do you think this change here is good to go?

[snowystinger]

bringing up with the team

[devongovett]

Not sure I quite understand. Usually in React you want state to be owned by a parent component and flow downward to children. Having state owned by a sibling is usually quite problematic as React does not guarantee a render order. That's why for example Tabs owns the state that both TabList and TabPanel access.

[nwidynski]

@devongovett Can you elaborate on which part you want me to clarify? Are you referring to the Carousel API or the menu hook problems? I suppose its mostly the menu problems you are interested in, but let me clarify the background:

To implement W3C's tabbed Carousel API, we hoist a tablist's state to CarouselInner, which is getting its collection built from the nested ListBox, pretty similar to what we do for ListBox & ComboBox. The issue we have at hand is that the id attribute of a TabPanel is entirely unrelated to what node key it points to - instead it currently is generated from the parents id, which sometimes does not even accept defaults. To pass this info around, collection items often leverage a WeakMap using their parents state - more on that in a sec.

This architecture makes it impossible for a parent component to reliably target a collection DOM element via its id. For us, this manifests in Carousel not being able to render a presentational wrapper for ListBoxItem, which would pose as the TabPanel for our TabList. To support this use-case, I would like to streamline all collection item hooks to a single way of generating the id attribute, using your getCollectionId() helper.

Node id attributes would always be based on the collection id, its aria role and its node key (e.g. <collectionid>-tabpanel-<key>). As a result, I would remove the id property of these hooks whenever still present, as this has previously caused issues #7978. Constructing ids that way, enables Carousel to calculate the correct id attribute for its pseudo tabpanel, making each tab of the users TabList have a valid aria-controls.

Now I have started work on getting the id changes done in a non-breaking PR, but have ran into issues with useMenuItem, which relies on its id prop to receive its id from the parent submenutrigger hook. As you identified, the state still needs to flow from the submenu trigger to the trigger item. Unfortunately though, there is no shared state object we could use to attach ids to and most other ideas would be breaking changes.

Luckily for us, the JSX structure of trigger elements and Menu in general is very fixed, allowing us to rely on the synchronous part of a trigger items render to always execute before its respective Menu, similar to our hidden collection render. Parts of this implementation unfortunately require strong maps, which we would like to properly clean up using the functionality of this PR.

I hope this could help - again, I will elaborate more on carousel in a dedicated RFC 👍 I can commit a WIP PR of the id changes if you would like to see code next to what im saying here.

[devongovett]

I'm unsure how the ListBox and Tabs relate to each other within a carousel pattern. The APG Example doesn't have a listbox, just tabs. An RFC would definitely help there.

the id attribute of a TabPanel is entirely unrelated to what node key it points to

I think it generates based on the a base id (attached to the state) and the collection node's id:

react-spectrum/packages/@react-aria/tabs/src/utils.ts

Line 31 in 4514dd7

return `${baseId}-${role}-${key}`;

For us, this manifests in Carousel not being able to render a presentational wrapper for ListBoxItem, which would pose as the TabPanel for our TabList

So is the problem here that you're using two different collection components (ListBox and TabList) and trying to sync them somehow? Understanding how these fit together would help.

[nwidynski]

@devongovett The problem here is completely independent of Carousel, and more so connected to my work on #7978 and #7240. It just so happens that Carousel was the reason why I picked up these issues, since it will make use of the fixes.

I think it generates based on the a base id (attached to the state) and the collection node's id:

That's exactly the point - the parent wrapper has no way to determine the internal baseId without querying the DOM and then rerendering. I somehow need access to the generated id of Tab inside of Carousel. My suggested way of achieving this was to rework id generation so baseId is replaced by getCollectionId(). This isn't specific to Tab but rather all useSelectableItem() derivative hooks, e.g. useGridListItem, useOption, etc. In this case the implementation specifics of useMenuItem prompted this PR, which I spun out since it makes sense regardless.

As to the requested relation between ListBox and TabList - Just like you hinted on X/Twitter, Carousel in essence can be thought of as a display only. Thereby we started off implementing the W3C basic style of a carousel, which only has the basic rotation controls and is pretty much just a collection of <div role="group" />.

Now role="group" containers are already well supported and utilized by our implementation of the ListBox pattern, so we thought it could make a lot of sense to have Carousel be more of a generic collection rotator, similar to Autocomplete. Additionally, our use case for Carousel requires support for virtualization, selection and focus control with multiple slides per view, while the APG examples only showcases very basic implementations with single slide views and no interactions - this only supported that design choice.

After finishing up the implementation using ListBox, we thought it may be viable to expand upon to provide consumers of our library an option to seamlessly use a "fake" tabbed carousel style, simply by rendering a TabList (Carousel poses as Tabs) as an adjacent collection, which in turn causes the role="group" slide wrappers to acquire tabpanel ids. As you suspected, TabList in this case really just means a second collection which is in sync. The plan is to sync both of these collections using #8553, which would allow us to provide multiple collection builders inside Carousel to build both a ListBox AND TabList state in the parent, then filter one by the other. Since the TabList state after filtering is guaranteed to only contain valid ListBox node keys, its Tab elements will automatically have valid aria-controls values set. Optionally, we could also attach additional information, e.g. ListBoxItem node props, to the corresponding Tab node, which could make it trivial to provide accurate aria-label defaults.

Since our implementation pretty much let's ListBox do all the heavy lifting for keyboard and VO users, we don't have much of an accessibility concern for Tabs or the rotation controls anymore, as they pretty much only control scroll offset, and could possibly even be entirely hidden from the tab sequence. We use a Layout derivative internally to calculate the scroll positions for rotation control and apply native CSS scroll snapping through the layout infos style.

Now we are in the middle of testing exactly how this would get announced, if TabList were exposed to keyboard and VO users properly, and whether its worth it to change focusedKey when changing Tab. Maybe we need to add an option to useTab and useTabPanel to change its aria semantics or render a second ListBox slot="tabs" instead, but this is all TBD in the RFC, don't take these specifics too serious for now 👍