[combobox] Keep native Home/End behavior when the key targets a text field

[sai6855]

fix #5480

[sai6855]

@claude review

[github-actions]

The review finished without producing a report.

---

🤖 Review generated with Claude Code · Opus 5 (High) · medium review depth · run

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@5539
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@5539
  

commit: 7228c0c

[sai6855]

@claude review

[github-actions]

PR review

The one-line runtime change is sound and well targeted: when a Combobox.Input lives inside the popup, useListNavigation's reference is the Combobox.Trigger (a button), so typeableComboboxReference was false and Home/End were being preventDefaulted for list navigation instead of reaching the focused text input. Keying off the event target instead fixes that, and it matches how the standard input-as-trigger combobox already behaves. Nothing here is merge-blocking. The findings are about test coverage that doesn't pin the actual behavior change, and a couple of small cleanups.

Tests (2)

1. 🟠 No test covers unmodified Home/End — the behavior the fix actually changes

Location: packages/react/src/combobox/input/ComboboxInput.test.tsx:563

if (!typeableComboboxReference && !isTypeableElement(getTarget(event.nativeEvent))) {
  if (event.key === 'Home') { ... }

The implementation does not look at event.shiftKey / event.ctrlKey at all — it disables list navigation for Home/End on any typeable target. That means plain Home and plain End inside a popup input also stopped moving the highlight and now move the caret. That is the larger, more user-visible half of the change, and every added combobox test uses a modifier ({Shift>}{Home}, {Control>}{Home}, {Shift>}{End}).

Failure scenario: A future contributor "narrows" the fix to if (!event.shiftKey && !event.ctrlKey && !event.metaKey) — reading the PR title and the test names as the spec. Every test in this PR still passes, while plain Home/End in a popup combobox input silently goes back to jumping the highlight instead of moving the caret.

Fix: Add a JSDOM test asserting plain {Home} inside the popup leaves the highlighted item unchanged, and a it.skipIf(isJSDOM) companion asserting plain {Home}/{End} collapse the caret to 0 / value.length in the popup input (mirroring the existing pressing Home moves caret to start tests at ComboboxInput.test.tsx:524 for the input-as-trigger case).

2. 🟡 Menu test name promises "modifiers" but only exercises Shift

Location: packages/react/src/menu/root/MenuRoot.test.tsx:253

it('still changes the highlighted item when Home and End are pressed with modifiers', async () => {
  ...
  await userEvent.keyboard('{Shift>}{End}{/Shift}');

The guard is valuable — it would fail if someone implemented this fix by checking event.shiftKey — but the plural name overstates what it asserts, and the body is otherwise a copy of changes the highlighted item using the Home and End keys directly above it (MenuRoot.test.tsx:226).

Failure scenario: A later event.ctrlKey/event.metaKey gate is added to commonOnKeyDown; the test named "with modifiers" passes, and Ctrl+Home silently stops moving the menu highlight.

Fix: Either add a {Control>}{Home}{/Control} assertion to match the name, or rename to ...when Home and End are pressed with Shift.

Simplifications (1)

1. 🟡 isTypeableElement runs on every keydown, not just Home/End

Location: packages/react/src/floating-ui-react/hooks/useListNavigation.ts:576

if (!typeableComboboxReference && !isTypeableElement(getTarget(event.nativeEvent))) {
  if (event.key === 'Home') { ... }
  if (event.key === 'End') { ... }
}

getTarget() calls composedPath() and isTypeableElement() calls element.matches(TYPEABLE_SELECTOR). Both now run for every keystroke that reaches commonOnKeyDown — every character typed into a combobox filter, every arrow key in a menu — to answer a question that only matters for two keys.

Failure scenario: A user typing in a combobox with a long list pays a composedPath() allocation plus a selector match per character, on a hot input path, for no benefit.

Fix: Check the key first so the DOM work is only done when it can matter:

if (
  (event.key === 'Home' || event.key === 'End') &&
  !typeableComboboxReference &&
  !isTypeableElement(getTarget(event.nativeEvent))
) {
  stopEvent(event);
  indexRef.current = event.key === 'Home' ? minIndex : maxIndex;
  onNavigate(event);
}

Docs (1)

1. 🟡 The new condition is non-obvious and unexplained

Location: packages/react/src/floating-ui-react/hooks/useListNavigation.ts:576

The reason there are now two typeable checks is genuinely subtle: typeableComboboxReference covers the input-as-trigger combobox, and the new target check covers the case where the reference is a non-typeable trigger but focus is on an input rendered inside the popup (inputInsidePopup). Nothing in the code says this, and the surrounding file is otherwise well commented about its non-obvious branches.

Failure scenario: A maintainer sees the two checks as redundant (in the input-as-trigger case the target is the reference) and deletes one — reintroducing this bug, or breaking the Combobox.Chips case where the reference is the input but focus sits on a non-typeable chip.

Fix: Add a short comment above the condition, e.g. // Home/End belong to the caret when a text field is focused — either the combobox reference itself, or an input rendered inside the popup.

Verdict

Approve after nits — the runtime fix is correct and consistent with existing combobox behavior; the only substantive gap is that the tests pin the modifier-key cases while leaving the plain Home/End behavior change uncovered.

---

🤖 Review generated with Claude Code · Opus 5 (High) · medium review depth · 34 turns · 6m25s · $1.64 · run

[netlify]

✅ Deploy Preview for base-ui ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	fc2c918
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a858cd10f90cc0009635c81
😎 Deploy Preview	https://deploy-preview-5539--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	🔺+17B(0.00%)	🔺+7B(0.00%)

Details of bundle changes

Performance

Total duration: 1,086.65 ms +100.63 ms^(+10.2%) | Renders: 92 ⁽⁺⁰⁾ | Paint: 1,772.21 ms +139.50 ms^(+8.5%)

Test	Duration	Renders
Checkbox mount (500 instances)	84.42 ms 🔺+24.46 ms(+40.8%)	1 (+0)

14 tests within noise — details

Metric alarms

Test	Metric	Change
Checkbox mount (500 instances)	bench:paint	🔺 +27.36 ms

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	7228c0c
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a8686ca8e3f7a000838665d
😎 Deploy Preview	https://deploy-preview-5539--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[sai6855]

@claude review

[github-actions]

PR review

The runtime change is narrow and looks correct: useListNavigation no longer hijacks Home/End when the key lands on a typeable element, which is exactly the input-inside-the-popup combobox case (role="combobox" is applied to the reference, so typeableComboboxReference is false when the trigger is the reference and the input lives in the popup). I found no merge-blocking defect. The remaining findings are a missing assertion on the actual user-visible outcome, a leaky listener in a test, some duplicated test setup, and a comment/title that under-describe what the guard now does.

Bugs (1)

1. ℹ️ Modified Home/End still navigate whenever the target isn't typeable, unlike useCompositeRoot

Location: packages/react/src/floating-ui-react/hooks/useListNavigation.ts:579

if (
  (event.key === 'Home' || event.key === 'End') &&
  !typeableComboboxReference &&
  !isTypeableElement(getTarget(event.nativeEvent))
) {

The fix keys off the event target, not off modifiers. The repo's other list-navigation primitive takes the opposite approach — useCompositeRoot bails out early via isModifierKeySet(event, modifierKeys) (packages/react/src/internals/composite/root/useCompositeRoot.ts:212) and additionally returns to native textbox behavior when event.shiftKey is held (line 235). So after this PR, Shift+Home moves the highlight in a Menu/Select (the new MenuRoot.test.tsx test pins exactly that) but is ignored in a Toolbar or other Composite-driven list.

For menus and selects there is no native Shift+Home behavior to preserve, so this is defensible and I'm not treating it as a defect — but it is worth a deliberate decision, since the PR title claims the opposite rule ("Prevent Home key navigation when shift key is pressed") and the code never inspects a modifier.

Failure scenario: A consumer who reports "Shift+Home shouldn't do list navigation" gets the fix in a combobox but not in a menu, and the two primitives keep diverging.

Fix: Either confirm the target-based rule is the intended cross-component contract (and keep the Menu test as the guard), or align with isModifierKeySet as the composite path already does.

Tests (2)

1. 🟠 Nothing asserts that Shift+Home/Shift+End actually extends the text selection

Location: packages/react/src/combobox/input/ComboboxInput.test.tsx:660

document.addEventListener('keydown', handleKeyDown);
await user.keyboard(`{Shift>}{${key}}{/Shift}`);
document.removeEventListener('keydown', handleKeyDown);

expect(seen).toEqual([false]);

The six new combobox tests cover the mechanism (highlight doesn't move; the event reaches document un-prevented) but never the outcome the issue is about — that Shift+Home selects from the caret to the start of the input value. That's a reasonable proxy in jsdom, which doesn't implement selection extension, but the file already establishes the escape hatch for real-browser behavior (it.skipIf(isJSDOM) at line 681 asserts scrollLeft after Home/End), so the real assertion is available in the Chromium env.

Failure scenario: A future change makes ComboboxInput.onKeyDown handle modified Home/End itself (it currently bails at ComboboxInput.tsx:343) and calls setSelectionRange collapsed. The event would still reach document un-prevented and the highlight would still not move, so every new test passes while Shift+Home stops selecting text again.

Fix: Add an it.skipIf(isJSDOM) case in the same style as line 681: type a value in the popup input, press {Shift>}{Home}{/Shift}, and assert selectionStart === 0 and selectionEnd === value.length (and the mirror for End).

2. 🟡 The keydown listener leaks when the assertion or key press throws

Location: packages/react/src/combobox/input/ComboboxInput.test.tsx:674

document.addEventListener('keydown', handleKeyDown);
await user.keyboard(`{Shift>}{${key}}{/Shift}`);
document.removeEventListener('keydown', handleKeyDown);

removeEventListener is not in a finally, so if user.keyboard rejects the handler stays attached to document for the rest of the file.

Failure scenario: This is an it.each over two keys. If the 'Home' iteration throws inside user.keyboard, the 'Home' handler is still attached during the 'End' iteration and every later test in the file, pushing into a dead array — noise that makes the second failure harder to read, plus a listener that outlives the test.

Fix: Wrap in try { ... } finally { document.removeEventListener('keydown', handleKeyDown); }.

Simplifications (1)

1. 🟡 Three near-identical modifier tests could be one it.each, and Ctrl+End would come along for free

Location: packages/react/src/combobox/input/ComboboxInput.test.tsx:563

The Shift+Home, Shift+End, and Ctrl+Home tests (lines 563–614) repeat the same eight-line setup — render, click trigger, wait for input focus, arrow down N times, wait for the highlight — and differ only in the modifier, the key, and how many {ArrowDown}s precede it. Ctrl+End is the one combination left untested.

Failure scenario: ~50 lines of duplicated setup; any change to PopupCombobox or to how the popup input takes focus has to be edited in three places, and the missing Ctrl+End case stays missing.

Fix: Collapse into one parameterized test, e.g.

it.each([
  ['Shift', 'Home'],
  ['Shift', 'End'],
  ['Control', 'Home'],
  ['Control', 'End'],
])('does not move the highlight when %s+%s is pressed inside the popup', async (modifier, key) => { ... });

highlighting a middle item (banana) once so a move in either direction fails the assertion.

Docs (2)

1. 🟡 The comment describes only the combobox case, but the guard is global

Location: packages/react/src/floating-ui-react/hooks/useListNavigation.ts:576

// The list-level Home/End shortcuts must not swallow the native caret and text-selection
// behavior when the key lands in a text field, which happens when a typeable combobox
// renders its input inside the popup rather than as the reference element.

useListNavigation is shared by Menu, Select, and Combobox, and the new check applies to any typeable target — including a consumer-rendered <input>, <textarea>, or [contenteditable] inside a menu or select popup (TYPEABLE_SELECTOR in floating-ui-react/utils/constants.ts:4). It also suppresses unmodified Home/End list navigation for those targets, not just modified ones; that only happens to be invisible for Combobox because ComboboxInput already stops unmodified Home/End itself.

Failure scenario: A maintainer debugging "Home no longer jumps to the first item in my menu with a filter input" reads this comment, concludes the guard is combobox-specific, and looks elsewhere.

Fix: Reword to state the general rule — list-level Home/End are skipped whenever the key targets a text field, so the caret keeps them — and mention the combobox-input-in-popup case as the motivating example.

2. 🟡 PR title contradicts the change and the scope doesn't follow the commit convention

Location: packages/react/src/floating-ui-react/hooks/useListNavigation.ts:579

The title "[useListNavigation] Prevent Home key navigation when shift key is pressed" describes a modifier check that isn't in the diff, and the new Menu test explicitly asserts that Shift+Home/Ctrl+Home do still change the highlighted item. AGENTS.md also asks for scopes mirroring package or component names.

Failure scenario: The squashed commit message and changelog entry tell users that modified Home no longer navigates, which is false for Menu and Select.

Fix: Retitle along the lines of [combobox] Keep native Home/End behavior when the key targets a text field.

Verdict

Approve after nits - the runtime guard is correct and scoped, but the tests don't pin the selection behavior the fix exists for, and the comment/title describe a different rule than the code implements.

---

🤖 Review generated with Claude Code · Opus 5 (High) · medium review depth · 44 turns · 8m0s · $2.61 · run

[sai6855]

For menus and selects there is no native Shift+Home behavior to preserve, so this is defensible and I'm not treating it as a defect — but it is worth a deliberate decision, since the PR title claims the opposite rule ("Prevent Home key navigation when shift key is pressed") and the code never inspects a modifier.

SInce Menu doesn't have Shift+Home behaviour, i'm leaving code as is.