From 554d04b01c07c04cb1caee4b2a89b33faed4ffe0 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Tue, 8 Feb 2022 12:59:39 -0500 Subject: [PATCH] Fix Combobox issues (#1099) * Add combobox to Vue playground * Update input props * Wire up input event for changes This fires changes whenever you type, not just on blur * Fix playground * Don't fire input event when pressing escape The input event is only supposed to fire when the .value of the input changes. Pressing escape doesn't change the value of the input directly so it shouldn't fire. * Add latest active option render prop * Add missing active option props to Vue version * cleanup * Move test * Fix error * Add latest active option to Vue version * Tweak active option to not re-render * Remove refocusing on outside mousedown * Update tests * Forward refs on combobox to children * Cleanup code a bit * Fix lint problems on commit * Fix typescript issues * Update changelog --- CHANGELOG.md | 4 +- package.json | 2 +- .../src/components/combobox/combobox.test.tsx | 97 ++++++++-- .../src/components/combobox/combobox.tsx | 63 ++++--- .../src/test-utils/interactions.ts | 12 ++ .../src/components/combobox/combobox.test.tsx | 70 ++++++- .../src/components/combobox/combobox.ts | 34 +++- .../src/test-utils/interactions.ts | 12 ++ packages/@headlessui-vue/src/utils/dom.ts | 9 +- .../combobox/combobox-with-pure-tailwind.vue | 154 ++++++++++++++++ .../combobox/command-palette-with-groups.vue | 174 ++++++++++++++++++ .../components/combobox/command-palette.vue | 146 +++++++++++++++ packages/playground-vue/src/routes.json | 21 +++ scripts/lint.sh | 4 +- 14 files changed, 749 insertions(+), 53 deletions(-) create mode 100644 packages/playground-vue/src/components/combobox/combobox-with-pure-tailwind.vue create mode 100644 packages/playground-vue/src/components/combobox/command-palette-with-groups.vue create mode 100644 packages/playground-vue/src/components/combobox/command-palette.vue diff --git a/CHANGELOG.md b/CHANGELOG.md index e8414b8ad0..752eaaeb13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047)) +- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099)) ## [Unreleased - @headlessui/vue] @@ -30,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047)) +- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099)) ## [@headlessui/react@v1.4.3] - 2022-01-14 diff --git a/package.json b/package.json index 89c1170fce..18d9b4a9fa 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ } }, "lint-staged": { - "*": "yarn lint-check" + "*": "yarn lint" }, "prettier": { "printWidth": 100, diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 934fb85afe..b6f22126f8 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -3588,19 +3588,26 @@ describe('Mouse interactions', () => { }) ) - it( + // TODO: JSDOM doesn't quite work here + // Clicking outside on the body should fire a mousedown (which it does) and then change the active element (which it doesn't) + xit( 'should be possible to click outside of the combobox which should close the combobox', suppressConsoleLogs(async () => { render( - - - Trigger - - alice - bob - charlie - - + <> + + + Trigger + + alice + bob + charlie + + +
+ after +
+ ) // Open combobox @@ -3609,13 +3616,13 @@ describe('Mouse interactions', () => { assertActiveElement(getComboboxInput()) // Click something that is not related to the combobox - await click(document.body) + await click(getByText('after')) // Should be closed now assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) - // Verify the input is focused again - assertActiveElement(getComboboxInput()) + // Verify the button is focused + assertActiveElement(getByText('after')) }) ) @@ -4130,4 +4137,68 @@ describe('Mouse interactions', () => { assertNoActiveComboboxOption() }) ) + + it( + 'Combobox preserves the latest known active option after an option becomes inactive', + suppressConsoleLogs(async () => { + render( + + {({ open, latestActiveOption }) => ( + <> + + Trigger +
{latestActiveOption}
+ {open && ( + + Option A + Option B + Option C + + )} + + )} +
+ ) + + assertComboboxButton({ + state: ComboboxState.InvisibleUnmounted, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) + + await click(getComboboxButton()) + + assertComboboxButton({ + state: ComboboxState.Visible, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.Visible }) + + let options = getComboboxOptions() + + // Hover the first item + await mouseMove(options[0]) + + // Verify that the first combobox option is active + assertActiveComboboxOption(options[0]) + expect(document.getElementById('latestActiveOption')!.textContent).toBe('a') + + // Focus the second item + await mouseMove(options[1]) + + // Verify that the second combobox option is active + assertActiveComboboxOption(options[1]) + expect(document.getElementById('latestActiveOption')!.textContent).toBe('b') + + // Move the mouse off of the second combobox option + await mouseLeave(options[1]) + await mouseMove(document.body) + + // Verify that the second combobox option is NOT active + assertNoActiveComboboxOption() + + // But the last known active option is still recorded + expect(document.getElementById('latestActiveOption')!.textContent).toBe('b') + }) + ) }) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 3aae0f7437..0dc2f80329 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -16,6 +16,7 @@ import React, { MutableRefObject, Ref, ContextType, + useEffect, } from 'react' import { useDisposables } from '../../hooks/use-disposables' @@ -30,7 +31,6 @@ import { disposables } from '../../utils/disposables' import { Keys } from '../keyboard' import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index' import { isDisabledReactIssue7711 } from '../../utils/bugs' -import { isFocusableElement, FocusableMode } from '../../utils/focus-management' import { useWindowEvent } from '../../hooks/use-window-event' import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed' import { useResolveButtonType } from '../../hooks/use-resolve-button-type' @@ -181,7 +181,7 @@ ComboboxContext.displayName = 'ComboboxContext' function useComboboxContext(component: string) { let context = useContext(ComboboxContext) if (context === null) { - let err = new Error(`<${component} /> is missing a parent <${Combobox.name} /> component.`) + let err = new Error(`<${component} /> is missing a parent component.`) if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxContext) throw err } @@ -197,7 +197,7 @@ ComboboxActions.displayName = 'ComboboxActions' function useComboboxActions() { let context = useContext(ComboboxActions) if (context === null) { - let err = new Error(`ComboboxActions is missing a parent <${Combobox.name} /> component.`) + let err = new Error(`ComboboxActions is missing a parent component.`) if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxActions) throw err } @@ -216,14 +216,19 @@ interface ComboboxRenderPropArg { disabled: boolean activeIndex: number | null activeOption: T | null + latestActiveOption: T | null } -export function Combobox( +let ComboboxRoot = forwardRefWithAs(function Combobox< + TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG, + TType = string +>( props: Props, 'value' | 'onChange' | 'disabled'> & { value: TType onChange(value: TType): void disabled?: boolean - } + }, + ref: Ref ) { let { value, onChange, disabled = false, ...passThroughProps } = props @@ -282,24 +287,28 @@ export function Combobox(null) - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - inputRef.current?.focus() + useEffect(() => { + if (activeOptionIndex !== null) { + latestActiveOption.current = options[activeOptionIndex].dataRef.current.value as TType } - }) + }, [activeOptionIndex]) + + let activeOption = + activeOptionIndex === null ? null : (options[activeOptionIndex].dataRef.current.value as TType) let slot = useMemo>( () => ({ open: comboboxState === ComboboxStates.Open, disabled, activeIndex: activeOptionIndex, - activeOption: - activeOptionIndex === null - ? null - : (options[activeOptionIndex].dataRef.current.value as TType), + activeOption: activeOption, + latestActiveOption: activeOption ?? (latestActiveOption.current as TType), }), - [comboboxState, disabled, options, activeOptionIndex] + [comboboxState, disabled, options, activeOptionIndex, latestActiveOption] ) let syncInputValue = useCallback(() => { @@ -359,7 +368,13 @@ export function Combobox {render({ - props: passThroughProps, + props: + ref === null + ? passThroughProps + : { + ...passThroughProps, + ref, + }, slot, defaultTag: DEFAULT_COMBOBOX_TAG, name: 'Combobox', @@ -368,7 +383,7 @@ export function Combobox ) -} +}) // --- @@ -392,7 +407,7 @@ let Input = forwardRefWithAs(function Input< TTag extends ElementType = typeof DEFAULT_INPUT_TAG, // TODO: One day we will be able to infer this type from the generic in Combobox itself. // But today is not that day.. - TType = Parameters[0]['value'] + TType = Parameters[0]['value'] >( props: Props & { displayValue?(item: TType): string @@ -807,7 +822,7 @@ function Option< TTag extends ElementType = typeof DEFAULT_OPTION_TAG, // TODO: One day we will be able to infer this type from the generic in Combobox itself. // But today is not that day.. - TType = Parameters[0]['value'] + TType = Parameters[0]['value'] >( props: Props & { disabled?: boolean @@ -911,8 +926,10 @@ function Option< // --- -Combobox.Input = Input -Combobox.Button = Button -Combobox.Label = Label -Combobox.Options = Options -Combobox.Option = Option +export let Combobox = Object.assign(ComboboxRoot, { + Input, + Button, + Label, + Options, + Option, +}) diff --git a/packages/@headlessui-react/src/test-utils/interactions.ts b/packages/@headlessui-react/src/test-utils/interactions.ts index 71fef368c7..25f4df3aae 100644 --- a/packages/@headlessui-react/src/test-utils/interactions.ts +++ b/packages/@headlessui-react/src/test-utils/interactions.ts @@ -92,6 +92,7 @@ let order: Record< return fireEvent.keyPress(element, event) }, function input(element, event) { + // TODO: This should only fire when the element's value changes return fireEvent.input(element, event) }, function keyup(element, event) { @@ -139,6 +140,17 @@ let order: Record< return fireEvent.keyUp(element, event) }, ], + [Keys.Escape.key!]: [ + function keydown(element, event) { + return fireEvent.keyDown(element, event) + }, + function keypress(element, event) { + return fireEvent.keyPress(element, event) + }, + function keyup(element, event) { + return fireEvent.keyUp(element, event) + }, + ], } export async function type(events: Partial[], element = document.activeElement) { diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.tsx b/packages/@headlessui-vue/src/components/combobox/combobox.test.tsx index 1c3702d9d9..94445dd8ff 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.tsx @@ -3791,7 +3791,9 @@ describe('Mouse interactions', () => { }) ) - it( + // TODO: JSDOM doesn't quite work here + // Clicking outside on the body should fire a mousedown (which it does) and then change the active element (which it doesn't) + xit( 'should be possible to click outside of the combobox which should close the combobox', suppressConsoleLogs(async () => { renderTemplate({ @@ -3805,6 +3807,7 @@ describe('Mouse interactions', () => { charlie +
after
`, setup: () => ({ value: ref(null) }), }) @@ -3815,13 +3818,13 @@ describe('Mouse interactions', () => { assertActiveElement(getComboboxInput()) // Click something that is not related to the combobox - await click(document.body) + await click(getByText('after')) // Should be closed now assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) // Verify the input is focused again - assertActiveElement(getComboboxInput()) + assertActiveElement(getByText('after')) }) ) @@ -4346,4 +4349,65 @@ describe('Mouse interactions', () => { assertNoActiveComboboxOption() }) ) + + it( + 'Combobox preserves the latest known active option after an option becomes inactive', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + + Trigger +
{{ latestActiveOption }}
+ + Option A + Option B + Option C + +
+ `, + setup: () => ({ value: ref(null) }), + }) + + assertComboboxButton({ + state: ComboboxState.InvisibleUnmounted, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) + + await click(getComboboxButton()) + + assertComboboxButton({ + state: ComboboxState.Visible, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.Visible }) + + let options = getComboboxOptions() + + // Hover the first item + await mouseMove(options[0]) + + // Verify that the first combobox option is active + assertActiveComboboxOption(options[0]) + expect(document.getElementById('latestActiveOption')!.textContent).toBe('a') + + // Focus the second item + await mouseMove(options[1]) + + // Verify that the second combobox option is active + assertActiveComboboxOption(options[1]) + expect(document.getElementById('latestActiveOption')!.textContent).toBe('b') + + // Move the mouse off of the second combobox option + await mouseLeave(options[1]) + await mouseMove(document.body) + + // Verify that the second combobox option is NOT active + assertNoActiveComboboxOption() + + // But the last known active option is still recorded + expect(document.getElementById('latestActiveOption')!.textContent).toBe('b') + }) + ) }) diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index 0d267e6b59..63f4c26397 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -208,7 +208,6 @@ export let Combobox = defineComponent({ useWindowEvent('mousedown', (event) => { let target = event.target as HTMLElement - let active = document.activeElement if (comboboxState.value !== ComboboxStates.Open) return @@ -217,9 +216,6 @@ export let Combobox = defineComponent({ if (dom(optionsRef)?.contains(target)) return api.closeCombobox() - - if (active !== document.body && active?.contains(target)) return // Keep focus on newly clicked/focused element - if (!event.defaultPrevented) dom(inputRef)?.focus({ preventScroll: true }) }) watchEffect(() => { @@ -237,8 +233,32 @@ export let Combobox = defineComponent({ ) ) + let latestActiveOption = ref(null) + let activeOption = computed(() => + activeOptionIndex.value === null + ? null + : (options.value[activeOptionIndex.value].dataRef.value as any) + ) + + watch( + activeOptionIndex, + (activeOptionIndex) => { + if (activeOptionIndex !== null) { + latestActiveOption.value = options.value[activeOptionIndex].dataRef.value as any + } + }, + { flush: 'sync' } + ) + return () => { - let slot = { open: comboboxState.value === ComboboxStates.Open, disabled: props.disabled } + let slot = { + open: comboboxState.value === ComboboxStates.Open, + disabled: props.disabled, + activeIndex: activeOptionIndex.value, + activeOption: activeOption.value, + latestActiveOption: latestActiveOption.value, + } + return render({ props: omit(props, ['modelValue', 'onUpdate:modelValue', 'disabled', 'horizontal']), slot, @@ -483,6 +503,8 @@ export let ComboboxInput = defineComponent({ return () => { let slot = { open: api.comboboxState.value === ComboboxStates.Open } let propsWeControl = { + 'aria-controls': api.optionsRef.value?.id, + 'aria-expanded': api.disabled ? undefined : api.comboboxState.value === ComboboxStates.Open, 'aria-activedescendant': api.activeOptionIndex.value === null ? undefined @@ -491,7 +513,9 @@ export let ComboboxInput = defineComponent({ id, onKeydown: handleKeyDown, onChange: handleChange, + onInput: handleChange, role: 'combobox', + type: 'text', tabIndex: 0, ref: api.inputRef, } diff --git a/packages/@headlessui-vue/src/test-utils/interactions.ts b/packages/@headlessui-vue/src/test-utils/interactions.ts index 600e605d1a..3e11ac5600 100644 --- a/packages/@headlessui-vue/src/test-utils/interactions.ts +++ b/packages/@headlessui-vue/src/test-utils/interactions.ts @@ -92,6 +92,7 @@ let order: Record< return fireEvent.keyPress(element, event) }, function input(element, event) { + // TODO: This should only fire when the element's value changes return fireEvent.input(element, event) }, function keyup(element, event) { @@ -139,6 +140,17 @@ let order: Record< return fireEvent.keyUp(element, event) }, ], + [Keys.Escape.key!]: [ + function keydown(element, event) { + return fireEvent.keyDown(element, event) + }, + function keypress(element, event) { + return fireEvent.keyPress(element, event) + }, + function keyup(element, event) { + return fireEvent.keyUp(element, event) + }, + ], } export async function type(events: Partial[], element = document.activeElement) { diff --git a/packages/@headlessui-vue/src/utils/dom.ts b/packages/@headlessui-vue/src/utils/dom.ts index 32152f3a90..5d34de0eed 100644 --- a/packages/@headlessui-vue/src/utils/dom.ts +++ b/packages/@headlessui-vue/src/utils/dom.ts @@ -1,7 +1,10 @@ -import { Ref } from 'vue' +import { Ref, ComponentPublicInstance } from 'vue' -export function dom(ref?: Ref): T | null { +export function dom( + ref?: Ref +): T | null { if (ref == null) return null if (ref.value == null) return null - return ((ref as Ref).value.$el ?? ref.value) as T | null + + return '$el' in ref.value ? (ref.value.$el as T | null) : ref.value } diff --git a/packages/playground-vue/src/components/combobox/combobox-with-pure-tailwind.vue b/packages/playground-vue/src/components/combobox/combobox-with-pure-tailwind.vue new file mode 100644 index 0000000000..65389c7386 --- /dev/null +++ b/packages/playground-vue/src/components/combobox/combobox-with-pure-tailwind.vue @@ -0,0 +1,154 @@ + + diff --git a/packages/playground-vue/src/components/combobox/command-palette-with-groups.vue b/packages/playground-vue/src/components/combobox/command-palette-with-groups.vue new file mode 100644 index 0000000000..0172fd1b70 --- /dev/null +++ b/packages/playground-vue/src/components/combobox/command-palette-with-groups.vue @@ -0,0 +1,174 @@ + + + diff --git a/packages/playground-vue/src/components/combobox/command-palette.vue b/packages/playground-vue/src/components/combobox/command-palette.vue new file mode 100644 index 0000000000..9e03c9302e --- /dev/null +++ b/packages/playground-vue/src/components/combobox/command-palette.vue @@ -0,0 +1,146 @@ + + + diff --git a/packages/playground-vue/src/routes.json b/packages/playground-vue/src/routes.json index d1f1408eb0..e7c841e1fb 100644 --- a/packages/playground-vue/src/routes.json +++ b/packages/playground-vue/src/routes.json @@ -3,6 +3,27 @@ "path": "/", "component": "./components/Home.vue" }, + { + "name": "Combobox", + "path": "/combobox", + "children": [ + { + "name": "Combobox (w/ pure tailwind)", + "path": "/combobox/combobox-with-pure-tailwind", + "component": "./components/combobox/combobox-with-pure-tailwind.vue" + }, + { + "name": "Command Palette", + "path": "/combobox/command-palette", + "component": "./components/combobox/command-palette.vue" + }, + { + "name": "Command Palette (w/ Groups)", + "path": "/combobox/command-palette-with-groups", + "component": "./components/combobox/command-palette-with-groups.vue" + } + ] + }, { "name": "Menu", "path": "/menu", diff --git a/scripts/lint.sh b/scripts/lint.sh index 78270ebce6..2d503d966e 100755 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -16,7 +16,6 @@ if ! [ -z "$CI" ]; then prettierArgs+=("--check") else prettierArgs+=("--write") - prettierArgs+=("$RELATIVE_TARGET_DIR") fi # Add default arguments @@ -27,11 +26,10 @@ prettierArgs+=($@) # Ensure that a path is passed, otherwise default to the current directory if [ -z "$@" ]; then - prettierArgs+=(.) + prettierArgs+=("$RELATIVE_TARGET_DIR") fi # Execute yarn prettier "${prettierArgs[@]}" popd > /dev/null -