diff --git a/.changeset/7687-combobox-option-disabled.md b/.changeset/7687-combobox-option-disabled.md new file mode 100644 index 0000000000..8d8fb4c507 --- /dev/null +++ b/.changeset/7687-combobox-option-disabled.md @@ -0,0 +1,42 @@ +--- +'@object-ui/components': minor +--- + +Honour `options[].disabled` on a `combobox` node (objectui#7687). + +**User-visible behaviour change, deliberately — hence `minor`, not `patch`.** The +member was already declared by `@object-ui/types` (`ComboboxOption.disabled`) and +already validated by the zod mirror (`ComboboxOptionSchema`, pinned as `boolean` +on both faces by the objectui#7087 twin-symmetry ruling), but the component never +read it: `Combobox` mapped each option to a `CommandItem` carrying `key`, `value` +and `onSelect` only. So an option authored `{ value, label, disabled: true }` +passed `safeValidateSchema`, type-checked against the published `ComboboxSchema`, +and then rendered as an ordinary, fully selectable option — a declared key with no +read site behind it, the class the enforce-or-remove ledgers exist to close. + +An author who already writes `disabled: true` today gets a different combobox +after this change: that option now renders dimmed and can no longer be chosen, by +click or by keyboard. That is the intended repair — declared and validated should +mean enforced — but it is a change in what existing metadata does, not a silent +internal fix, so it is priced as a behaviour change rather than a patch. + +The alternative remedy, retiring `disabled` from `ComboboxOption` and the zod +mirror, was weighed and **not** adopted: it narrows a published surface and would +require the objectui#7087 twin-symmetry pin to be changed, where honouring the key +restores declared = enforced at the cost of one prop. The spelling follows the +sibling select renderer, which already sets `disabled={opt.disabled}` on its +`SelectItem`. + +Nothing else moves. The whole-control `disabled` prop (the one forwarded to the +trigger button) is untouched, no key is added to `@object-ui/types`, and no new +key is introduced — this release only starts reading one that was already +published. + +`@object-ui/types` is deliberately **not** given its own bump. The one file that +changes there is a test, `component-docs-disabled-inherited-7239.test.ts`: its +census over `content/docs/components` counts every documented `disabled?:` row, +and documenting the member adds a legitimate row that the ledger now claims as +INDEPENDENT (the shipped `ComboboxOption` declares `disabled` itself and does not +extend `BaseSchema`, so the narrow `boolean` spelling is correct for it). No +shipped type or value moves in that package, so there is no behaviour there to +version. diff --git a/content/docs/components/form/combobox.mdx b/content/docs/components/form/combobox.mdx index da2154c0af..ed6518e0fc 100644 --- a/content/docs/components/form/combobox.mdx +++ b/content/docs/components/form/combobox.mdx @@ -28,6 +28,7 @@ The Combobox component combines a text input with a dropdown list, allowing user interface ComboboxOption { value: string; label: string; + disabled?: boolean; // Option renders dimmed and cannot be selected } interface ComboboxSchema { diff --git a/packages/components/src/__tests__/combobox-option-disabled.test.tsx b/packages/components/src/__tests__/combobox-option-disabled.test.tsx new file mode 100644 index 0000000000..de369ec483 --- /dev/null +++ b/packages/components/src/__tests__/combobox-option-disabled.test.tsx @@ -0,0 +1,108 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#7687 — `options[].disabled` on a `combobox` node must be READ. + * + * The member was declared (`@object-ui/types` `ComboboxOption.disabled`), + * validated (`ComboboxOptionSchema` in the zod mirror, pinned as `boolean` on + * both faces by `disabled-twin-symmetry-7087.test.ts`) and never read: the + * component mapped each option to a `CommandItem` with `key` / `value` / + * `onSelect` only, so an option authored `disabled: true` passed validation, + * type-checked against the published `ComboboxSchema`, and rendered as an + * ordinary selectable option — a declared key with no read site behind it. + * + * ⛔ An attribute-only pin is not enough here: it passes on "styled disabled + * but still clickable", which is the exact defect this card is about. So the + * behaviour is pinned too — a disabled option must not fire `onValueChange`. + * The third test is the CONTROL that keeps that negative from being vacuous: + * without it, a popover that never opened would satisfy "not called". + */ +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Combobox } from '../custom/combobox'; + +const OPTIONS = [ + { value: 'alpha', label: 'Alpha' }, + { value: 'beta', label: 'Beta', disabled: true }, + { value: 'gamma', label: 'Gamma', disabled: false }, +]; + +function openDropdown() { + fireEvent.click(screen.getByRole('combobox')); +} + +describe('Combobox honours options[].disabled (objectui#7687)', () => { + it('marks an option authored `disabled: true` as disabled in the DOM', () => { + render(); + openDropdown(); + + const beta = screen.getByRole('option', { name: /Beta/ }); + // `data-disabled` is what the CommandItem wrapper's className already + // styles (`data-[disabled=true]:opacity-50 …:pointer-events-none`); + // `aria-disabled` is what assistive tech and cmdk's own valid-item + // selector read. + expect(beta).toHaveAttribute('data-disabled', 'true'); + expect(beta).toHaveAttribute('aria-disabled', 'true'); + }); + + it('refuses to select a disabled option — no onValueChange', () => { + const onValueChange = vi.fn(); + render(); + openDropdown(); + + fireEvent.click(screen.getByRole('option', { name: /Beta/ })); + expect(onValueChange).not.toHaveBeenCalled(); + }); + + it('refuses the keyboard path too — Enter on a search narrowed to the disabled option selects nothing', () => { + const onValueChange = vi.fn(); + render(); + openDropdown(); + + // A separate code path from the click above: cmdk keeps disabled items out + // of the valid-item selector its arrow keys and its auto-select walk, and + // never registers the `cmdk-item-select` listener Enter dispatches. With + // the list narrowed to `Beta` alone there is nothing left to select. + const search = screen.getByPlaceholderText('Search...'); + fireEvent.change(search, { target: { value: 'bet' } }); + expect(screen.getByRole('option', { name: /Beta/ })).toBeInTheDocument(); + expect(screen.queryByRole('option', { name: /Alpha/ })).toBeNull(); + + fireEvent.keyDown(search, { key: 'Enter' }); + expect(onValueChange).not.toHaveBeenCalled(); + }); + + it('CONTROL — options without `disabled: true` stay selectable', () => { + const onValueChange = vi.fn(); + render(); + openDropdown(); + + // Omitted (`alpha`) and explicitly `false` (`gamma`) are both enabled. + const alpha = screen.getByRole('option', { name: /Alpha/ }); + const gamma = screen.getByRole('option', { name: /Gamma/ }); + expect(alpha).toHaveAttribute('data-disabled', 'false'); + expect(gamma).toHaveAttribute('data-disabled', 'false'); + + fireEvent.click(alpha); + expect(onValueChange).toHaveBeenCalledWith('alpha'); + }); + + it('CONTROL — the keyboard path still selects an enabled option', () => { + const onValueChange = vi.fn(); + render(); + openDropdown(); + + const search = screen.getByPlaceholderText('Search...'); + fireEvent.change(search, { target: { value: 'gam' } }); + fireEvent.keyDown(search, { key: 'Enter' }); + expect(onValueChange).toHaveBeenCalledWith('gamma'); + }); +}); diff --git a/packages/components/src/custom/combobox.tsx b/packages/components/src/custom/combobox.tsx index 6eae8e5468..7a47a9c759 100644 --- a/packages/components/src/custom/combobox.tsx +++ b/packages/components/src/custom/combobox.tsx @@ -115,6 +115,19 @@ export function Combobox({ { onValueChange?.(currentValue === value ? "" : currentValue) setOpen(false) diff --git a/packages/types/src/__tests__/component-docs-disabled-inherited-7239.test.ts b/packages/types/src/__tests__/component-docs-disabled-inherited-7239.test.ts index f8b03314aa..814cfb7980 100644 --- a/packages/types/src/__tests__/component-docs-disabled-inherited-7239.test.ts +++ b/packages/types/src/__tests__/component-docs-disabled-inherited-7239.test.ts @@ -78,6 +78,33 @@ * That last pair is the point of the control: a failure that reddened the * independent rows too would mean the sweep was indiscriminate, not that the * inherited rows were wrong. + * + * ## Amendments + * + * The population is a ledger, not a constant: it moves when a page gains or + * loses a real `disabled` row. Each move is recorded here with its cause, so a + * later reader can tell a deliberate claim from a number someone bumped to get + * back to green. + * + * - **22 -> 23 rows, INDEPENDENT 8 -> 9 (objectui#7687).** `ComboboxOption` + * joins the independent table. Its `disabled` was always DECLARED by + * `packages/types/src/form.ts` and validated by `ComboboxOptionSchema`, but + * the combobox component never read it, so an option authored + * `disabled: true` rendered selectable; #7687 makes the component honour it + * and documents the member on `form/combobox.mdx`, which is the row this + * census then measured. It classifies INDEPENDENT on this file's own + * criterion, not by resemblance to its neighbours: the shipped + * `ComboboxOption` declares `disabled` itself and does NOT extend + * `BaseSchema`, so the narrow `boolean` is the correct spelling and the + * second INDEPENDENT assertion holds for it unchanged. + * + * ⚠️ Note for the class, since the #7687 card recorded the opposite: that + * page's `interface` block sits in a `plaintext` fence and the card + * concluded no CI gate could see it. True of the doc-type gates named + * above, and false overall — THIS census reads those `.mdx` files as text + * and caught the new row. A doc edit under `content/docs/components` that + * touches a `disabled` row is answerable to `packages/types`, so a run + * narrowed to the package the code fix lives in cannot see it. */ import { describe, expect, it } from 'vitest'; @@ -184,6 +211,8 @@ const INDEPENDENT = [ { page: 'basic/button-group.mdx', iface: 'ButtonGroupButton', shippedName: 'ButtonGroupButton' }, { page: 'disclosure/accordion.mdx', iface: 'AccordionItem', shippedName: 'AccordionItem' }, { page: 'disclosure/toggle-group.mdx', iface: 'ToggleGroupItem', shippedName: 'ToggleGroupItem' }, + // Added by objectui#7687, which made the member real — see `## Amendments`. + { page: 'form/combobox.mdx', iface: 'ComboboxOption', shippedName: 'ComboboxOption' }, { page: 'form/form.mdx', iface: 'FormField', shippedName: 'FormField' }, { page: 'form/radio-group.mdx', iface: 'RadioOption', shippedName: 'RadioOption' }, // Doc-local names; the shipped shape they illustrate is `MenuCommandItem`. @@ -246,7 +275,7 @@ describe('the two tables account for every documented `disabled` row (objectui#7 it('sees exactly the population this card measured', () => { expect({ rows: ROWS.length, inherited: INHERITED.length, independent: INDEPENDENT.length }).toEqual( - { rows: 22, inherited: 14, independent: 8 }, + { rows: 23, inherited: 14, independent: 9 }, ); }); });