Repository navigation
fix(ui-inputs): typeahead on select-only listboxes, attrs reach the combobox, alphabeticalSort off by default, IME guard (WR-1991, WR-1990) - #285
Conversation
`useListbox` had no character search, so a SingleSelect replacing a native <select> lost typing-to-jump (lokalekeuze LK-1434, a launch blocker). `useListbox` takes an optional `typeaheadLabels` getter (internal; the composable is not exported) and implements the WAI-ARIA listbox typeahead with native <select> parity: the typed string grows while keys arrive within 500 ms; a single or repeated character moves to the NEXT option starting with it, so repeating it cycles; a longer string matches by prefix from the current option, so a highlight that still matches stays put; the search wraps, compares case-insensitively, and moves nothing on no match. Space joins the string only once one is being typed, and is then prevented so the trigger is not clicked on keyup. Modified keys are not typeahead. Closing drops the string. Closed, a match opens the list on that option and commits nothing (the APG select-only combobox), never a native-style closed commit: that would toggle membership on a MultiSelect and commit on a stray keystroke. SingleSelect, MultiSelect and GroupSelect pass their rendered labels; the comboboxes do not, since their typed characters are the query (negative pin). Specs in happy-dom per control (34 cases) plus a real-keyboard arm in the browser suite. ui-inputs 0.15.0 -> 0.17.0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MHxNh28encKSSVY9rFM9u
…(WR-1991) Every select and combobox rendered a root <div> and inherited attrs onto it, so `aria-label`/`aria-labelledby` landed on the wrapper and could never name the element carrying role="combobox" (lokalekeuze LK-1434 had to rebuild the name in its wrapper). The site docs claimed attrs reach the native control. Fleet census before choosing (33 direct uses of the six components across 8 repos at their local HEADs): 4 pass a fall-through attr, all of them `class` (ublgenie editorial Select/Multiselect, styling the whole control). Moving everything to the control would restyle them, so the split is Vue's own: `class` and `style` stay on the root, every other attr goes to the combobox element (the trigger button, or the text input). The control's bind comes first, so a consumer attr never overrides the component's own id, role or ARIA wiring (pinned). Applied to all six members, which share the root shape. Spec per member: aria-label and data-* land on the control; class and style stay on the root (regression pin); id/role cannot be hijacked (pin). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MHxNh28encKSSVY9rFM9u
Commander ruling 2026-10-03: the caller's order is the order, and sorting is the opt-in. SingleSelect, MultiSelect, Combobox and MultiCombobox flip the default; GroupSelect/GroupCombobox have no sort and are unchanged. Fleet census of direct uses at the repos' local HEADs: 31 of the four sortable components across 8 repos; 25 pass `alphabetical-sort` explicitly, every one of them `false`; the 6 that relied on the old default are named in the CHANGELOG (entreezuil BaseSelect, brick-inventory-orchestrator x5) so each can opt back in. Spec per member: caller order by default, sorted with `alphabeticalSort`. The existing specs that index the sorted order now opt into it explicitly (happy-dom helpers, two browser fixtures, the shadow-root fixture). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MHxNh28encKSSVY9rFM9u
…ses (WR-1990) Both assigned the model on every `input`, so a consumer that swapped native `v-model` for them got every IME candidate (lokalekeuze: a GET per candidate). Vue's `vModelText` ignores `input` while composing and commits on `compositionend`; `commitOutsideComposition` does the same for a field the component binds by hand, and both controls use it. Grep of the family's input handlers: TextInput and Textarea share the shape. DateInput (a native date picker, no IME) and NumberInput (valueAsNumber coercion) do not, and the comboboxes' `onInput` drives the local query, not the model; those are named as residuals, not changed. Specs in happy-dom: neither happy-dom nor Chromium can be driven by a real IME from a test, so both would run the same dispatched composition events, and the rule is the component's own logic. RED on base: start, four candidate inputs, end -> one emit of the committed value; pins for plain typing and for typing after a composition. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MHxNh28encKSSVY9rFM9u
Deploying fs-packages with
|
| Latest commit: |
37c7879
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d7222df4.fs-packages.pages.dev |
| Branch Preview URL: | https://fix-wr-1991-single-select-ty.fs-packages.pages.dev |
|
Crit reviewt deze pull request. Crit is een automatische reviewer. Zijn review staat als aparte review op deze pull request, onder dit bericht of, bij een mislukte ronde, na de volgende poging.
Een review lezenEén review per ronde. Bovenaan de telling en het oordeel. Daaronder de issues, dan de nitpicks ingeklapt.
Issue of nitpickHet verschil is niet ernst. Elke vondst draagt drie tags. Geldt één nitpick-waarde, dan is het een nitpick; anders een issue. Een vaste regel kiest, geen agent.
Wat doe je met een vondstVoor een issue werken drie dingen, zolang je het zegt.
Wat niet werkt: een thread zelf resolven zonder fix of antwoord. "Werkt bij mij" en "fixen we later" tellen niet.
Veelgestelde vragenEen runtime-issue komt bij ons bijna nooit voor. Mag ik hem laten staan?Ja. Schrijf in de thread dat je het risico accepteert en waarom; crit sluit de thread. Let op: een retry, een cron-overlap of een dubbele webhook gebeurt ook bij één gebruiker. Crit vond iets in code die ik niet heb aangeraakt. Waarom staat dat op mijn pull request?Crit zet een vondst waar het probleem woont. Breekt jouw wijziging die code, dan is het Wat betekent "proof gap" en wat moet ik ermee?Crit zag een echt mechanisme maar kon één stap niet bewijzen; de proof gap zegt welke. Antwoord in de thread of dat pad bestaat, en fix het als dat zo is. Ik heb de thread geresolved op GitHub, maar crit komt er toch weer mee. Waarom?Crit kijkt naar de code, niet naar de knop. Staat het probleem op de nieuwe head nog, dan post crit het opnieuw; fix het of schrijf in de thread waarom je het laat staan. Wat moet ik precies in een thread schrijven om iets te laten staan?Noem het gedrag dat je laat staan en wie het oppakt; een Kendo-report zonder ticketnummer telt ook. Crit zoekt het report nooit op en leest alleen wat jij in de thread schrijft. Ik heb gefixt en gepusht, maar crit ziet mijn reactie niet. Wat ging er mis?Crit leest de threads vlak na een push, dus een reactie van daarna mist die ronde. Eerst reageren, dan pushen; de fix zelf ziet crit altijd. Mag ik alles in de finder trace vertrouwen?De paden en regels wel: crit liep ze na voordat de issue werd gepost. De zinnen eromheen niet altijd. De vetgedrukte kop en de alinea eronder zijn de geverifieerde tekst; waar de trace daarvan afwijkt, wint de alinea. Een absolute bijzin in de trace ("only called by tests") is een claim van de zoeker, geen oordeel. Ik laat een agent de fix maken. Wat geef ik hem?De hele comment, inclusief de trace. De trace kan verdere sites noemen; laat hem elke site in de repo controleren voordat hij fixt. Een issue die "three routes" zegt en er één verankert, noemt de andere twee in de trace. Een pad onder node_modules of vendor is leesspoor, geen site. Moet ik op nitpicks reageren?Nee. Laten liggen is een legitieme keuze: een nitpick heeft geen thread en blokkeert niet. Zolang de code hetzelfde blijft, kan hij in een volgende ronde opnieuw in de ingeklapte lijst staan. Waarom zegt crit "request changes" terwijl er geen nieuwe issues zijn?Een eerdere thread waarvan het probleem nog staat, blokkeert ook. Kijk onder Still open: daar staat de thread met wat er nog ontbreekt. Kan crit per repo minder streng?Ja, met twee schakelaars per repo: approve en request changes. Staat request changes uit, dan wordt het oordeel comment en blokkeert de pull request nooit. |
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
2 issues · 0 nitpicks · head 2281663fa4
Crit requests changes — 2 issues.
Issues
An update during IME composition can overwrite the candidate text in TextInput and Textarea
packages/ui-inputs/src/internal/composition.ts:20 — see inline
Closed SingleSelect typeahead ignores the selected option when choosing where to search
packages/ui-inputs/src/composables/useListbox.ts:171 — see inline
…ches from the committed option (WR-1990/WR-1991 round 1) crit bc3052a334fb: TextInput and Textarea bound `:value` to the model, which stays stale until compositionend, and Vue re-patches `value` on every render. A prop change mid-composition therefore restored the old text and wiped the candidate. RED on 2281663 for both controls: `expected 'a' to be 'aか'`. `commitOutsideComposition` now keeps a draft of the field's own text while composing, and the controls bind `:value` to `shown` (draft ?? model). That is the job vModelText's `if (el.composing) return` does on update. crit c4bc09617301: closing resets the pointer to -1, so a closed-control search started at index 0. With Banana chosen, `b` re-highlighted Banana. RED on 2281663 for SingleSelect and GroupSelect: `expected 'f-opt-2' to be 'f-opt-3'`. `useListbox` takes an internal `committedIndex`; with nothing highlighted, the search starts from it, as a native <select> searches from its value. MultiSelect has no single committed option and searches from the top (pinned). The last-toggled member is not visible on a closed control, and an invisible anchor is worse than none. Mutants: binding the model (the reviewed shape) and a frozen draft each red the re-render case; ignoring the committed index reds both Banana cases; starting AT the committed option reds them plus the cycling case. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
2 issues · 1 nitpick · head 8276c7474c
Crit requests changes — 2 issues.
Issues
Click dismissal preserves stale typeahead state in SingleSelect and MultiSelect
packages/ui-inputs/src/components/SingleSelect.vue:231 — not on the diff, so not inline
Both toggle handlers flip open.value directly, bypassing close() and preserving typed and pointer. After a click close and reopen within 500 ms, a extends an earlier b to ba. SingleSelect can retain Banana and commit it when the user intended Apricot.
Sites
packages/ui-inputs/src/components/SingleSelect.vue:231-233
packages/ui-inputs/src/components/MultiSelect.vue:249-251
Finder trace · correctness lane · not verified by the judge beyond the text above
Claim. Clicking SingleSelect closed and reopened within 500 ms preserves the previous typeahead string, so the next key can highlight and commit the wrong option.
Consequence. After typing b to highlight Banana, a user can click to close, click to reopen, then type a; the stale b makes the control search for ba and retain Banana instead of starting a fresh search for Apricot. Pressing Enter commits the wrong value. MultiSelect has the same toggle path.
Evidence. useListbox.ts:140-145 stores the typed string and clears it only in dropTyped; close() calls that cleanup at :147-151. SingleSelect's click handler calls toggle (SingleSelect.vue:19), whose implementation at :231-233 changes open.value directly without calling close(). MultiSelect likewise toggles open.value directly (MultiSelect.vue:249-251). Thus a click-close leaves both the buffer and pointer intact. With Banana highlighted by b, close and reopen by click, then press a before the timer expires: typeahead appends it to b and searches for ba from the existing pointer (useListbox.ts:171-185), retaining Banana. GroupSelect's toggle calls close() when closing, so it clears this state.
Precedent. packages/ui-inputs/src/components/GroupSelect.vue:231 uses close() when the trigger closes, clearing the typeahead buffer and highlight.
harm_requires: runtime_state · provenance: adjacent · confidence: confirmed
Sustained key repeat makes typeahead scan an unbounded buffer on every key
packages/ui-inputs/src/composables/useListbox.ts:173 — see inline
1 nitpick
Attribute routing tests leave consumer focus, blur, and keydown listeners untested
packages/ui-inputs/tests/attrs-to-control.spec.ts:45 — The attribute suite passes only static attributes and never dispatches an event to a consumer listener. A routing change that drops focus, blur, or keydown listeners could pass these assertions. Consumers would lose events the changelog promises on the control.
Sites
packages/ui-inputs/tests/attrs-to-control.spec.ts:31-76
nitpick because code change required — the harm needs an edit that has not happened
…lect-typeahead Absorbs #282 (ui-inputs 0.16.0). Conflicts resolved: package.json and the lockfile's ui-inputs workspace entry take 0.17.0; CHANGELOG keeps both entries verbatim, 0.17.0 above 0.16.0. No other change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…WR-1991 round 2) crit 2b1c77d89279: key repeat (~30 Hz) never lets the 500 ms idle reset fire, so a held key grows the typed string without limit, and each key re-scanned the whole string to decide whether it was one repeated character. Measured on 0814456 in happy-dom: the last 1,000 of 17,000 held repeats took 9x (bare) and 14x (after "b") as long as the first 1,000. Invariant: typeahead work per key is bounded independently of how long a key is held. Mechanism: whether the string is one repeated character is kept as a flag. It is set when a new string starts and cleared by any other character, instead of being re-derived by a scan. Matching is unchanged: the needle and the search start read exactly as before, and every prefix-match and cycling spec stays green. A length cap was considered and not taken; with the scan gone, no per-key step reads the whole string, and a cap would be a second rule to keep equivalent to the first. Specs: held key bare and after another character. Each times the last block of repeats against the first in the same run (ratio < 4) and is RED on 0814456. Plus a case for a repeated character cycling again after a string resets, which kills the mutant whose flag never re-arms. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
2 issues · 2 nitpicks · head d626b4bb7a
Crit requests changes — 2 issues.
Issues
Trigger closing still preserves the typeahead prefix in SingleSelect and MultiSelect
packages/ui-inputs/src/composables/useListbox.ts:176 — see inline
Continuous typeahead input grows the typed buffer without a limit
packages/ui-inputs/src/composables/useListbox.ts:180 — see inline
2 nitpicks
Attribute tests leave consumer listener delivery and keydown order unchecked
packages/ui-inputs/tests/attrs-to-control.spec.ts:45 — The attribute suite never dispatches focus, blur, or keydown for any of the six controls. A later listener routing regression would pass these tests. Consumers could then lose the documented hooks or handler order.
Sites
packages/ui-inputs/tests/attrs-to-control.spec.ts:31-76
nitpick because code change required — the harm needs an edit that has not happened
A trailing input event can emit the composed value twice
packages/ui-inputs/src/internal/composition.ts:23 — The compositionend handler commits the field value and clears draft. A following input with the same value commits again because onInput sees a null draft. Consumers responding to model updates can process the completed value twice.
Sites
packages/ui-inputs/src/internal/composition.ts:23-30
nitpick because unconfirmed — proof gap: Verify whether a supported browser with a real IME sends an input event carrying the completed value after compositionend.
… state (WR-1991) useListbox now returns `open` read-only plus `openList()` / `close()` / `toggle()`. The six listbox components route every open, close and toggle through them, so the transition to closed resets the highlight, the typed string, its timer and the repetition flag in exactly one place (`close()`). SingleSelect and MultiSelect toggled `open` directly, which left the typed string alive after a trigger click closed the list. A source guard asserts no component writes `open.value` and that it scanned the six listbox hosts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… string (WR-1991) Open, type a prefix, close via the trigger, reopen inside the 500 ms idle window, type again: the search starts fresh. RED on d626b4b for SingleSelect and MultiSelect (the stale "ba" stayed on Banana); GroupSelect is a regression pin, green at base because its toggle already closed through close(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…WR-1991) Key repeat never lets the 500 ms idle reset fire, so a held key grew the typed string without limit. It now keeps its first 64 characters: the prefix is what matches, and a sliding tail would match from the middle of a word. CHANGELOG: the 0.17.0 typeahead entry names the cap and the close-drops-string rule, and a Changed entry records that a trigger close on SingleSelect and MultiSelect now clears the highlight (shipped in 0.16.0; changed by the one-owner refactor in the previous commit). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
1 issue · 2 nitpicks · head 020c9575e4
Crit requests changes — 1 issue.
Issues
Typeahead misses Greek labels when separate keystrokes fold differently from the label.
packages/ui-inputs/src/composables/useListbox.ts:193 — see inline
2 nitpicks
A trailing input event can emit the IME commit a second time after compositionend.
packages/ui-inputs/src/internal/composition.ts:29 — onCompositionend clears draft and commits the field value. onInput commits again when a later event sees draft as null. Consumers receive two updates for one IME selection if a browser delivers that trailing event.
Sites
packages/ui-inputs/src/internal/composition.ts:23-30
packages/ui-inputs/src/components/TextInput.vue:13-15
packages/ui-inputs/src/components/Textarea.vue:13-15
nitpick because unconfirmed — proof gap: A supported browser's IME event sequence must establish whether a final input event follows compositionend.
Unmatched typeahead keys synchronously scan every option label without a work limit.
packages/ui-inputs/src/composables/useListbox.ts:200 — The select components pass their full option lists to typeahead. An unmatched key makes typeahead lowercase every label synchronously. A consumer with a very large list can stall its page during that scan.
Sites
packages/ui-inputs/src/composables/useListbox.ts:200-207
packages/ui-inputs/src/components/SingleSelect.vue:226
packages/ui-inputs/src/components/MultiSelect.vue:246
packages/ui-inputs/src/components/GroupSelect.vue:225
nitpick because unconfirmed — proof gap: A consumer binding attacker-controlled options is needed to establish the claimed untrusted-input path.
…ime (WR-1991) crit b8d6a958f182: keys were lowercased one by one but labels as a whole, and a whole-string lowercase is contextual (Σ ending a word becomes ς), so typed ΟΣ never matched the label ΟΣ. Both sides now go through the same per-code-point fold. Lowercasing the whole typed buffer instead would break the prefix case (ΟΣ → ος no longer prefixes ΟΣΑ → οσα); a spec pins it. The repeat-to-cycle check compared the folded key with typed[0], one code unit, so a key whose fold is two units (İ → i̇) never cycled and matched plain-i labels. The first folded key is now kept whole. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kqhe46TCCff7tLC8aWguGd
…1991) crit 14846337b68b: the typeahead predicate counted UTF-16 code units, so an emoji key (two units) never searched. It now counts code points, as the fold does; named keys such as Enter stay excluded. crit 46a7ba313237: every key re-folded every label. The folded labels are now a computed, refolded only when the labels change (GroupSelect built a fresh array per key, so identity alone would not have held). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kqhe46TCCff7tLC8aWguGd
…etween it and typeahead Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kqhe46TCCff7tLC8aWguGd
…ener forwarding pinned (WR-1990, WR-1991) crit f8549b3e4dd2: TextInput/Textarea now end a live composition on `change` as Vue's vModelText does (it binds change to onCompositionEnd); outside a composition `change` does nothing, also as vModelText. crit 1d595df16db0: the 64 cap counted UTF-16 units, so the 33rd emoji key was dropped. It now counts keys with a counter (no rescan per key). crit 9d6a6693754f: a spec per select-family host proves a consumer listener passed as an attr fires on the role=combobox element. crit 30ba9376644d (AltGr/Option) is not built: measured, no clean predicate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kqhe46TCCff7tLC8aWguGd
|
Deferred, not built, from a private review round on |
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
1 issue · 0 nitpicks · head dc7cf20b9d
Crit requests changes — 1 issue.
Issues
Select typeahead folds and scans unbounded option lists on the browser main thread
packages/ui-inputs/src/composables/useListbox.ts:194 — see inline
… (WR-2035), D2 AltGr/Option (WR-2031) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
crit-ai
left a comment
There was a problem hiding this comment.
Crit review
0 issues · 0 nitpicks · head 37c78795fa
Crit approves — nothing blocking at this head.
No issues or nitpicks.
Settled, not re-filed: 6
What
ui-inputs
0.17.0: four fixes, one commit each. Merge after #282 (0.16.0). This branch is cut frommain, so once #282 lands the General merges it forward. Expect conflicts in the CHANGELOG,package.json, the lockfile version line, the README, the site docs andinteraction.browser.spec.ts.WR-1991: SingleSelect drop-in gaps (seed: lokalekeuze LK-1434)
SingleSelect,MultiSelect,GroupSelect). Typeahead is a lokalekeuze launch blocker (Commander ruling 2026-10-03).MultiSelectfrom a stray key.useListboxisn't exported, so this adds no public API.classandstylestay on the root; every other attribute goes to the element withrole="combobox". A consumer can therefore setaria-label/aria-labelledby. The consumer's attributes bind before the component's own, so a consumer can't overrideid,roleor ARIA.class(ublgenie), and they keep working. lokalekeuze'sSelectFieldpassesclasstwice; it keeps working too, and can drop its hidden<label for>workaround.@focus/@blurnow fire, and a consumer@keydownruns on the control.placeholder12/33,emptyText19/33 andoptionsLabel9/33.optionsLabelis an accessible name, so those 9 announce "Options" in English. This goes to a separate ruling; the Medic's recommendation is a dev-only warning plus derivingoptionsLabelfrom the field label.alphabeticalSortdefaults tofalse(Commander ruling 2026-10-03), so the caller's order is kept. Of 31 uses, 25 already passfalse. The 6 that relied on the old default are named in the CHANGELOG (entreezuilBaseSelect; 5 files in brick-inventory-orchestrator), so each can opt back in.WR-1990: IME composition guard
TextInputandTextarea(the only components sharing that input handler) no longer emit while an IME is composing; they emit the committed value oncompositionend. That matches Vue'svModelText. Seed: lokalekeuze fired one GET per IME candidate.Verification
test:coverage1473 at 100 %;test:browser137 ui-inputs + 6 dialog.Residuals (tracked)
preventDefaultwithoutstopPropagation; that's a family-wide decision.Execution report (war room):
reports/fs-packages/execution/2026-10-03-medic-ui-inputs-single-select-typeahead-and-ime.mdpackages/ui-inputs/DECISIONS.mdD1, WR-2035): select typeahead folds every option label once per options change and prefix-scans the folded labels on each printable key — O(n) on the main thread, with no large-list budget. It adds no new cost class next to the existing per-change label map,localeComparesort and full open render.isTypeaheadrefusesctrlKey/altKey).🤖 Generated with Claude Code
https://claude.ai/code/session_016MHxNh28encKSSVY9rFM9u