Repository navigation
feat: support description text in tree items - #10710
minwookshin wants to merge 8 commits into
Conversation
Developed and tested with assistance from OpenAI Codex.
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
In #10665 you asked
Would you prefer aria-describedby here, rather than GridList's combined aria-labelledby behavior?
Did you test both with various combinations of screen readers/OS/browser? If so, do you have the data that informed your decision?
|
I checked the browser accessibility tree, but I haven't compared both approaches with screen readers yet. |
|
Let's match how |
| // TODO: should it return a state specifically for isExpanded? Or is aria attribute sufficient? | ||
| return { | ||
| ...gridListAria, | ||
| rowProps: { |
There was a problem hiding this comment.
I assume you don't need this override. We can just inherit the behavior from useGridListItem?
| let {onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp} = | ||
| listMap.get(state)!; | ||
| let descriptionId = useSlotId(); | ||
| let descriptionId = useSlotId([node]); |
There was a problem hiding this comment.
I assume this is related to this test?:
updates the description relationship when the slot is removed and added
If so, was this an existing bug in GridList? and can you add a test specifically for GridList. It can be basically the same as the Tree one. That way we can verify before and after
There was a problem hiding this comment.
Would node.key (or id? i'm going off memory for this comment) be better?
There was a problem hiding this comment.
don't forget to answer in comment thread
There was a problem hiding this comment.
Yes, GridList had the same stale aria-labelledby reference; the new test covers removing and re-adding a description with an explicit ID. node.key still fails because the item's key stays the same when the description changes.
There was a problem hiding this comment.
but won't the node change on pretty much every render? I think they are immutable if i recall correctly, so this could be a lot of re-renders?
There was a problem hiding this comment.
You were right: with no description, changing an unrelated item prop caused two extra aria-labelledby updates. I narrowed the dependency to node.props.children, so those updates no longer refresh the slot. Description removal and reinsertion still work.
Added regressions for Tree and GridList. Full unit, SSR and browser suites pass; the browser run needed retries for intermittent WebKit failures in other components.
There was a problem hiding this comment.
If someone is using the renderProps children as a function won't that cause new children every render as well?
There was a problem hiding this comment.
You're right. Inline functions caused extra updates, and stable functions missed descriptions added on selection. The slot now keeps its ID and updates presence only when it changes, which also preserves React 18 focus restoration. Added both render-function cases for Tree and GridList; the full unit suites on React 18 and 19, SSR and browser checks pass.
|
looks like it's failing a test in React 18 |
| let {onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp} = | ||
| listMap.get(state)!; | ||
| let descriptionId = useSlotId(); | ||
| let descriptionId = useId(); |
There was a problem hiding this comment.
I appreciate that you looked into this, however, it is pre-existing. Unless you have a use case that you need this for though, let's remove these changes.
If you do have a need for it, lets open a new issue and PR to address it so that we don't incur performance hits.
There was a problem hiding this comment.
A callback ref in descriptionProps should work. I agree though, this is out of scope. Other hooks likely have the same problem, so this should be fixed together.
There was a problem hiding this comment.
Removed the shared GridList hook changes and dynamic-slot tests. This now uses the existing GridList behavior and stays focused on the Tree description slot. Full unit, SSR and browser suites pass; the Tree/GridList tests also pass on React 18.
Closes #10665
Tree items need a description slot that follows the existing GridList accessibility behavior.
Text slot="description"now uses the inheritedaria-labelledbyrelationship, so the row name includes its description. This removes the Tree-specific override and adds the slot, story, documentation, and static naming regressions. The shared GridList hook is unchanged.Implemented with OpenAI Codex.
✅ Pull Request Checklist:
📝 Test Instructions:
Open React Aria Components → Tree → With Descriptions. The parent row should be named “Projects Shared project files”; expanding it reveals “Report Last updated today”. Check keyboard selection, expansion/collapse and pointer activation. Regressions cover combined row names, explicit item labels, and rows without a description.
October 21 validation after narrowing the scope: full unit tests pass (8,441 tests, 16 skipped), SSR tests pass (74), and browser tests pass (423, 63 skipped). The 215 Tree/GridList tests also pass on React 18.3.1 with strict mode and virtualization enabled. Formatting, type checking, lint, package checks and constraints pass.
The shared description-presence changes and dynamic-slot regressions have been removed following review. Dynamic description updates remain the existing GridList behavior and are outside this PR's scope.
Earlier story verification passed keyboard/pointer checks in Chromium, Firefox and WebKit with English/light/wide and Arabic/dark/narrow layouts. Chromium's accessibility tree and native Safari both expose the combined row name. VoiceOver speech remains unverified because the local caption-window inspection timed out; its settings were restored. Touch and high-contrast mode were not checked.
Storybook used the standard Parcel server with a local Unix-socket path-length workaround. Neither the workaround nor test fixtures change repository files.
🧢 Your Project:
Open-source contribution.