Type characters in limel-select to jump to a matching option - #4193
Type characters in limel-select to jump to a matching option#4193TommyLindh2 wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesSelect typeahead
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant limel-select
participant TypeaheadBuffer
participant findTypeaheadMatch
participant limel-list
Keyboard->>limel-select: keydown character
limel-select->>TypeaheadBuffer: append character
limel-select->>findTypeaheadMatch: match buffered candidates
findTypeaheadMatch-->>limel-select: return row index
limel-select->>limel-list: focus and scroll matching row
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4193/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example-tests/components/select.spec.ts`:
- Around line 31-35: Update openByTyping so it presses the complete character
sequence before waiting for surface(page) visibility, avoiding an assertion or
other wait between characters that can exceed TYPEAHEAD_BUFFER_TIMEOUT. Press
the initial character through trigger(page), send remaining characters
immediately via page.keyboard, then assert the surface is visible once after the
sequence; preserve the existing trigger visibility check.
In `@src/components/select/select.e2e.tsx`:
- Around line 512-529: Update the pressKey helper to create a cancelable
KeyboardEvent and return the dispatched event so callers can inspect
defaultPrevented. Preserve the existing key, bubbling, composition, and modifier
behavior while enabling tests of handleTypeaheadKey’s preventDefault handling.
In `@src/util/keycodes.ts`:
- Around line 6-9: Update the keyboard check in the input-field handler around
the event comparison at `event.key === SPACE` to compare `event.code` with
`SPACE`, preserving the existing action behavior when the space bar is pressed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cda9bde7-6105-41d9-a5fc-f37f8fb90277
📒 Files selected for processing (8)
example-tests/components/select.spec.tssrc/components/list/list.tsxsrc/components/select/select.e2e.tsxsrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/keycodes.tssrc/util/typeahead.spec.tssrc/util/typeahead.ts
| /** `KeyboardEvent.code` for the space bar. NOT a `KeyboardEvent.key`. */ | ||
| export const SPACE = 'Space'; | ||
| /** `KeyboardEvent.key` for the space bar. */ | ||
| export const SPACEBAR = ' '; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every usage of the SPACE and SPACEBAR constants with context.
rg -nP --type=ts --type=tsx -C4 '\bSPACE(BAR)?\b' src example-tests 2>/dev/null \
|| rg -nP -g '*.ts' -g '*.tsx' -C4 '\bSPACE(BAR)?\b' .Repository: Lundalogik/lime-elements
Length of output: 4116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant key handling context and broader SPACE exports/usages.
sed -n '1,30p' src/util/keycodes.ts
sed -n '1020,1042p' src/components/input-field/input-field.tsx
sed -n '15,25p' src/components/input-field/input-field.tsx
printf '\nAll tracked text usages of SPACE and SPACEBAR identifiers:\n'
git ls-files '*.{ts,tsx}' | xargs rg -H -n -C2 '\bSPACE(BAR)?\b' || true
printf '\nAll tracked text usages of KeyboardEvent.code comparisons to space-like values:\n'
rg -n -C2 '\.code\s*(===|===|==)\s*(\'"'"'Space\'"'"'|SPACE|SPACEBAR)' -g '*.ts' -g '*.tsx' . || trueRepository: Lundalogik/lime-elements
Length of output: 6134
Compare SPACE against KeyboardEvent.code, not event.key.
src/components/input-field/input-field.tsx:1033 compares event.key === SPACE, but SPACE is 'Space', so pressing space does not enable the action. Use event.code === SPACE, or replace it with the current string literal if this handler should still accept other space key forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/keycodes.ts` around lines 6 - 9, Update the keyboard check in the
input-field handler around the event comparison at `event.key === SPACE` to
compare `event.code` with `SPACE`, preserving the existing action behavior when
the space bar is pressed.
There was a problem hiding this comment.
The bug is real — it is called out in the PR description — but I am deliberately
not fixing it in this PR.
It is also worse than described. The handler is bound to an
<i role="button" tabIndex={0}>, not a native <button>
(input-field.tsx:796-807), so there is no synthesized click to fall back on.
The trailing action icon is focusable and announced as a button, yet the space
bar does nothing at all. That is an ARIA violation, not merely dead code — and
the handler is named handleIconKeyPress while being bound to onKeyDown, the
same staleness this PR cleans up in limel-select.
Why it stays out of this PR, per commits-and-prs.md:
- "One PR = one concern" and "Keep the diff focused — sneaking an unrelated
refactor into a bug-fix PR is how regressions slip through." - It is a behavior change in a different component: it enables an activation
path that has never worked. That needs its own tests, which do not exist for
this handler today. - It earns its own
fix(input-field)changelog entry. Folding it into
fix(select): jump to the option matching typed characterswould hide it from
consumers, since our changelog is generated from commit subjects.
So it deserves its own issue and PR rather than a quiet ride along with this one.
Only keycodes.ts is touched here, and only to add SPACEBAR = ' ' next to the
existing SPACE = 'Space' — SPACE itself is left alone precisely so this
PR does not change limel-input-field behavior as a side effect.
Typing characters now moves the highlight to the option whose text starts with them, the way a native `<select>` does. Characters accumulate for a second, so typing `n`, `e` reaches "Netherlands" rather than the next option starting with `n`, and repeating a character cycles through every option starting with it. This works both on the closed component, which opens the dropdown with the match highlighted, and in the open dropdown, where reaching an option previously took one arrow key press per row. Typing only moves the highlight. The value is still changed by `Enter` or a click, so consumers do not see a `change` event per keystroke, and `multiple` behaves the same as single select. Characters are intercepted in the capture phase on `limel-list`, because `MDCList` listens for `keydown` on the `ul` inside that element's shadow root. Anything reaching it is added to MDC's own typeahead buffer, which suppresses selection with `Enter` for as long as the buffer lives, and a space is treated as a selection. Also move the trigger off the deprecated `keypress` event to `keydown`, and compare the space bar against `event.key` instead of the `SPACE` constant, which holds a `KeyboardEvent.code` and so never matched. Opening on the space bar previously only worked through the button's synthesized click. Closes #4192
`mdcList.hasTypeahead = true` has never done anything, because MDC indexes each row by looking for `.mdc-deprecated-list-item__primary-text`, while `limel-list-item` renders its label as `<span class="label">`, leaving MDC's match index empty. Keep the assignment as a marker rather than removing it, and record both why it is inert and why reviving it is not wanted, so that the next reader neither trusts it nor "fixes" it.
6d0986d to
5c0d5b3
Compare
Type the whole sequence through one `pressSequentially`, so no round trip to the browser sits between two characters. An awaited assertion between them could outlast `TYPEAHEAD_BUFFER_TIMEOUT` on a loaded runner, and the characters would stop being treated as one word. Typing that fast uncovered an ordering bug. Opening the dropdown queues a `setMenuFocus` that waits for it to become visible. A second character arriving before that resolves would find its own match and focus it right away, leaving the first character's index recorded as pending — so the queued focus then applied the older match on top of the newer one, and `l`, `e` landed on "Luke Skywalker" instead of "Leia Organo". Record the index even when the row is focused synchronously, so the queued focus is idempotent. Also make the dismissal test discriminating: it pressed the same character twice, which lands on the first match whether or not the buffer was discarded. A two-character buffer cannot match anything once "l" is appended to it, so the dropdown would not reopen at all if the buffer had survived.
Dispatch the synthetic key events as `cancelable`, the way a browser does. Without it `preventDefault` is a no-op and `defaultPrevented` stays `false`, so a regression that stopped suppressing consumed characters would go unnoticed. Return the event from the helper and assert both directions: a consumed character is suppressed, and a key the typeahead ignores is left alone.
1 participant
Summary by CodeRabbit
New Features
Tests
Closes #4192
Why
Reaching an option in
limel-selectfrom the keyboard meant pressing ↓once per row — around 30 presses to get to "Wisconsin" in the states example,
where a native
<select>takes two. This adds the type-to-jump behavior anative
<select>has, without adding any public API.What changed for consumers
Typing characters moves the highlight to the option whose text starts with them:
n,ereaches "Netherlands" rather than thenext option starting with
n.the end.
highlighted) and in the open dropdown.
change. It moves the highlight only; the value isstill set by Enter or a click, so
multiplebehaves the same assingle select.
containing spaces is reachable ("New York").
No new props, events, or exported types —
src/util/typeahead.tsis internal,so this is a
fixrather than afeat.Worth a reviewer's attention
The capture-phase listener is load-bearing, not stylistic.
MDCListlistensfor
keydownon theulinsidelimel-list's shadow root. Any character thatreaches it lands in MDC's own typeahead buffer, which guards
notifyActionbehind
isTypeaheadInProgress()— so typing and then immediately pressingEnter would select nothing — and MDC treats a space as a selection.
So
select.tsxintercepts on the capture phase of thelimel-listelement andstops propagation for every character it consumes, including non-matching ones.
Same pattern as
handleListKeyDownCaptureinmenu.tsx.Why not just fix MDC's built-in typeahead.
limel-listalready setshasTypeahead = true, but it is inert: MDC looks for.mdc-deprecated-list-item__primary-textwhilelimel-list-itemrenders<span class="label">. Reviving it would have been a one-line change, and wasrejected because three of its behaviors are not configurable — a hard-coded
300ms buffer (which defeats multi-character matching), Enter ignored
while that buffer lives, and the space bar excluded from matches — and because it
would also enable typeahead in
limel-menu-list, wherelimel-menutreatssingle characters as item hotkeys. The second commit documents this on the
hasTypeaheadline so it does not get "fixed" later.Index alignment.
data-indexcounts separators, MDC's internal indices donot. The typeahead derives its candidates from
createMenuItems— the same arraythe template renders — with separators mapped to
nullso they still occupy anindex.
example-tests/components/select.spec.tscovers this end to end via thestates example.
Two drive-by fixes that the typeahead required, both in the first commit
because they are not separately revertable: the trigger moves off the deprecated
keypressevent tokeydown, and the space bar is compared againstevent.keyinstead of the
SPACEconstant, which holds aKeyboardEvent.codeand so nevermatched. Opening on the space bar previously only worked through the button's
synthesized click. Note the same latent
SPACEcomparison still exists ininput-field.tsx— left alone as out of scope.Pre-existing a11y bug found but not fixed: the trigger renders
aria-expandedas a boolean attribute —""when open, absent when closed —instead of the literal
"true"/"false", so assistive tech never hears"collapsed". Out of scope here since fixing it touches the axe baseline and
visual snapshots; happy to open a separate issue.
Verification
npm run lint,npm run build— cleannpm test— 1938 passed, 8 skippednpm run test:examples:components— 10 passed, stable across repeatedparallel runs
(
src/util/typeahead.spec.ts), 6 inselect.e2e.tsx, and 9 Playwright exampletests driving real key events and real focus
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Verification was automated only — Chromium via Vitest browser mode and
Playwright, on macOS. Manual checks in Firefox and Safari would be welcome,
particularly the focus and
scrollIntoViewbehavior inside the portal.Windows:
Linux:
macOS:
Mobile:
Mobile renders the native
<select>, which has its own typeahead, so the newcode path is a deliberate no-op there.