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 | 65 ++++--- .../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, 750 insertions(+), 54 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 e8414b8..752eaae 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 89c1170..18d9b4a 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 934fb85..b6f2212 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 3aae0f7..0dc2f80 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) + + 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 71fef36..25f4df3 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 1c3702d..94445dd 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 0d267e6..63f4c26 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 600e605..3e11ac5 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 32152f3..5d34de0 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 0000000..65389c7 --- /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 0000000..0172fd1 --- /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 0000000..9e03c93 --- /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 d1f1408..e7c841e 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 78270eb..2d503d9 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 -