From ba33eb487d7468e0117ae60fb820a398318dc025 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 22 Sep 2026 09:24:03 +0200 Subject: [PATCH] Finish the Dropdown selection the keyboard started A filtered Dropdown narrowed correctly and made an option keyboard-active with ArrowDown, but Enter left the filter text unchanged, the listbox mounted and aria-expanded true. The value was reported through onChange; nothing else about the commit happened. React Aria treats a filtered single selection as finished only when the key the ComboBox was handed changes, and deliberately stands down otherwise so the application can drive the closing. That key was derived from the `value` prop alone, so an application that does not feed the emitted value straight back - or that answers with a value none of the options carry - never got the overlay closed or the filter text settled, and had no handle on either: the Dropdown exposes no open state and no input value. The Dropdown now remembers the option the user committed, so the commit completes on its own, and closes the filtered overlay itself instead of waiting for a value that may never resolve. An incoming `value` still decides what is selected and still drops the remembered option the moment the consumer answers, so a round-tripping application renders exactly as before. The same memory gives the unfiltered trigger its selected label instead of leaving the placeholder standing. Covered by jsdom specs for ArrowDown, ArrowUp, Escape and the unfiltered path across both bindings, and by a browser story that types, arrows and commits with no value bound at all. (#239) --- Documentation/Dropdown/index.md | 2 + Source/Dropdown/Dropdown.stories.tsx | 43 ++++++ Source/Dropdown/DropdownImplementation.tsx | 78 ++++++++--- .../and_a_filter_narrows_the_options.tsx | 93 +++++++++++++ .../and_no_filter_is_in_play.tsx | 64 +++++++++ ..._consumer_does_not_take_the_value_back.tsx | 58 +++++++++ .../and_the_list_is_dismissed_instead.tsx | 58 +++++++++ .../given/a_dropdown.tsx | 123 ++++++++++++++++++ .../scripts/verify-storybook-indexes.mjs | 8 +- 9 files changed, 506 insertions(+), 21 deletions(-) create mode 100644 Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_a_filter_narrows_the_options.tsx create mode 100644 Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_no_filter_is_in_play.tsx create mode 100644 Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_consumer_does_not_take_the_value_back.tsx create mode 100644 Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_list_is_dismissed_instead.tsx create mode 100644 Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/given/a_dropdown.tsx diff --git a/Documentation/Dropdown/index.md b/Documentation/Dropdown/index.md index b083e845..5615033f 100644 --- a/Documentation/Dropdown/index.md +++ b/Documentation/Dropdown/index.md @@ -21,6 +21,8 @@ description: Single, filtered, and multiple selection with documented names, rol When option objects contain `label` and `value`, those fields are used automatically. Use `optionLabel` and `optionValue` for another shape. +Choosing an option — with Enter on the keyboard-active option, or with a click — reports it through `onChange` once, closes the list, and shows that option. The Dropdown completes the selection itself, so a `value` that is not fed straight back, or that resolves to none of the options, never leaves the list hanging open over stale filter text. A `value` supplied afterwards still decides what is selected. + ## Label the control An external native label associates with the primary button, filter input, or multiple-select control through `htmlFor` and the Dropdown's `id`: diff --git a/Source/Dropdown/Dropdown.stories.tsx b/Source/Dropdown/Dropdown.stories.tsx index f237691e..dceeb066 100644 --- a/Source/Dropdown/Dropdown.stories.tsx +++ b/Source/Dropdown/Dropdown.stories.tsx @@ -117,6 +117,49 @@ export const StateMatrix: Story = { ), }; +// Deliberately hands the Dropdown no `value`: the keyboard has to finish its own commit - close the +// overlay and settle the filter text on the chosen option - without an application feeding the +// emitted value back in. +const UnboundFilteredDropdown = () => { + const [committed, setCommitted] = useState(null); + return ( +
+ + aria-label='Role' + options={roles} + optionLabel='label' + optionValue='value' + placeholder='Select a role' + filter + filterPlaceholder='Find a role' + onChange={setCommitted} + /> + Committed: {committed ?? 'none'} +
+ ); +}; + +export const FilteredKeyboardCommit: Story = { + render: () => , + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const filter = canvas.getByRole('combobox', { name: 'Role' }); + await userEvent.click(filter); + await userEvent.keyboard('dev'); + const listbox = await within(document.body).findByRole('listbox'); + await expect(within(listbox).getAllByRole('option')).toHaveLength(1); + await userEvent.keyboard('{ArrowDown}'); + await expect(filter.getAttribute('aria-activedescendant')).toBe( + within(listbox).getByRole('option').id, + ); + await userEvent.keyboard('{Enter}'); + await expect(canvas.getByText('Committed: developer')).toBeVisible(); + await expect(filter).toHaveValue('Developer'); + await expect(filter).toHaveAttribute('aria-expanded', 'false'); + await expect(within(document.body).queryByRole('listbox')).toBeNull(); + }, +}; + export const FilteredAndOpen: Story = { render: () => , play: async ({ canvasElement }) => { diff --git a/Source/Dropdown/DropdownImplementation.tsx b/Source/Dropdown/DropdownImplementation.tsx index 6acf81ab..942a3862 100644 --- a/Source/Dropdown/DropdownImplementation.tsx +++ b/Source/Dropdown/DropdownImplementation.tsx @@ -1,8 +1,8 @@ // Copyright (c) Cratis. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -import { useCallback, useState } from 'react'; -import type { JSX, Key } from 'react'; +import { useCallback, useContext, useEffect, useRef, useState } from 'react'; +import type { JSX, Key, MouseEvent, RefObject } from 'react'; import { Button as AriaButton, ListBox, @@ -14,6 +14,7 @@ import { import { Button as ComboBoxButton, ComboBox, + ComboBoxStateContext, ComboBoxValue, Input, ListBox as ComboBoxListBox, @@ -91,6 +92,30 @@ const resolveOptions = ( const classNames = (...values: Array) => values.filter(Boolean).join(' '); +/** The part of the ComboBox lifecycle a commit has to reach, from outside the ComboBox subtree. */ +interface CommittableComboBox { + close: () => void; +} + +/** + * Publishes the ComboBox state to the commit handler, which is declared outside the ComboBox and so + * cannot read the context itself. Renders nothing. + */ +const ComboBoxCommitBridge = ({ + handle, +}: { + handle: RefObject; +}) => { + const state = useContext(ComboBoxStateContext); + useEffect(() => { + handle.current = state; + return () => { + handle.current = null; + }; + }); + return null; +}; + const renderTriggerWithOpenState = (props: JSX.IntrinsicElements['button']) => ( @@ -686,12 +735,7 @@ export const DropdownImplementation = ({ data-cratis-part='clear' data-disabled={disabled || undefined} aria-label={clearSelectionLabel} - onClick={(event) => - onChange?.(null as T, { - source: 'user', - nativeEvent: event.nativeEvent, - }) - } + onClick={clearSelection} > diff --git a/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_a_filter_narrows_the_options.tsx b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_a_filter_narrows_the_options.tsx new file mode 100644 index 00000000..e925cfa1 --- /dev/null +++ b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_a_filter_narrows_the_options.tsx @@ -0,0 +1,93 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// @vitest-environment jsdom + +import { expect } from 'chai'; +import { afterEach, describe, it } from 'vitest'; +import { unmountPrimitive } from '../../../Common/for_Primitives/given/a_primitive_dom'; +import { + filterText, + focusDropdown, + isExpanded, + listbox, + mountDropdown, + options, + pressKey, + selectedState, + typeIntoFilter, + type DropdownBinding, + type MountedDropdown, +} from './given/a_dropdown'; + +// The keyboard owes the same finished commit whether or not the consuming application feeds the +// emitted value straight back, so every arrow-key case runs against both bindings. +const bindings: DropdownBinding[] = ['accepts', 'never answers']; + +describe('when committing an option with the keyboard and a filter narrows the options', () => { + let mounted: MountedDropdown | undefined; + + afterEach(async () => { + if (mounted) await unmountPrimitive(mounted); + mounted = undefined; + }); + + for (const binding of bindings) { + it(`should commit the option ArrowDown made active when the consumer ${binding} the value`, async () => { + mounted = await mountDropdown({ binding }); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'front'); + + expect(options().map((option) => option.textContent)).to.deep.equal([ + 'Frontend developer', + ]); + + await pressKey('ArrowDown', mounted.control); + expect(mounted.control.getAttribute('aria-activedescendant')).to.equal( + options()[0].id, + ); + + await pressKey('Enter', mounted.control); + + expect(mounted.changes).to.deep.equal(['frontend']); + expect(filterText(mounted)).to.equal('Frontend developer'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal('true'); + }); + + it(`should commit the option ArrowUp made active when the consumer ${binding} the value`, async () => { + mounted = await mountDropdown({ binding }); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'developer'); + + expect(options().map((option) => option.textContent)).to.deep.equal([ + 'Backend developer', + 'Frontend developer', + ]); + + await pressKey('ArrowUp', mounted.control); + expect(mounted.control.getAttribute('aria-activedescendant')).to.equal( + options()[1].id, + ); + + await pressKey('Enter', mounted.control); + + expect(mounted.changes).to.deep.equal(['frontend']); + expect(filterText(mounted)).to.equal('Frontend developer'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal('true'); + }); + } + + it('should report the committed value exactly once', async () => { + mounted = await mountDropdown(); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'design'); + await pressKey('ArrowDown', mounted.control); + await pressKey('Enter', mounted.control); + + expect(mounted.changes).to.deep.equal(['designer']); + }); +}); diff --git a/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_no_filter_is_in_play.tsx b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_no_filter_is_in_play.tsx new file mode 100644 index 00000000..f20d751b --- /dev/null +++ b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_no_filter_is_in_play.tsx @@ -0,0 +1,64 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// @vitest-environment jsdom + +import { expect } from 'chai'; +import { afterEach, describe, it } from 'vitest'; +import { unmountPrimitive } from '../../../Common/for_Primitives/given/a_primitive_dom'; +import { + isExpanded, + listbox, + mountDropdown, + options, + pressKey, + selectedState, + type DropdownBinding, + type MountedDropdown, +} from './given/a_dropdown'; + +const bindings: DropdownBinding[] = ['accepts', 'never answers']; + +describe('when committing an option with the keyboard and no filter is in play', () => { + let mounted: MountedDropdown | undefined; + + afterEach(async () => { + if (mounted) await unmountPrimitive(mounted); + mounted = undefined; + }); + + for (const binding of bindings) { + it(`should open on ArrowDown and commit the active option with Enter when the consumer ${binding} the value`, async () => { + mounted = await mountDropdown({ filter: false, binding }); + expect(mounted.control.textContent).to.contain('Select a role'); + + await pressKey('ArrowDown', mounted.control); + expect(options().map((option) => option.textContent)).to.deep.equal([ + 'Backend developer', + 'Frontend developer', + 'Designer', + ]); + + await pressKey('ArrowDown'); + await pressKey('Enter'); + + expect(mounted.changes).to.deep.equal(['frontend']); + expect(mounted.control.textContent).to.contain('Frontend developer'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal('true'); + }); + } + + it('should leave the selection alone when Escape dismisses the list', async () => { + mounted = await mountDropdown({ filter: false }); + await pressKey('ArrowDown', mounted.control); + await pressKey('Escape'); + + expect(mounted.changes).to.deep.equal([]); + expect(mounted.control.textContent).to.contain('Select a role'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal(null); + }); +}); diff --git a/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_consumer_does_not_take_the_value_back.tsx b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_consumer_does_not_take_the_value_back.tsx new file mode 100644 index 00000000..67d0f9fc --- /dev/null +++ b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_consumer_does_not_take_the_value_back.tsx @@ -0,0 +1,58 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// @vitest-environment jsdom + +import { expect } from 'chai'; +import { afterEach, describe, it } from 'vitest'; +import { unmountPrimitive } from '../../../Common/for_Primitives/given/a_primitive_dom'; +import { + filterText, + focusDropdown, + isExpanded, + listbox, + mountDropdown, + pressKey, + selectedState, + typeIntoFilter, + type MountedDropdown, +} from './given/a_dropdown'; + +describe('when committing an option with the keyboard and the consumer does not take the value back', () => { + let mounted: MountedDropdown | undefined; + + afterEach(async () => { + if (mounted) await unmountPrimitive(mounted); + mounted = undefined; + }); + + it('should still commit, close and show the label when no value prop answers', async () => { + mounted = await mountDropdown({ binding: 'never answers' }); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'front'); + await pressKey('ArrowDown', mounted.control); + await pressKey('Enter', mounted.control); + + expect(mounted.changes).to.deep.equal(['frontend']); + expect(filterText(mounted)).to.equal('Frontend developer'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal('true'); + }); + + it('should let a later value from the consumer win over what was committed', async () => { + mounted = await mountDropdown({ binding: 'rewrites' }); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'front'); + await pressKey('ArrowDown', mounted.control); + await pressKey('Enter', mounted.control); + + // The consumer answered with a value no option carries, so nothing is selected - but the + // overlay is the Dropdown's to close, and the stale filter text is never left behind. + expect(mounted.changes).to.deep.equal(['frontend']); + expect(filterText(mounted)).to.equal(''); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal(null); + }); +}); diff --git a/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_list_is_dismissed_instead.tsx b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_list_is_dismissed_instead.tsx new file mode 100644 index 00000000..d3e9d3a4 --- /dev/null +++ b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/and_the_list_is_dismissed_instead.tsx @@ -0,0 +1,58 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// @vitest-environment jsdom + +import { expect } from 'chai'; +import { afterEach, describe, it } from 'vitest'; +import { unmountPrimitive } from '../../../Common/for_Primitives/given/a_primitive_dom'; +import { + filterText, + focusDropdown, + isExpanded, + listbox, + mountDropdown, + pressKey, + selectedState, + typeIntoFilter, + type MountedDropdown, +} from './given/a_dropdown'; + +describe('when committing an option with the keyboard and the list is dismissed instead', () => { + let mounted: MountedDropdown | undefined; + + afterEach(async () => { + if (mounted) await unmountPrimitive(mounted); + mounted = undefined; + }); + + it('should leave an empty selection alone when Escape dismisses the active option', async () => { + mounted = await mountDropdown(); + await focusDropdown(mounted); + await typeIntoFilter(mounted, 'front'); + await pressKey('ArrowDown', mounted.control); + await pressKey('Escape', mounted.control); + + expect(mounted.changes).to.deep.equal([]); + expect(filterText(mounted)).to.equal(''); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal(null); + }); + + it('should restore the standing selection when Escape dismisses the active option', async () => { + mounted = await mountDropdown({ initialValue: 'backend' }); + await focusDropdown(mounted); + expect(filterText(mounted)).to.equal('Backend developer'); + + await typeIntoFilter(mounted, 'front'); + await pressKey('ArrowDown', mounted.control); + await pressKey('Escape', mounted.control); + + expect(mounted.changes).to.deep.equal([]); + expect(filterText(mounted)).to.equal('Backend developer'); + expect(isExpanded(mounted)).to.equal('false'); + expect(listbox()).to.equal(null); + expect(selectedState(mounted)).to.equal('true'); + }); +}); diff --git a/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/given/a_dropdown.tsx b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/given/a_dropdown.tsx new file mode 100644 index 00000000..a2c367f7 --- /dev/null +++ b/Source/Dropdown/for_Dropdown/when_committing_an_option_with_the_keyboard/given/a_dropdown.tsx @@ -0,0 +1,123 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +import { act, useState } from 'react'; +import { CratisComponentsProvider } from '../../../../Common/CratisComponentsProvider'; +import { + mountPrimitive, + setNativeValue, + type MountedPrimitive, +} from '../../../../Common/for_Primitives/given/a_primitive_dom'; +import { Dropdown } from '../../../Dropdown'; + +export const roles = [ + { label: 'Backend developer', value: 'backend' }, + { label: 'Frontend developer', value: 'frontend' }, + { label: 'Designer', value: 'designer' }, +]; + +/** + * How the consuming application answers `onChange`. `accepts` is the documented round trip; + * the other two are the bindings a real application arrives at without meaning to, and the + * Dropdown owes them the same committed selection, closed overlay and settled filter text. + */ +export type DropdownBinding = 'accepts' | 'never answers' | 'rewrites'; + +export interface MountedDropdown extends MountedPrimitive { + changes: Array; + control: HTMLElement; +} + +interface DropdownFixtureOptions { + binding?: DropdownBinding; + initialValue?: string | null; + filter?: boolean; +} + +const ensureBrowserGlobals = () => { + (globalThis as unknown as { ResizeObserver: unknown }).ResizeObserver ??= class { + observe() { + return undefined; + } + unobserve() { + return undefined; + } + disconnect() { + return undefined; + } + }; +}; + +export const mountDropdown = async ({ + binding = 'accepts', + initialValue = null, + filter = true, +}: DropdownFixtureOptions = {}): Promise => { + ensureBrowserGlobals(); + const changes: Array = []; + + const Host = () => { + const [value, setValue] = useState(initialValue); + return ( + + + aria-label='Role' + placeholder='Select a role' + filterPlaceholder='Find a role' + filter={filter} + options={roles} + optionLabel='label' + optionValue='value' + {...(binding === 'never answers' ? {} : { value })} + onChange={(next) => { + changes.push(next); + if (binding === 'accepts') setValue(next); + if (binding === 'rewrites') + setValue(next === null ? null : next.toUpperCase()); + }} + /> + + ); + }; + + const mounted = await mountPrimitive(); + const control = mounted.container.querySelector( + filter ? '[data-cratis-part="filter"]' : '[data-cratis-part="trigger"]', + ); + if (!control) throw new Error('Dropdown fixture did not render.'); + return { ...mounted, changes, control }; +}; + +export const focusDropdown = async (mounted: MountedDropdown) => { + await act(async () => { + mounted.control.focus(); + mounted.control.dispatchEvent(new FocusEvent('focus', { bubbles: false })); + mounted.control.dispatchEvent(new FocusEvent('focusin', { bubbles: true })); + await Promise.resolve(); + }); +}; + +export const typeIntoFilter = async (mounted: MountedDropdown, text: string) => + setNativeValue(mounted.control as HTMLInputElement, text); + +export const pressKey = async (key: string, element?: HTMLElement) => { + const target = element ?? (document.activeElement as HTMLElement); + await act(async () => { + target.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true })); + target.dispatchEvent(new KeyboardEvent('keyup', { key, bubbles: true })); + await Promise.resolve(); + }); +}; + +export const filterText = (mounted: MountedDropdown) => + (mounted.control as HTMLInputElement).value; +export const isExpanded = (mounted: MountedDropdown) => + mounted.control.getAttribute('aria-expanded'); +export const listbox = () => + document.querySelector('[data-cratis-part="listbox"]'); +export const options = () => + Array.from(document.querySelectorAll('[data-cratis-part="option"]')); +export const selectedState = (mounted: MountedDropdown) => + mounted.container + .querySelector('[data-cratis-part="root"]') + ?.getAttribute('data-selected'); diff --git a/Storybook/scripts/verify-storybook-indexes.mjs b/Storybook/scripts/verify-storybook-indexes.mjs index bbdedf94..76acaf1e 100644 --- a/Storybook/scripts/verify-storybook-indexes.mjs +++ b/Storybook/scripts/verify-storybook-indexes.mjs @@ -41,8 +41,8 @@ for (const adapter of inventory.adapters) { const entries = Object.values(index.entries ?? {}); const storyIds = entries.filter(entry => entry.type === 'story').map(entry => entry.id).sort(); const docsIds = entries.filter(entry => entry.type === 'docs').map(entry => entry.id).sort(); - if (storyIds.length !== 327 || docsIds.length !== 74) { - throw new Error(`${adapter.metadata.id} indexed ${storyIds.length} stories and ${docsIds.length} autodocs pages; expected 327 and 74.`); + if (storyIds.length !== 328 || docsIds.length !== 74) { + throw new Error(`${adapter.metadata.id} indexed ${storyIds.length} stories and ${docsIds.length} autodocs pages; expected 328 and 74.`); } canonicalStoryIds ??= storyIds; canonicalDocsIds ??= docsIds; @@ -86,8 +86,8 @@ const { slotOwningModules, matrixStoryIds } = computeRendererMatrixScope({ if (slotOwningModules.size !== 14) { throw new Error(`Expected 14 slot-owning modules (the stable nine-slot presentation profile plus experimental slots), found ${slotOwningModules.size}.`); } -if (matrixStoryIds.size !== 175) { - throw new Error(`Expected 175 stories to require the full renderer matrix, found ${matrixStoryIds.size}. If this is an intentional consequence of adding or removing a slotted/composite component, update this pinned count.`); +if (matrixStoryIds.size !== 176) { + throw new Error(`Expected 176 stories to require the full renderer matrix, found ${matrixStoryIds.size}. If this is an intentional consequence of adding or removing a slotted/composite component, update this pinned count.`); } const appearances = 2;