Skip to content

[combobox] Fix cancellation when preserving the filter after selection - #5362

Open
atomiks wants to merge 15 commits into
mui:masterfrom
atomiks:claude/combobox-eventdetails-docs-f5ddc7
Open

[combobox] Fix cancellation when preserving the filter after selection#5362
atomiks wants to merge 15 commits into
mui:masterfrom
atomiks:claude/combobox-eventdetails-docs-f5ddc7

Conversation

@atomiks

@atomiks atomiks commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes two cancellation bugs that could leave stale filter text after selecting an item in multiple mode and later reopening the popup:

  • A canceled selection-triggered input-clear was recorded as completed before the cancellation check. This suppressed the cleanup clear when the popup unmounted, so the next open could remain filtered by the previous query.
  • When reopening interrupted the close animation, the clear standing in for unmount cleanup inherited the reopening gesture. This made it indistinguishable from a selection-triggered clear, so consumer cancellation logic could cancel the cleanup too.

The runtime changes ensure that only committed clears are recorded and that cleanup clears carry no gesture event.

We have two options to support keeping filter text in multiple mode in v1:

  1. keepFilterText prop ([combobox] Add keepFilterText prop for repeated selections #4360), which is conditional on multiple (not great)
  2. isItemPress eventDetails property for onInputChange (input-clear blocks this in v1)

For No. 2, to keep the typed filter after selection, cancel the flagged clear in onInputValueChange (input inside the popup) or the item-press close in onOpenChange (input outside).

Why input-clear should change in v2

input-clear is the only effect-based reason in an otherwise cause-based union (item-press, escape-key, clear-press, …). The effect is redundant because onInputValueChange already receives the new value, while the cause is what consumers actually need to discriminate on. isItemPress bridges this additively since v1 reasons are frozen (#4348).

In v2, we should replace input-clear with cause-based reasons and drop isItemPress.

@atomiks atomiks added component: combobox Changes related to the combobox component. docs Improvements or additions to the documentation. labels Jul 29, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jul 29, 2026

Copy link
Copy Markdown

commit: 9781f50

@code-infra-dashboard

code-infra-dashboard Bot commented Jul 29, 2026

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+158B(+0.04%) 🔺+59B(+0.04%)

Details of bundle changes

Performance

Total duration: 1,310.36 ms +35.29 ms(+2.8%) | Renders: 76 (+0) | Paint: 2,072.80 ms +70.30 ms(+3.5%)

Test Duration Renders
Checkbox mount (500 instances) 97.69 ms 🔺+25.30 ms(+35.0%) 1 (+0)
Slider mount (300 instances) 132.89 ms 🔺+23.75 ms(+21.8%) 2 (+0)
Tooltip mount (300 contained roots) 63.65 ms 🔺+12.12 ms(+23.5%) 1 (+0)
Scroll Area mount (300 instances) 86.81 ms ▼-22.01 ms(-20.2%) 3 (+0)

11 tests within noise — details

Metric alarms

Test Metric Change
Checkbox mount (500 instances) bench:paint 🔺 +32.08 ms
Tooltip mount (300 contained roots) bench:paint 🔺 +20.55 ms

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

@netlify

netlify Bot commented Jul 29, 2026

Copy link
Copy Markdown

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 9781f50
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a7588b5fcceea00087cbbff
😎 Deploy Preview https://deploy-preview-5362--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

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.

@atomiks
atomiks force-pushed the claude/combobox-eventdetails-docs-f5ddc7 branch from cb7e9f4 to 7e35596 Compare July 29, 2026 00:21
@atomiks atomiks changed the title [combobox] Document keeping the filter text after selection [combobox] Fix cancellation when preserving the filter after selection Jul 29, 2026
@atomiks atomiks added type: bug It doesn't behave as expected. and removed docs Improvements or additions to the documentation. labels Jul 29, 2026
@atomiks
atomiks force-pushed the claude/combobox-eventdetails-docs-f5ddc7 branch from 2cce734 to 477303e Compare July 29, 2026 03:11
@atomiks
atomiks marked this pull request as ready for review July 29, 2026 03:17
@atomiks atomiks added the type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. label Jul 30, 2026
@atomiks

atomiks commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

@claude review low

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR review

The two cancellation fixes in setInputValue/setOpen hold up under tracing: recording hadInputClearRef only after the cancel check, and issuing the reopen cleanup clear with no gesture event and no isItemPress, both do what the description claims. One side effect of widening the reopen guard is worth a look before merge — setCloseQuery(null) now also runs when the input is outside the popup, but queryChangedAfterOpen is not reset there, which narrows the list on an interrupted-exit reopen in single mode. Nothing here is merge-blocking. Reviewed at low effort, so this pass covers runtime correctness only (tests, simplifications, and docs prose were out of scope).

Bugs (1)

1. 🟠 Reopening during the exit animation now filters a single-select list down to just the selected item

Location: packages/react/src/combobox/root/AriaCombobox.tsx:636

if (nextOpen && closeQuery !== null) {
  const isInputChange = eventDetails.reason === REASONS.inputChange;
  const clearsPendingInput =
    !isInputChange && inputInsidePopup && !inline && inputValue !== '';

  if (!isInputChange && (clearsPendingInput || inputValue === '')) {
    setQueryChangedAfterOpen(false);
  }

  setCloseQuery(null);
  ...

The outer guard lost its inputInsidePopup && !inline condition, so setCloseQuery(null) now also executes when the input is rendered outside the popup. closeQuery is set on close for both placements (setCloseQuery(query) at lines 663 and 672 is gated only on !inline), so this is a new code path, not a no-op.

For single mode with the input outside the popup, the reset of queryChangedAfterOpen does not fire on that path: clearsPendingInput is false (not inputInsidePopup) and inputValue is the label setSelectedValue just wrote, so it is non-empty. The result is closeQuery === null and queryChangedAfterOpen === true, which makes query fall back to the full selected label while shouldBypassFiltering (line 241, which requires !queryChangedAfterOpen) stays disabled.

Before this PR the block was skipped entirely for outside-popup inputs, so query stayed frozen at the previously typed text. Neither old nor new behavior matches the canonical post-unmount reopen, where handleUnmount resets queryChangedAfterOpen and shouldBypassFiltering shows the full list — but the change makes the visible result strictly narrower.

Failure scenario: Single-select combobox with the input outside the popup and an exit animation. Type ap, click Apple (popup starts animating out, input now reads Apple), then click the trigger again before the animation finishes. The list reopens showing only Apple instead of the full item list. Previously it showed the items matching ap; after a fully completed close it correctly shows everything.

Fix: Also reset queryChangedAfterOpen on the outside-popup path when the input reflects the current selection rather than a surviving filter — e.g. extend the condition with (!inputInsidePopup && inputValue === selectedLabelString). A blanket !inputInsidePopup reset would be simpler but reintroduces exactly the hazard the new comment at line 643 warns about: after an Escape close with no selection the typed filter is still visible in the outside input, and clearing the flag would let the items sync at line 1111 overwrite it with the selected label.

Verdict

Approve after nits - the cancellation fixes are correct; the only issue is a narrow interrupted-exit-animation regression in single-select filtering introduced by widening the reopen guard.


🤖 Review generated with Claude Code · low effort · 20 turns · 5m15s · $1.34 · run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: combobox Changes related to the combobox component. type: bug It doesn't behave as expected. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant