fix(ui): forward ToggleGroup's orientation to the primitive - #166
Merged
Conversation
Base UI's composite drives arrow-key movement from `orientation`, but our wrapper destructured the prop and spent it on `data-orientation` and the class list only, so the primitive never saw it and kept its horizontal default. The vertical date-preset group added in #165 was the first caller to notice: visually a column, but Left/Right moved through it and Up/Down did nothing. ArrowDown → focus did not move ArrowRight → focus moved The four pre-existing callers are horizontal, which is why this survived until a vertical group existed. Comes with a regression test, which needs vitest to accept `.tsx` at all: `include` now covers `src/**/*.test.tsx`, and a component test declares `@vitest-environment jsdom` in its own docblock so the node default still applies to everything else. No projects, no CI change. Worth stating why the sibling wrappers are not the same bug: `size` on Card, Avatar, Select, Switch and AlertDialog, and `align` on InputGroup, are styling-only props shadcn expresses as data attributes on purpose. `orientation` is the one that changes what the primitive does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A bug in #165, found by finally running the keyboard check that PR shipped without.
What is wrong
ToggleGrouptakesorientationas its own prop, destructures it, and spends it ondata-orientationand the class list. It never reachesToggleGroupPrimitive. Base UI's composite drives arrow-key movement from the real prop, so it kept its horizontal default.The vertical date-preset group added in #165 is the first caller to notice. It renders as a column but moves with Left/Right, and Up/Down does nothing:
The four pre-existing callers (
appearance-toggle,date-range-selector,account-list,holdings-table) are all horizontal, which is why this survived until a vertical group existed.Fix is one line: pass
orientationto the primitive as well as the data attribute.How this got in
#165 shipped the keyboard behaviour unverified, and I said so in its description — CI cannot see it, since there were no component tests. I also said I could not verify it without Chrome remote debugging. That was wrong: Base UI's roving focus is plain JS and jsdom drives it fine. Five green checks and a reviewed mock did not catch a control navigating along the wrong axis.
The regression test
src/components/ui/toggle-group.test.tsxpins all four directions — vertical moves on ArrowDown and not on ArrowRight, horizontal moves on ArrowRight, and the group is a single tab stop.Getting it to run at all needed vitest to accept
.tsx:includenow coverssrc/**/*.test.tsx.@vitest-environment jsdomin its own docblock, so thenodedefault still applies to every other file.No projects, no workspace, no CI change. This also closes the "component tests are not currently possible" gap recorded in
AGENTS.md— worth a follow-up to reword that line once this lands.Not the same bug
sizeon Card, Avatar, Select, Switch and AlertDialog, andalignon InputGroup, follow the same destructure-to-data-attribute shape. Those are styling-only props shadcn expresses that way deliberately.orientationis the one that changes what the primitive does, which is what made it a bug. Checked, no other instances.Testing
pnpm typecheck,pnpm lint, and the new test under the project's real vitest config all pass (4/4).Still not verified in a browser. The test drives Base UI's focus management directly, which is where the bug was, but a pass through the Reports date picker would confirm it end to end.
🤖 Generated with Claude Code