feat(TreeView): add opt-in virtualization via height prop#2475
feat(TreeView): add opt-in virtualization via height prop#2475mateoviilla1 wants to merge 6 commits into
Conversation
Long TreeView lists kept every row mounted and re-rendered all of them on each selection change, so a single checkbox click could freeze for ~1s while react-aria reconciled the whole list. Add an opt-in `height` prop: when set to a non-zero value the tree becomes a fixed-height scroll region wrapped in react-aria's Virtualizer + ListLayout, so only the rows in view are rendered and selection changes stay fast. When omitted (or 0) the tree grows to fit all rows exactly as before — the non-virtualized path is unchanged. - ListLayout gap/padding mirror the .TreeView --space-quarter/--space-half tokens so virtualized and non-virtualized rows are spaced identically - caller-supplied style is merged with the height (height wins) rather than clobbering it - .TreeView--virtualized neutralizes flex/gap/container padding since rows are absolutely positioned and spacing comes from the layout options - docs + unit tests (incl. virtualized cascade selection) + e2e screenshot Ref dequelabs/axe-reports#3277
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Pull request overview
Adds opt-in virtualization to TreeView via a new height prop, aiming to keep selection interactions responsive for very large trees by only rendering rows in view while preserving existing “grow-to-fit” behavior when height is omitted (or 0).
Changes:
- Introduces
height?: number | stringto enable a virtualized rendering path usingreact-aria-componentsVirtualizer+ListLayout. - Adds
.TreeView--virtualizedstyling to neutralize non-virtualized layout (flex/gap/padding) so spacing comes from the virtualizer layout options. - Expands automated coverage with new unit tests, docs examples, and an e2e screenshot case for the virtualized mode.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/styles/tree-view.css | Adds a virtualized modifier class to adjust layout behavior for absolutely positioned virtual rows. |
| packages/react/src/components/TreeView/index.tsx | Implements opt-in virtualization via height and merges height with caller style. |
| packages/react/src/components/TreeView/TreeView.test.tsx | Adds unit tests for virtualization activation, height behavior, style merge, a11y, and cascade selection. |
| packages/react/src/components/TreeView/screenshots.e2e.tsx | Adds a new visual regression case for a long, virtualized TreeView in light/dark themes. |
| docs/pages/components/TreeView.mdx | Documents virtualized usage and adds height to the props table plus examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…aselines - type TreeViewProps with React.HTMLAttributes<HTMLDivElement> so style/ data-*/handlers are typed (matches ProgressBar); drop the style cast in the component and the object cast in the style-merge test - clarify the height prop docs: 0 is treated as unset (non-virtualized) - fix the virtualized e2e case: the screenshot harness sizes the mounted root to content (min-height/max-width: max-content on #root > div), which overrode the fixed height and collapsed the absolutely-positioned virtual rows; wrap in a div + give the tree an explicit width so it renders the fixed-height scroll region as in a real app - add the tree-view-virtualized light/dark baselines (only the ~8 visible rows of a 200-item list render, confirming virtualization windows correctly) Ref dequelabs/axe-reports#3277
|
Addressed the review feedback in d14b8cc: Copilot comments
Screenshots failure While generating the baseline I found the virtualized screenshot rendered the full 200-row tree instead of a clipped scroll region. Root cause was the CT harness template ( |
rheisler-deque
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR to add this feature to Cauldron. I have tested the virtualization functionality in the Amplitude build, and the component is definitely only rendering a small number of rows near the visible set.
This would be the first example of virtualization in Cauldron. We try to make sure Cauldron components are accessible and useful in a general sense, not just to solve a specific problem that one consumer needs to solve. I think adding virtualization to the TreeView is reasonable because it's likely to have an issue with virtualization for any consumer. Virtualization in the TreeView could serve as a model for other components that need it, and later be generalized to wrap any component if needed.
However, I don't think this implementation is ready to be integrated into Cauldron, which will affect all consumers including those outside Deque. I've noted a potential major accessibility issue below, and it's missing features most consumers need when virtualizing a large list of elements. This PR sets the pattern for virtualization we'll use in other elements in Cauldron when more virtualization is needed, so I want to make sure we identify the gaps and make a plan to fill them before this gets merged.
Accessibility Concern
Elements that are removed from the DOM are also removed from the accessibility tree. That means a user exploring the TreeView with a screen reader may not be able to perceive all the elements without scrolling them into view. Right now, you can scroll elements into view with the Up, Down, PageUp, and PageDown keys, but assistive technologies change key behavior, so we would need to verify that they still work with all major screen readers active, and that mobile screen reader users can navigate the list.
Assistive Technology users often explore a page by looking at all the elements of a certain type. JAWS for example, has a view to show you all the buttons, all the links, all the list items, etc. TalkBack on Android lets you navigate from element to element of a specific type, but only for elements that are already rendered. I believe a user doing either of these things would only perceive the elements of the TreeView that were rendered when they arrived on the page, and I'm not sure they would have a way to actually reach them.
In this implementation, they do have a way to perceive that there are unrendered elements, which is very good. In virtualized mode the rows keep correct aria-level, aria-posinset, and aria-setsize. A child row still reports posinset=N setsize=200, so I'm pretty sure it would announce "item 51 of 200" if there were 200 items in the tree.
Upstream Issue
In virtualized mode the treegrid gains aria-rowcount set to the full row total, but the rows never get a matching aria-rowindex. When a grid or treegrid sets aria-rowcount because the DOM holds only a subset of rows, each rendered row is supposed to carry aria-rowindex so assistive technology can place it in the full set. That pairing is missing here.
The cause is in react-aria-components, not this PR. Its useTree sets aria-rowcount from the virtualized flag, but its TreeItem never passes that flag down, so the aria-rowindex branch never runs. We cannot fix it from the Cauldron wrapper without patching react-aria. The practical harm is limited, because position still reaches the user through aria-posinset and aria-setsize, and axe does not flag the missing pair. Suggestion: file this upstream with react-aria and note the inconsistency in the documentation.
Usability Gaps
- Related to the accessibility issue above, browser-find (pressing CMD+F or Control+F) stops working with this PR. Virtualization removes off-screen rows from the DOM, so browser-find cannot find text in the virtualized rows. TreeView has no search or filter prop, so there is no replacement. A consumer who sets
heightloses find-in-page over the full tree with no warning, making the TreeView less perceivable especially for sighted keyboard users. - Similarly, there's also no way to reveal or jump to an off-screen node. Consumers often need to scroll to the selected node or to a node a search matched. The public props expose no scroll-to-key or reveal-node method.
Technical Gaps
- Expansion is uncontrolled, meaning a consumer cannot implement its own solution to searching the tree. TreeView supports only
defaultExpandedKeys(index.tsx:44), so a consumer cannot expand a matched node's ancestors under program control, which they would need to do to implement a search function. This is pre-existing, but this PR makes the impact worse. - The whole
itemsarray must be built up front. There is no lazy or async loading of children. This caps how large a tree the feature really serves. Rendering is much more memory-intensive than holding list items in memory, but I think virtualization as a feature should make it possible to virtualize a list that is too large to hold, not just to large to render. This could be deferred to later because virtualizing rendering solves the more common, more impactful problem.
Path Forward
I think Virtualization for the TreeView needs a holistic UX design including accessibility consideration because it's a feature, not a bug fix. This design should fill the gaps above as well as any I missed, and explicitly specify the user experience for assistive technology users based on testing with real ATs.
Once we have that, I think your team and the Cauldron maintainers need to decide which parts of the feature will be implemented now and which will be deferred. We should have tickets covering anything that won't be done now, and the docs should mention which parts of this feature are not yet implemented.
Finally, QA will need to test the user experience with assistive technologies to make sure it matches what is intended holistically, rather than just fixing the bug that led to this PR.
I'm not the gatekeeper of what goes in Cauldron. I'm very happy to discuss my concerns if you disagree with them. I'd like to hear from more people from @dequelabs/cauldron-codeowners about what else this feature would need to be included in Cauldron, and from members of @dequelabs/cauldron-design about what it would take to design this feature comprehensively. Mateo is trying to fix a bug, and I want to give him a realistic idea of what it will take to do that with virtualization.
…ualization tests - treat "0"/"0px" the same as numeric 0 (non-virtualized) so an unmeasured string height doesn't render a zero-height, row-less scroll region - reword "janky" in prop docs/JSDoc to plain language - unit tests: prop height wins over a conflicting style.height; string-zero height stays non-virtualized - e2e: assert only a subset of the 200 rows is in the DOM (windowing regression guard) and add a keyboard-focus-retention test across scroll - docs: note browser-find loss, missing scroll-to API, and the upstream react-aria aria-rowindex gap in virtualized mode
|
Thanks @rheisler-deque — this is a really helpful framing, and I agree that virtualization is a feature that deserves a holistic UX + a11y design rather than landing as a bug-fix side effect. I've addressed the concrete inline suggestions in f3ac120 (string-zero normalization, the "janky" rewording, the windowing DOM-count assertion, the keyboard-focus-retention e2e, and the conflicting-
On the bigger questions — the AT experience validated against real screen readers, controlled expansion for search, lazy/async children, and which pieces ship now vs. get deferred with tracking tickets — I don't think I should decide those unilaterally. I'd like to get @dequelabs/cauldron-codeowners and @dequelabs/cauldron-design input on scope before I put more into this PR. I'm happy to drive whatever slice we agree lands now and open tickets for the rest. Worth noting the original driver was a downstream perf bug (dequelabs/axe-reports#3277); if the holistic design is a larger effort, one option is to keep this PR as the rendering-perf fix behind the documented caveats and track the fuller feature separately. Open to whichever path the team prefers. |
getByRole('row', { name: 'Item 1' }) matched Item 1/10/11/12 and tripped
Playwright strict mode. Match exact names for the click target and the
asserted focused row.
|
It looks like there is not a cheaper way to address the performance issue (in Cauldron at least), so virtualization seems like a solid approach. I think that the per-click freeze we are running into is likely caused by react-aria re-rendering every row on selection change. This may be fixable upstream by having each row subscribe only to its own selection state; could be worth logging an issue in react-aria. One other thing worth checking during AT testing: because the tab stop pins to the last-focused row, mixed mouse-scroll + keyboard re-entry can snap the view back when the current scroll point differs from the last focused item. This is pretty minor though. To conclude, I think it's probably worth landing this now. Have you tested on the original subdimensions tree to see if it fixes the sluggishness? I think that some of the improvements that Ryan suggested are probably okay to land as follow-ups, assuming our designers agree with them. |
|
@frankensteinke Yes, I have tested with a subdimensions tree of 400 children, and cascade selection works properly. |
Performance analysis: why the freeze scales with tree sizeSharing the cost model behind the lag, with locally-seeded test trees of increasing size. Separating what's exact (complexity + row counts) from what's estimated (the ms constants). What's actually mountedThe step force-expands the whole tree (
The cost modelTwo regimes, both linear in N (rows mounted): 1. Initial mount/paint — create + lay out + style-recalc all N rows: 2. Per-click re-render — the actual pain point. react-aria's Tree re-renders every mounted row on any The O(N) tree build and the cascade Set-update are both dominated by this — cascading a parent adds O(children) keys, then still triggers one full O(N) re-render. Nesting itself adds no algorithmic complexity; it just inflates N (a flat 10,100 freezes identically). Anchoring
Where slowness starts (answering the row-count question)
What virtualization changesThe Numbers from locally-seeded perf trees; complexity/row counts are exact, ms figures are estimates anchored to prior local measurement. |
Summary
Adds opt-in virtualization to
TreeViewvia a newheightprop.Long lists kept every row mounted and re-rendered all of them on each
selection change, so a single checkbox click could freeze for ~1s while
react-aria reconciled the whole tree (surfaced downstream in axe-reports).
When
heightis set to a non-zero value, the tree becomes a fixed-heightscroll region wrapped in react-aria's
Virtualizer+ListLayout, so only therows in view are rendered and selection stays fast regardless of list length.
When
heightis omitted (or0), the tree grows to fit all rows exactly asbefore — the non-virtualized path is unchanged.
Details
ListLayoutgap/paddingmirror the.TreeView--space-quarter(2px) /--space-half(4px) tokens, so virtualized and non-virtualized rows arespaced identically.
styleis merged with the height (height wins) rather thanclobbering it or being clobbered by the
...otherpassthrough.heightis documented as a stable prop — toggling it at runtime remountsthe underlying tree (resets uncontrolled expansion/scroll/focus).
.TreeView--virtualizedneutralizes flex/gap/container padding, sincevirtualized rows are absolutely positioned and spacing comes from the layout.
cascadeSelect/cascadeDeselect(covered by atest + docs example).
Testing
pnpm --filter @deque/cauldron-react test— 39 TreeView tests pass, incl.virtualized render/selection, zero-height fallback, style merge, virtualized
cascade, and a
jest-axecheck. Existing snapshots unchanged.update-screenshotslabel.QA notes: exercise a long tree with
heightset — scrolling windows rows, andcheckbox clicks toggle instantly; confirm a tree without
heightis unchanged.Ref dequelabs/axe-reports#3277