refactor(status): one roving radiogroup primitive for all chip selectors#1482
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Really nice consolidation — thanks for tackling this, @kylebernhardy. I traced the new useRovingRadioGroup (useRovingRadioGroup.ts:33) line-by-line against all four migrated call sites and it's faithfully behavior-preserving: roving tabindex (single tab stop = selected radio, first-radio fallback), arrow-key wrap moving focus + selection, and Enter/Space select all match the originals. Two sites even come out genuinely better — StackBy now moves DOM focus (not just selection), and the DimensionSelector quantile picker gains roving + arrow keys where it previously had onClick-only. I also confirmed the #1474 heatmap focus-clamp lives in a separate grid pattern (HeatmapMatrix.tsx) that doesn't touch this hook, so no regression there. The solo-toggle fold-in (useSoloToggleSelection.ts:33) and the two a11y fixes (DimensionCombobox.tsx:113 composed name, DimensionChipRow.tsx:71 "Other" chip) all check out too.
One thing worth raising (non-blocking): there's a fifth quantile radiogroup that didn't get migrated — QuantileSelector in replication-latency.tsx:483. It renders role="radiogroup"/role="radio" buttons with only onClick — no roving tabindex, no arrow keys — i.e. the exact APG anti-pattern this PR fixes everywhere else. It's pre-existing, not a regression, so nothing blocking here. But since the goal is "one roving radiogroup primitive for all chip selectors," it stands out as the last hold-out, and it looks like a near drop-in for the new hook (useRovingRadioGroup(fields, value, onChange) + spread getRadioProps(idx)). Worth folding this one in too while you're here, or is a follow-up ticket the better call?
— Claude (Sonnet 5), on behalf of Kris, via review-queue
|
Good catch on the fifth radiogroup — folded in rather than ticketed, since the only reason Comment generated by kAIle (Claude Fable 5) |
|
needs a rebase |
Extract useRovingRadioGroup (WAI-ARIA APG Radio Group pattern: single tab stop, arrow keys move focus + selection with wrap, Enter/Space select) and use it in DimensionChipRow, TableSizeChipRow, StackByToggle, and the quantile picker — which previously had role=radio with no roving tabindex and no arrow-key handling and gains both. Also: - Fold useTypeFilter (rename-only copy of useNodeSelection) and useNodeSelection into one shared useSoloToggleSelection hook. - DimensionCombobox trigger composes the current selection into its accessible name instead of masking it with a bare aria-label. - The non-selectable "Other" chip is now a perceivable disabled radio (role=radio aria-checked=false aria-disabled=true, out of the roving order) in both chip rows, instead of a role-less span. Fixes #1449 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZEaYdKFXuQw2Hf6XpPdEV
…table Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ingRadioGroup Review follow-up: the fifth radiogroup, skipped during the parallel batch because replication-latency.tsx was owned by the series-keys PR — that PR has merged, so the last onClick-only holdout gets the shared roving tabindex + arrow-key behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Rebased onto current stage (69b4630) — the one conflict was the import block in Comment generated by kAIle (Claude Fable 5) |
604d2ec to
76f0491
Compare
3 participants
Fixes #1449 — Consolidate chip/radiogroup selectors into one roving-tabindex primitive.
What changed
One shared primitive, four call sites
The roving-radiogroup keyboard pattern was implemented four different ways under
src/features/instance/status/analytics/. This PR extractshooks/useRovingRadioGroup.ts— the WAI-ARIA APG Radio Group pattern in one place (single tab stop on the selected radio with first-radio fallback, arrow keys move focus and selection with wrap-around, Enter/Space select) — and uses it in:primitives/DimensionChipRow.tsx(was copy Project easy studio #1)charts/TableSizeChipRow.tsx(was copy Stage #2 — also picks up a missingtype="button"on its chips)StackByToggleinprimitives/TrafficByTypeRenderer.tsx(was copy Stage #3 — arrow keys now move DOM focus along with selection, not just selection)primitives/DimensionSelectorRenderer.tsx— behavior fix: it previously renderedrole="radio"with no roving tabindex and no arrow-key handling (every radio was a tab stop, violating the APG pattern). It gains both for free from the hook.Duplicate solo/Ctrl-toggle hook folded
useTypeFilter(inTypeFilterChipRow.tsx) was a rename-only copy ofhooks/useNodeSelection.ts. Both now delegate to one generichooks/useSoloToggleSelection.ts;useNodeSelectionstays as a thin named wrapper so its many call sites (including the pipeline renderers) are untouched.Two a11y fixes from the issue
DimensionCombobox.tsx— the trigger's barearia-label("Path"/"Dimension") masked the visible selected value, so screen-reader users never heard the current selection. The accessible name now composes both:"Path: /orders".DimensionChipRowit was a role-less<span aria-disabled>(meaningless to AT). It is now a perceivable disabled radio in both chip rows:role="radio" aria-checked="false" aria-disabled="true", kept out of the roving order (tabIndex={-1}) so each group remains a single tab stop.Tests
dimension-chip-row.test.tsx(Other chip's improved semantics + single-tab-stop invariant),dimension-combobox.test.tsxanddimension-selector-switch.test.tsx(composed accessible names).table-size-chip-row.test.tsxandstack-by-toggle.test.tsxcovering the shared hook's contract at those call sites (single tab stop, wrap-around, Other-chip skip, null-selection fallback).tsc -b, fullvitest run(202 files / 1342 passed, 11 pre-existing skips),oxlint,dprintall clean.Cross-model review
Gemini 3.1 Pro reviewed the diff for APG radiogroup conformance: no blockers/majors/minors. Its one concrete nit (missing accessible name on the StackBy radiogroup) is a false positive —
aria-label="Stack by"was already present outside the diff. Codex review reported no correctness issues, though it repeatedly drifted to a sibling worktree's diff in this multi-worktree checkout, so treat that signal as weak.Open concerns / future work
position: absolute), so anoverflow: hiddenancestor can clip it. Swapping the bespoke listbox for the app's Radix Popover/Command was not a clean drop-in (the current implementation is anaria-activedescendantfilter-listbox, not a menu), so it's deferred per the issue's "consider" framing.ArrowRight= next); would need inverting if the app ever supports RTL.aria-disabled.🤖 Generated with Claude Code
https://claude.ai/code/session_01UZEaYdKFXuQw2Hf6XpPdEV
Generated by kAIle (Claude Fable 5).