Skip to content

[WC-3347]: fix(combobox-web): keep filter text cleared after select-all + Backspace - #2386

Open
samuelreichert wants to merge 8 commits into
mainfrom
WC-3347-combobox-multiselect-backspace
Open

[WC-3347]: fix(combobox-web): keep filter text cleared after select-all + Backspace#2386
samuelreichert wants to merge 8 commits into
mainfrom
WC-3347-combobox-multiselect-backspace

Conversation

@samuelreichert

@samuelreichert samuelreichert commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Pull request type

Bug fix (non-breaking change which fixes an issue)


Description

Reported by a customer: in a multi-select Combobox, type filter text, press Ctrl/Cmd+A, press Backspace. The input looks empty — but click outside the Combobox and back in, and the text is there again. Using Delete instead of Backspace works correctly.

Root cause. MultiSelection.tsx layers a custom onKeyDown on top of downshift's input props, gated on inputRef.current?.selectionStart === 0:

if (
    (event.key === "Backspace" && inputRef.current?.selectionStart === 0) ||
    (event.key === "ArrowLeft" && isSelectedItemsBoxStyle && inputRef.current?.selectionStart === 0)
) {
    setActiveIndex(selectedItems.length - 1);
}

Fix. downshift already ships exactly the predicate this handler needs, and applies it to its own dropdown Backspace handling — but does not export it. Mirror it locally as isChipNavigationPermitted: no modifier held, and either an empty input or a caret collapsed at position 0. The ArrowLeft branch carried the identical faulty check and is corrected with it. The helper carries a comment naming the downshift version and source file so a future upgrade re-checks it.

What should be covered while testing?

Multi-select Combobox with at least one already-selected chip (with zero chips setActiveIndex(-1) is a no-op and the bug never surfaces):

  1. Type filter text → Ctrl/Cmd+A → Backspace. Input clears, focus stays in the input, no chip becomes active. Click outside, click back in — still empty.
  2. Same with Delete — unchanged behaviour.
  3. Select only part of the text starting at position 0, press Backspace — only the selected characters go, trailing text survives.
  4. Empty filter input + Backspace — last chip becomes active and can be removed (regression risk: this must still work).
  5. selectedItemsStyle="boxes": ArrowLeft with a collapsed caret at position 0 reaches the chips; ArrowLeft with text selected stays in the input.
  6. Single-select Combobox: Backspace on an empty input still clears the selection.

@samuelreichert
samuelreichert requested a review from a team as a code owner August 14, 2026 14:43
@github-actions

This comment has been minimized.

@samuelreichert
samuelreichert force-pushed the WC-3347-combobox-multiselect-backspace branch 2 times, most recently from 9e0e5af to f1e2229 Compare August 17, 2026 08:51
@github-actions

This comment has been minimized.

@samuelreichert
samuelreichert force-pushed the WC-3347-combobox-multiselect-backspace branch from f1e2229 to 95cb2ae Compare August 18, 2026 09:34
@github-actions

This comment has been minimized.

r0b1n
r0b1n previously approved these changes Aug 20, 2026
@samuelreichert
samuelreichert force-pushed the WC-3347-combobox-multiselect-backspace branch from d702c56 to f1afa89 Compare August 27, 2026 12:52
samuelreichert and others added 8 commits August 27, 2026 14:53
…c change

Backspace/Delete on a selected chip drops keyboard focus to the document
body unless the chip was the last one, because downshift only re-focuses
when its own activeIndex changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e removal

