Skip to content

Fix AbstractCombobox.onInput() never firing on native input events - #672

Merged
fpigeonjr merged 1 commit into
masterfrom
gh-659-abstractcombobox-oninput-is-a-public-no-op-input-e
Sep 1, 2026
Merged

Fix AbstractCombobox.onInput() never firing on native input events#672
fpigeonjr merged 1 commit into
masterfrom
gh-659-abstractcombobox-oninput-is-a-public-no-op-input-e

Conversation

@fpigeonjr

Copy link
Copy Markdown
Contributor

Description

AbstractCombobox (src/ui-kit/experimental/aria/abstract-combobox/abstract-combobox.ts) declares a public onInput(callback, context) method and registers "input" as a valid event name in _initEventDispatcher, but nothing in the class ever dispatched that event — the native input DOM listener in _setupInputEvents only dispatched "search". Any consumer calling combobox.onInput(cb, ctx) registered a callback that was never invoked.

Fixed by dispatching "input" alongside "search" from the same native input listener, so onInput callbacks now fire without changing existing "search" behavior (which sam-picker's onSearch output and its spec depend on).

Added a regression spec (abstract-combobox.spec.ts) mirroring the existing onSearch coverage, confirming onInput callbacks are invoked when the input fires a native input event.

A Playwright/e2e test was intentionally not added — this is a pure event-wiring bug (jsdom's synthetic input dispatch behaves identically to a real browser here), not a CSS-cascade or real-pointer hit-testing issue, so per AGENTS.md's Vitest/Playwright boundary guidance a Vitest spec is sufficient to catch and pin this regression.

Motivation and Context

Closes #659

Type of Change (Select One and Apply Label)

  • Bug fix (non-breaking change which fixes an issue) → Apply bugfix label
  • New feature (non-breaking change which adds functionality) → Apply enhancement label
  • Breaking change (fix or feature that would cause existing functionality to change) → Apply breaking label
  • Documentation / configuration update → Apply maintenance label

How to Test

  1. npm ci && npm ci --prefix test-app
  2. cd test-app && npx vitest run --config vitest.config.mts abstract-combobox — confirm the new "dispatches an input event whenever the input fires a native input event" spec passes, alongside the existing onSearch spec.
  3. npm --prefix test-app test — confirm the full suite (151 files / 1378 tests) still passes.

Expected result: onInput(cb, ctx) callbacks registered on AbstractCombobox fire when the underlying input fires a native input event; onSearch continues to fire as before.

Screenshots (if appropriate)

N/A — backend/logic change only, no UI changes.

Checklist

  • Branch name follows convention (e.g. gh-<number>-<slug>)
  • PR title starts with a verb in the imperative mood
  • I have self-reviewed my own code
  • format:check passes (npm run format:check)
  • lint passes (npm run lint)
  • build passes (cd test-app && npm run build)
  • Tests pass and coverage is reported (cd test-app && npm test)
  • If this change requires a documentation update, I have updated it accordingly
  • If there are dependent changes, they have been merged and published in downstream modules

Note on coverage gate: npm run coverage:check currently fails on this branch, but this failure pre-exists on master (verified against a clean clone at db5cc25b, the current master HEAD, and confirmed by the two most recent master CI runs both failing on the same gate) — unrelated to this change. This PR's coverage is measured slightly higher than master's (88.58% vs 88.57% statements) due to the added spec.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes AbstractCombobox.onInput() so registered callbacks receive native input events.

Changes:

  • Dispatches "input" alongside existing "search" events.
  • Adds regression coverage for onInput().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
abstract-combobox.ts Dispatches registered input callbacks.
abstract-combobox.spec.ts Verifies native input event handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@fpigeonjr
fpigeonjr marked this pull request as ready for review September 1, 2026 17:33
@fpigeonjr
fpigeonjr requested a review from a team as a code owner September 1, 2026 17:33
@fpigeonjr
fpigeonjr marked this pull request as draft September 1, 2026 17:38
The native input listener in _setupInputEvents only dispatched the
"search" event, so any onInput(cb, ctx) registration was a dead
callback despite "input" being a registered event name in
_initEventDispatcher.

Dispatch "input" alongside "search" from the same native input
listener so onInput callbacks actually fire, and add a regression
spec mirroring the existing onSearch coverage.

Closes #659
@fpigeonjr
fpigeonjr force-pushed the gh-659-abstractcombobox-oninput-is-a-public-no-op-input-e branch from fd98db7 to b5abb25 Compare September 1, 2026 18:33
@fpigeonjr
fpigeonjr marked this pull request as ready for review September 1, 2026 18:57
@fpigeonjr
fpigeonjr merged commit f6e7223 into master Sep 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AbstractCombobox.onInput() is a public no-op — input event never dispatched

3 participants