feat(ui-inputs): add GroupSelect and GroupCombobox atoms (v0.13.0) - #224
feat(ui-inputs): add GroupSelect and GroupCombobox atoms (v0.13.0)#224Elenyan-Script wants to merge 11 commits into
Conversation
Two new atoms for grouped-option listboxes, mirroring the existing SingleSelect/Combobox and MultiSelect/MultiCombobox split (ADR-0043): - GroupRow union type (header | option) so group headers interleave with navigable options in a single row sequence without occupying index space - GroupOptionList.vue — internal listbox popup; headers carry role=presentation so they are valid listbox children but are skipped by the keyboard path - GroupSelect.vue — button trigger, non-searchable; flatOptions bridges all groups for index-keyed pointer, commit, and aria-selected - GroupCombobox.vue — input trigger with per-group filtering; WR-0576 browse-to-change equality rule, select-all-on-open, dismiss revert, and async edit-form re-sync watch; defineExpose focus handle (WR-0448) - 46 tests (22 GroupSelect + 24 GroupCombobox), 100% coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- v8 ignore the unreachable race-guard `return false` in both commit() functions (defensive guard against a concurrent filter change between keydown and the watcher flush — cannot be triggered in tests) - GroupSelect: add empty-group skip test (line 151 continue branch) and clear-entry mouseover test (GroupOptionList clearHover emit) - GroupCombobox: add option mouseover test (@hover handler), getter label test (labelOf function branch), header=false test, and mutedOptions test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
1 issue · 4 nitpicks · head 0c37abeb9a
Crit requests changes — 1 issue.
Issues
ui-inputs version bump for new public exports ships without a changeset file
packages/ui-inputs/package.json:3 — see inline
4 nitpicks
GroupSelect and GroupCombobox classes have no CSS rules in styles.css
packages/ui-inputs/styles.css:437 — GroupSelect.vue and GroupCombobox.vue apply ui-groupselect__* and ui-groupcombobox__* classes, but styles.css's shared menu/option/clear/empty rule blocks enumerate only the select, combobox, multiselect, and multicombobox variants. Neither new SFC carries its own style block. The popup menu, options, clear entry, empty row, and the new group-header row render with no background, padding, hover, or selection styling for these two components.
nitpick because pre-existing — this pull request did not write those lines
Group header v-for key is derived from display text, not group identity
packages/ui-inputs/src/components/GroupOptionList.vue:358 — GroupOptionList.vue keys header rows as h:${row.text} in its v-for loop instead of using a stable group identifier. Two groups sharing the same display text produce duplicate keys. On a re-render triggered by filtering, Vue's keyed diff can reuse or move the wrong header DOM node between the two same-named groups.
nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Requires exercising a live combobox with two same-text groups through a filter-triggered re-render and inspecting DOM node reuse to confirm a visible mis-render actually occurs.
axe-core browser audit suite omits the new GroupSelect and GroupCombobox components
packages/ui-inputs/tests/browser/axe.browser.spec.ts:17 — axe.browser.spec.ts audits every other listbox-family component — SingleSelect, Combobox, MultiSelect, MultiCombobox and others — but never imports or audits GroupSelect or GroupCombobox. GroupOptionList.vue reproduces the same role=option empty-row and clear-entry patterns the file's own comment says exist to be audited in a real browser. A real ARIA defect in the new grouped-listbox markup, such as an invalid aria-required-children combination, ships without this suite catching it.
nitpick because no runtime path — nothing reaches the harm at this head; pre-existing — this pull request did not write those lines
Real-Chromium interaction test suite omits GroupSelect and GroupCombobox
packages/ui-inputs/tests/browser/interaction.browser.spec.ts:15 — interaction.browser.spec.ts exercises real pointer and keyboard CDP events for every other listbox-family component but does not import or test GroupSelect or GroupCombobox. Native disabled-input click suppression, Popover API top-layer rendering, and floating-ui positioning under autoUpdate for the two new components are validated only by happy-dom's synthetic events. Any real-browser-only divergence in these behaviours would go undetected.
nitpick because no runtime path — nothing reaches the harm at this head; pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Cannot run the Playwright/Chromium lane in this review to confirm an actual behavioural divergence between happy-dom and real-browser event handling for the two new components.
There was a problem hiding this comment.
Adds GroupSelect/GroupCombobox atoms mirroring the existing select family, but ships zero CSS for the new class prefixes so the popup renders unstyled — plus group headers aren't conveyed to assistive tech and the option-list markup is forked rather than shared.
Not anchorable to the diff
- [high] GroupSelect/GroupCombobox class families ship with zero CSS rules — New
ui-groupselect/ui-groupcomboboxclasses (GroupSelect.vue, GroupCombobox.vue, GroupOptionList.vue) match no selector in styles.css — every existing rule enumerates only the four legacy families (.ui-select__menu, .ui-combobox__menu, .ui-multiselect__menu, .ui-multicombobox__menu:437-440, plus__option/__clear/__empty/chevron rules). styles.css is a published export ("./style.css": "./styles.css"), so any consumer importing it renders the popup unstyled: transparent/borderless/unscrollable, no option highlight, chevron falls back to 300x150 default, group-header indistinguishable from options. Fix: extend the existing shared selector groups with the two new prefixes, add a__group-headerrule.
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 2 nitpicks · head 67187cb615
Crit requests changes — 1 open thread.
2 nitpicks
GroupSelect and GroupCombobox are never run through the real-browser axe-core audit
packages/ui-inputs/tests/browser/axe.browser.spec.ts — axe.browser.spec.ts mounts eleven existing components inside FormField and asserts zero axe violations in both open and closed states, but the new GroupSelect and GroupCombobox components are absent from its import list and from interaction.browser.spec.ts. These two components introduce a DOM shape none of the audited components use: role="presentation" header
nitpick because pre-existing — this pull request did not write those lines
Group header row keys collide when two groups share the same header text
packages/ui-inputs/src/components/GroupOptionList.vue:349 — GroupOptionList.vue keys header rows in its v-for as h:${row.text}, using only the group's display text with no group index or identity mixed in. The groups prop on both GroupSelect.vue and GroupCombobox.vue is typed as {options: T[]; text: string; header?: boolean}[] with no uniqueness requirement on text, and nothing in either component de-duplicates group text before it reaches GroupOptionList. When a caller passes two groups with identical text, such as two 'Other' buckets from server data, Vue receives duplicate keys in the same render pass, causing a duplicate-key warning and undefined vnode reuse between the two header nodes.
nitpick because pre-existing — this pull request did not write those lines
Still open
packages/ui-inputs/src/components/GroupOptionList.vue — already filed, still open
Settled, not re-filed: 2
…eader ARIA Two issues raised in PR review: 1. GroupSelect/GroupCombobox shipped with zero CSS rules — every existing shared selector block enumerated only the four legacy families. Extended menu, option, muted/active option, clear, clear-active, empty, and reduced-motion blocks with the two new prefixes. Added GroupSelect trigger/placeholder/chevron rules. Added group-container and group-header rules keyed on new --ui-group-header-* vars. 2. Group headers used role="presentation", making group names invisible to AT. Changed GroupOptionList to the APG listbox grouping pattern: named groups render as <li role="group" aria-label="…"> with the visual header as an aria-hidden="true" span inside, so screen readers announce the group name without double-reading it. Options with header:false still render flat (no group wrapper). Updated two GroupSelect tests that asserted the old role="presentation" structure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… coverage
Tighten .ui-group{select,combobox}__group ul to a direct-child selector (> ul)
so consumer <ul> elements inside the #option slot are not silently stripped of
list-style/padding/margin.
Add GroupSelect and GroupCombobox to axe.browser.spec.ts: three real-browser
axe-core cases covering the grouped listbox DOM shape (role="group" + role="option"
interleaved inside role="listbox"), the committing clear entry, and the filtered
combobox path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 2 nitpicks · head bcd4c12f84
Crit approves — nothing blocking at this head.
2 nitpicks
GroupSelect and GroupCombobox ship with no entry in the ui-inputs docs page
docs/packages/ui-inputs.md:198 — The PR adds GroupSelect and GroupCombobox to packages/ui-inputs/src/index.ts, but docs/packages/ui-inputs.md is untouched. Its component table still lists only SingleSelect, Combobox, MultiSelect, and MultiCombobox from the select family. The line 'Two types complete the public surface' right after the table is now inaccurate. A consumer reading the canonical docs page has no way to discover the two new components or their props.
nitpick because pre-existing — this pull request did not write those lines
Browser-tests CI check status for the new grouped-listbox axe audits cannot be verified as green
packages/ui-inputs/tests/browser/axe.browser.spec.ts:1635 — The diff adds three new axe-core audits for GroupSelect and GroupCombobox in the serially-run browser spec, exercising a role=group containing a role=presentation list of role=option items. Whether this new ARIA nesting passes axe's aria-required-children/aria-required-owned checks cannot be determined without running the browser test suite. If the new markup violates those rules, the accessibility regression would ship undetected by this review.
nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: No Bash/execution tool is available to run npm run test:browser (or vitest run against packages/ui-inputs/vitest.browser.config.ts) in this worktree to capture the actual pass/fail result and any violating assertion.
Settled, not re-filed: 2
…in GroupOptionList html-aria disallows role="group" on <li> (axe aria-allowed-role). The APG grouped listbox pattern is preserved by giving the wrapper <li> role="presentation" and moving role="group" + aria-label to the inner <ul>, which explicitly allows role="group". AT sees through the presentation hole to the named group as before. Update GroupSelect.spec.ts to assert the corrected structure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR adds GroupSelect/GroupCombobox atoms with grouped-option rendering; headline concern is a new grouping bug where a header:false group placed after a named group gets silently absorbed into that group's run (wrong aria-label, wrong visual block).
Still open
Full detail on each linked thread, not re-pasted here.
- [medium] GroupOptionList forks OptionList markup instead of extending it —
packages/ui-inputs/src/components/GroupOptionList.vue:1-94— thread · change pushed but doesn't resolve — see reply
Resolved since last review: 2.
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 1 nitpick · head 2fd8c4234a
Crit requests changes — 1 open thread.
1 nitpick
GroupCombobox.spec.ts never exercises the required/invalid truthy branches, risking the 100% gate
packages/ui-inputs/tests/GroupCombobox.spec.ts — GroupCombobox.vue binds :aria-required, :aria-invalid, and the is-invalid class to the required and invalid props, but GroupCombobox.spec.ts never mounts the component with either prop set to true. packages/ui-inputs/vitest.config.ts enforces branches:100 over src/**/*.{ts,vue}, and the sibling GroupSelect.spec.ts already covers the same three props in one test. Without an equivalent test, npm run test:coverage is at risk of failing on these newly-added truthy branches.
nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Could not execute npm run test:coverage in this environment to confirm these specific branches are the ones that push ui-inputs coverage below the 100% threshold.
Still open
packages/ui-inputs/src/components/GroupOptionList.vue — already filed, still open
Settled, not re-filed: 2
Goosterhof
left a comment
There was a problem hiding this comment.
One blocker anchored inline, plus confirmation on the prior round's findings.
CI is red at this head (check/ci-passed fail the 100% coverage gate on packages/ui-inputs/src/**) — see the inline note on GroupOptionList.vue:52. The PR's own test-plan checkbox claiming 100% coverage doesn't hold at HEAD.
Confirming the prior review's open items: the header:false-after-a-named-group absorption bug (GroupOptionList.vue:116) is real, traced through GroupSelect.vue's rows computed, which emits no boundary marker for a headerless run. The reuse-fork observation (GroupOptionList.vue vs OptionList.vue) also stands — the two are near-identical outside the grouping logic.
The CSS and ARIA priors are fixed: styles.css now ships full .ui-groupselect/.ui-groupcombobox rules, and the group wrapper pairs role=group + aria-label with an aria-hidden visual header.
Requesting changes on the coverage gate; the absorption bug and the reuse-fork note stand as before.
A `header:false` group that followed a named group had its options absorbed into the prior group's role="group": `groupedRuns` kept appending to the open run because a headerless group emitted no row to break it. Add a `boundary` GroupRow marker, emitted by both row builders (GroupSelect `rows` / GroupCombobox `filteredRows`) ahead of a headerless group's options; `groupedRuns` closes the open run on it so those options render flat via the existing `!current` path. Regression tests added to both spec files; 100% coverage held. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
1 issue · 0 nitpicks · head a7c4804f26
Crit requests changes — 1 issue.
Issues
Document the exported grouped selects in the ui-inputs package guide
docs/packages/ui-inputs.md:198 — not on the diff, so not inline
The package barrel exports GroupSelect and GroupCombobox, but the guide lists only four select controls. The guide documents options as the required collection prop and never describes groups. Consumers cannot learn the grouped controls' required input shape from the published package documentation.
Settled, not re-filed: 1
…st forking OptionList GroupOptionList copied OptionList's option-row markup verbatim (role="option", aria-selected, is-active/is-muted chrome, hover/commit wiring, #option slot), diverging only in the group-run layout — so any future a11y/markup fix to an option had to land in two bodies. Extract that row into a new internal ListboxOption.vue and render it from both OptionList (flat) and GroupOptionList (grouped + headerless runs). The option markup now lives once; GroupOptionList owns only the role="group"/aria-label wrappers and the headerless-run layout. DOM is byte-identical; 100% coverage held (ListboxOption fully exercised via the existing suites). Addresses PR script-development#224 review finding (dmooibroek): GroupOptionList forks OptionList markup instead of extending it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
1 issue · 1 nitpick · head 2a84089748
Crit requests changes — 1 issue.
Issues
Public documentation omits the newly exported grouped select controls
packages/ui-inputs/README.md:24 — not on the diff, so not inline
The package exports GroupSelect and GroupCombobox. The README and package documentation catalog only the earlier select controls. Consumers cannot discover the grouped controls or their groups contract from published documentation.
1 nitpick
Published validator tests omit both grouped select components
packages/ui-inputs/tests/published-prop-validators.spec.ts:41 — The published validator cases exclude GroupSelect and GroupCombobox. Their component tests import source SFCs. A distribution-only LabelKey validator regression can reach grouped-control consumers without CI detecting it.
nitpick because code change required — the harm needs an edit that has not happened; pre-existing — this pull request did not write those lines
There was a problem hiding this comment.
Adds GroupSelect/GroupCombobox atoms with grouped-option rendering; both prior medium findings (absorption bug, forked markup) are fixed, but the fix itself reveals group-row derivation logic is now duplicated across the two components and should be extracted to the shared group-rows module.
Resolved since last review: 2.
…related components - Updated GroupSelect.vue to use OptionList for rendering options, improving consistency across components. - Modified MultiSelect, MultiCombobox, and SingleSelect to utilize rows instead of labels for options. - Enhanced OptionList to handle both flat and grouped options, collapsing group headers appropriately. - Adjusted tests for GroupSelect and GroupCombobox to reflect changes in option rendering and interaction. - Removed GroupOptionList component and updated related test utilities for clarity and maintainability.
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 1 nitpick · head 19d0210e3d
Crit approves — nothing blocking at this head.
1 nitpick
Published validator tests omit both grouped controls
packages/ui-inputs/tests/published-prop-validators.spec.ts:39 — The CASES array omits GroupSelect and GroupCombobox despite both declaring label: LabelKey<T>.
Only this suite imports the built package, as its documentation states.
CI would not detect a dist validator regression for either grouped control.
Consumers could receive prop-validation warnings.
nitpick because code change required — the harm needs an edit that has not happened
GroupSelect and GroupCombobox held the identical GroupRow-derivation loop (header/boundary + option-index pushing), so the header:false boundary fix had to land in both copies. Move the encoding to a single buildGroupRows() helper in src/internal/group-rows.ts and call it from both -- the GroupRow invariant is now single-site. The empty-group skip guard is a no-op on the GroupCombobox path (filteredData already drops empty groups). Behaviour and DOM unchanged; 100% coverage + browser suites held. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jasperboerhof
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 0 nitpicks · head 99b69d150d
Crit approves — nothing blocking at this head.
No issues or nitpicks.
Summary
GroupSelect(button trigger) andGroupCombobox(input trigger) atoms for grouped-option listboxes, mirroring the existingSingleSelect/ComboboxandMultiSelect/MultiComboboxsplitGroupRowunion type (header | option) so group headers interleave with navigable options in a single row sequence without occupying index spaceGroupOptionList.vue— internal listbox popup; headers carryrole=presentationso they are valid listbox children but skipped by the keyboard pathdefineExposefocus handle (WR-0448) onGroupComboboxTest plan
npm run test:coveragepasses with 100% coverage inpackages/ui-inputsGroupSelect: render/ARIA, open, group headers interleaved, select-on-click, clear entry, keyboard (ArrowDown/Enter/Escape/Home/End), aria-activedescendant, disabled, required/invalid/describedby, emptyText,header: false, mutedOptions,#optionslotGroupCombobox: render, open, filter narrowing with empty group hidden, WR-0576 browse-to-change, Escape revert, click-outside revert, select-all-on-open, keyboard commit, clear keyboard path, idle re-sync, async edit-form pattern, focus handle,#optionslotGenerated with Claude Code