Skip to content

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

Open
samuelreichert wants to merge 4 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 4 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

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 New isChipNavigationPermitted helper; updated onKeyDown guard
packages/pluggableWidgets/combobox-web/src/__tests__/MultiSelection.spec.tsx New describe block with 8 unit tests for keyboard/chip navigation matrix
packages/pluggableWidgets/combobox-web/e2e/ComboboxMultiSelectionKeys.spec.js New E2E regression spec for Backspace/Delete round-trip
packages/pluggableWidgets/combobox-web/CHANGELOG.md ## [Unreleased] → ### Fixed entry added
packages/pluggableWidgets/combobox-web/openspec/… Archived change artifacts (design, proposal, spec, tasks)

Skipped (out of scope): pnpm-lock.yaml, dist/, generated snapshots

CI checks: could not run gh pr checks without approval — verify before merging.


Findings

⚠️ Low — E2E beforeEach missing readiness wait after actionButton1 click

File: packages/pluggableWidgets/combobox-web/e2e/ComboboxMultiSelectionKeys.spec.js lines 9–13

Note: The existing Combobox.spec.js calls waitForMendixApp(page) after clicking actionButton1 (which opens a modal) before clicking the tab inside it. The new spec clicks tabPage2 immediately after actionButton1 without any intervening wait. Each test body guards with toBeVisible({ timeout: 10000 }) which will retry, but if the modal is still opening when tabPage2 is clicked, Playwright may click a non-interactive element. To match the proven pattern:

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);  // wait for modal to settle
    await page.click(".mx-name-tabPage2");
});

⚠️ Low — { delay: 10 } on option clicks is a hardcoded timing hint

File: packages/pluggableWidgets/combobox-web/e2e/ComboboxMultiSelectionKeys.spec.js lines 46–47

Note: options.nth(0).click({ delay: 10 }) uses a hardcoded delay. Playwright's click() already auto-waits for actionability; the delay doesn't add retry semantics. If the concern is that the dropdown collapses between clicks, a web-first assertion between them is more robust:

await options.nth(0).click();
await expect(options.nth(1)).toBeVisible();
await options.nth(1).click();

⚠️ Low — Archived tasks.md references deprecated window.mx.session.logout() pattern

File: packages/pluggableWidgets/combobox-web/openspec/changes/archive/.../tasks.md line 311

Note: Task 4.1 says "including window.mx.session.logout() cleanup" — but the e2e guidelines explicitly state the fixture handles logout automatically and manual teardown is no longer needed. The live code correctly omits it. This is stale text in an archived doc only, so non-blocking, but worth cleaning up to avoid confusing future contributors.


⚠️ Low — openspec/specs/multiselect-keyboard-interaction/spec.md has unfilled Purpose placeholder

File: packages/pluggableWidgets/combobox-web/openspec/specs/multiselect-keyboard-interaction/spec.md line 5

Note: "TBD - created by archiving change fix-combobox-multiselect-backspace-select-all. Update Purpose after archive." was never filled in. Not runtime code, but this is the canonical spec file (not the archive copy) so future readers will land here first.


Positives

  • isChipNavigationPermitted faithfully mirrors downshift's own isKeyDownOperationPermitted — the fix converges on the library's contract rather than inventing an independent rule that could drift.
  • The comment naming the downshift version (7.6.2) and source path (dist/downshift.cjs.js) reduces future regression risk on upgrades — exactly the right level of documentation.
  • Unit test coverage is thorough: full selection, partial selection anchored at 0, caret-at-end, empty input (chip activated), empty input with no chips (no-throw), modifier-key held, and ArrowLeft in both collapsed and selected states.
  • E2E spec uses @mendix/run-e2e/fixtures (not raw Playwright), all mx-name-* selectors, and web-first assertions (toHaveValue, toBeVisible) throughout.
  • CHANGELOG entry is user-facing and behaviour-only — no implementation detail leaked.
  • Fix is narrowly scoped: only the guard logic changed, no surrounding refactor or unrelated cleanup.

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.

1 participant