downshift removes the selected item but only moves DOM focus when its own
activeIndex changes, which it does not for any chip other than the last.
Track the chip being removed and restore focus to the item that takes its
place once the removal renders, falling back to the filter input.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…moval

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@samuelreichert
samuelreichert force-pushed the WC-3347-combobox-multiselect-backspace branch from f1afa89 to 0bfd36f Compare August 27, 2026 12:53
@samuelreichert
samuelreichert requested a review from r0b1n August 27, 2026 12:54
@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/combobox-web/src/components/MultiSelection/MultiSelection.tsx Added isChipNavigationPermitted helper, chip-ref array, chipToRefocusRef, and a useEffect that restores focus after chip removal
packages/pluggableWidgets/combobox-web/src/__tests__/MultiSelection.spec.tsx Two new describe blocks covering the WC-3347 key-guard matrix and the chip-focus-after-removal regression
packages/pluggableWidgets/combobox-web/e2e/ComboboxMultiSelectionKeys.spec.js New E2E spec covering both bug regressions end-to-end
packages/pluggableWidgets/combobox-web/CHANGELOG.md Two ### Fixed entries under [Unreleased]
packages/pluggableWidgets/combobox-web/openspec/… OpenSpec artifacts (archived change + live change + synced spec)

CI checks could not be fetched (sandbox restriction). All reviewed code compiles cleanly per the tasks log (pnpm run build green, 61 unit tests passing, lint clean).

Skipped (out of scope): dist/, pnpm-lock.yaml, openspec/ docs (design/proposal/tasks artefacts — not runtime code)


Findings

⚠️ Low — E2E spec uses page.goto without waitForMendixApp

File: packages/pluggableWidgets/combobox-web/e2e/ComboboxMultiSelectionKeys.spec.js line 33 (and line 80)

Note: Combobox.spec.js (the established baseline) imports waitForMendixApp from @mendix/run-e2e/mendix-helpers and calls it right after page.goto. The guidelines note that the custom fixture auto-wraps page.goto() to call waitForMendixApp(), so if that is the case here the omission is harmless — but the baseline spec also calls it explicitly for the second navigation step (await waitForMendixApp(page) after actionButton1 click). The new spec instead uses await page.click(".mx-name-actionButton1") with no subsequent wait, then immediately tries to interact with .mx-name-tabPage2. Under slow CI this may race. A defensive waitForMendixApp (or an await expect on a landmark element) after the actionButton1 click would align with the project baseline.

Suggested fix:

import { waitForMendixApp } from "@mendix/run-e2e/mendix-helpers";

test.beforeEach(async ({ page }) => {
    await page.goto("/p/combobox");
    await page.click(".mx-name-actionButton1");
    await waitForMendixApp(page);          // <-- add, matching Combobox.spec.js
    await page.click(".mx-name-tabPage2");
});

⚠️ Low — chipRefs array is never trimmed when chips are removed

File: packages/pluggableWidgets/combobox-web/src/components/MultiSelection/MultiSelection.tsx line 51 / lines 190–192

Note: The chipRefs.current array grows as chips are added (index-keyed inline ref callbacks) but the slots are never deleted when a chip is removed. After removing chip 2 from a 3-chip list chipRefs.current still holds a stale entry at index 2 (null, because the node unmounted). The focus restore effect bounds-checks with Math.min and uses optional chaining (?.focus()), so there is no crash or incorrect focus — but the array accumulates null slots indefinitely for long sessions with repeated add/remove cycles. Not a functional bug at any realistic scale, but worth noting. Clearing stale tail slots after each removal (e.g. chipRefs.current = chipRefs.current.slice(0, selectedItems.length)) inside the effect after focusing would keep the array tight.


Positives

  • isChipNavigationPermitted exactly mirrors downshift's internal isKeyDownOperationPermitted with an explicit version comment — this is the right level of alignment and makes future downshift upgrades auditable.
  • The useEffect keyed on selectedItems.length (not selectedItems identity) is the correct dependency to avoid spurious re-runs; the comment explaining why is genuinely useful.
  • The decision to use useEffect instead of useLayoutEffect so the widget's effect runs after downshift's own focus effect — and therefore wins — is a subtle correctness detail that is both right and documented.
  • Unit tests cover all three chip positions (first, middle, last) plus the single-chip case, for both Backspace and Delete, with document.activeElement assertions and an explicit not.toBe(document.body) guard on the reported regression.
  • The applyValueChange / component.rerender pattern in the test setup correctly models the Mendix client feeding updated props back in, which is the mechanism that unmounts the chip DOM node.
  • CHANGELOG entries are user-facing only — no implementation detail leaked, and no premature version bump.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants