Skip to content

fix(listbox): clicking disabled option no longer deselects current selection#247

Merged
ianmcburnie merged 2 commits intomasterfrom
246-listbox-bug
Apr 1, 2026
Merged

fix(listbox): clicking disabled option no longer deselects current selection#247
ianmcburnie merged 2 commits intomasterfrom
246-listbox-bug

Conversation

@ianmcburnie
Copy link
Copy Markdown
Member

@ianmcburnie ianmcburnie commented Apr 1, 2026

  • Clicking a disabled option (aria-disabled="true") in a listbox was incorrectly deselecting the currently selected item
    • Root cause: select() called unselect() before checking whether the target item was disabled, so any call path (mouse click, Space/Enter keyboard, auto-select on navigation, or programmatic) could clear the current selection when targeting a disabled option
    • Fix: moved the aria-disabled guard to the top of select() so that unselect() is never called when the target option is disabled — covering all call sites, not just _onClick
    • Additionally fixed _onListboxClick in makeup-listbox-button to not collapse the overlay when a disabled option is clicked

Test plan

  • Added Playwright tests for both auto-select and manual-select listbox variants confirming that clicking a disabled option preserves the existing selection
  • Added Playwright tests for makeup-listbox-button confirming the overlay stays open when clicking a disabled option
  • All listbox/listbox-button Playwright tests pass

…lection (#246)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes makeup-listbox so that clicking an aria-disabled="true" option does not clear the current selection, aligning mouse behavior with the existing navigation-emitter handling for disabled items.

Changes:

  • Add an aria-disabled guard to the listbox _onClick handler to prevent calling select() for disabled options.
  • Add Playwright coverage for both manual-select and auto-select listbox variants with a pre-selected option to ensure disabled clicks preserve selection.
  • Regenerate distribution (dist/) and docs (docs/ui/...) bundled artifacts to include the fix.

Reviewed changes

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

Show a summary per file
File Description
packages/ui/makeup-listbox/src/index.js Prevents click-selection logic from running when the clicked option is aria-disabled="true".
packages/ui/makeup-listbox/test/index.spec.js Adds regression tests ensuring disabled option clicks don’t deselect the currently selected item (manual + auto select).
packages/ui/makeup-listbox/dist/mjs/index.js Rebuilt ESM bundle containing the click guard.
packages/ui/makeup-listbox/dist/cjs/index.js Rebuilt CJS bundle containing the click guard.
docs/ui/makeup-listbox/index.min.js Rebuilt docs bundle containing the click guard.
docs/ui/makeup-listbox/index.min.js.map Updated sourcemap for the rebuilt docs bundle.
docs/ui/makeup-listbox-button/index.min.js Rebuilt docs bundle (includes listbox code) containing the click guard.
docs/ui/makeup-listbox-button/index.min.js.map Updated sourcemap for the rebuilt listbox-button docs bundle.
docs/ui/makeup-combobox/index.min.js Rebuilt docs bundle (includes listbox code) containing the click guard.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…l paths

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ianmcburnie
Copy link
Copy Markdown
Member Author

@copilot review again

Copy link
Copy Markdown

Copilot AI commented Apr 1, 2026

@copilot review again

Reviewed — the code looks good. Here's a summary of the current state:

makeup-listbox/src/index.jsselect() method (commit f621718):
The disabled guard was moved to the top of select() before unselect() is called (line 126). This covers all call paths — mouse click, Space/Enter keyboard, autoSelect on navigation, and programmatic calls. No code path can clear the current selection when targeting a disabled item.

makeup-listbox-button/src/index.js_onListboxClick() (commit f621718):
The collapse guard now checks aria-disabled before collapsing the overlay, so clicking a disabled option no longer dismisses the listbox-button dropdown.

Tests:
Both listbox and listbox-button have Playwright tests covering disabled-click behavior. click({ force: true }) is intentional — Playwright treats aria-disabled="true" as a native disabled attribute and refuses to click without it, but the DOM events still fire, which is exactly the scenario we're guarding against.

Copilot AI changed the title fix(listbox): clicking disabled option no longer deselects current se… fix(listbox): clicking disabled option no longer deselects current selection Apr 1, 2026
@ianmcburnie ianmcburnie merged commit ca8d43a into master Apr 1, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants