Skip to content

UI x-radio: fix keyboard navigation when value of first/last radio option is null#4308

Merged
calebporzio merged 3 commits into
alpinejs:mainfrom
gdebrauwer:feature/ui-radio-keyboard-navigation-fix
Feb 11, 2026
Merged

UI x-radio: fix keyboard navigation when value of first/last radio option is null#4308
calebporzio merged 3 commits into
alpinejs:mainfrom
gdebrauwer:feature/ui-radio-keyboard-navigation-fix

Conversation

@gdebrauwer

Copy link
Copy Markdown
Contributor

When the first or last x-radio:option has null as value, then keyboard navigation does not work correctly. It skips that option because it thinks that is already reached the end or beginning of the options array (caused by next || all[0] or prev || all.slice(-1)[0]). See screen recordings as a visual example.

Untitled2.mov
Untitled.mov

@ekwoka ekwoka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting case!

Looks like most of this needs to be refactored...

Comment thread packages/ui/src/radio.js Outdated
Comment on lines +109 to +110
let index = all.indexOf(option)
let next = all.length > index + 1 ? all[index + 1] : all[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let index = all.indexOf(option)
let next = all.length > index + 1 ? all[index + 1] : all[0]
let index = all.indexOf(option)
let next = all[(index + 1 + all.length) % all.length]

Comment thread packages/ui/src/radio.js Outdated
Comment on lines +118 to +119
let index = all.indexOf(option)
let prev = index >= 1 ? all[all.indexOf(option) - 1] : all.slice(-1)[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let index = all.indexOf(option)
let prev = index >= 1 ? all[all.indexOf(option) - 1] : all.slice(-1)[0]
let index = all.indexOf(option)
let prev = all[(index - 1 + all.length) % all.length]

@gdebrauwer
gdebrauwer requested a review from ekwoka July 19, 2024 14:14
@calebporzio

Copy link
Copy Markdown
Collaborator

PR Review: #4308 — UI x-radio: fix keyboard navigation when value of first/last radio option is null

Type: Bug fix
Verdict: Merge

What's happening (plain English)

  1. You have an x-radio group where one of the options has null as its value (e.g. a "None" option)
  2. User navigates with arrow keys. When keyboard nav lands on the null-valued option, the old code does next = next || all[0] — but null is falsy, so the || fallback fires
  3. Instead of landing on the null option, it skips straight to the first/last option — broken wrapping behavior
  4. There's actually a second bug the PR silently fixes: __focusOptionNext was using this.__optionValues.indexOf(option) to compute the index but then looking up in all (which filters out disabled options). These are different arrays, so the index can be wrong when disabled options exist before the active one. The PR fixes this to all.indexOf(option) for both directions

Other approaches considered

  1. Use undefined check instead of ||: Could do next = next !== undefined ? next : all[0]. Fixes the null issue but leaves the wrong-array-indexing bug in place. Worse.
  2. Use ?? (nullish coalescing): next ?? all[0] fixes undefined fallback but still treats the valid null value as "not found." Same underlying problem.
  3. Modular arithmetic (what the PR does): all[(index + 1 + all.length) % all.length] — handles wrapping in a single expression, uses the correct array for indexing, and doesn't care about falsy values at all. This is the cleanest approach.

Changes Made

No changes made — the PR is clean as-is.

Test Results

  • With fix reverted (original code): 2 of 11 tests FAIL (the two new null-value tests)
  • With fix applied: All 11 tests PASS
  • Regression verified empirically — the tests genuinely target the fix

Code Review

  • packages/ui/src/radio.js:109-110 — modular arithmetic for next. Clean, correct.
  • packages/ui/src/radio.js:118-119 — same pattern for prev. Consistent.
  • Tests at tests/cypress/integration/plugins/ui/radio.spec.js:256-297 — two tests cover null-as-first and null-as-last. Both exercise full wrap-around in both directions. Minimal, focused, good documentation of the bug.
  • Style matches Alpine conventions: no semicolons, let throughout.
  • No built assets in the diff.

Security

No security concerns identified.

Verdict

Merge this. It's a clean, surgical two-line fix (per function) that replaces a broken falsy-fallback pattern with correct modular arithmetic. The tests are well-written and empirically verified. The PR also incidentally fixes a separate indexing bug in __focusOptionNext where the wrong array was used for indexOf. No regressions, no scope creep.


Reviewed by Claude

@calebporzio
calebporzio merged commit 6e57d82 into alpinejs:main Feb 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants