From 88b068cff1ad8e9b320972713da647e296076187 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 8 Aug 2023 19:09:01 +0200 Subject: [PATCH] Improve resetting values when using the `nullable` prop on the `Combobox` component (#2660) * move `nullable` handling to `onChange` of `Combobox.Input` itself We were specifically handling backspace/delete keys to verify if the `Combobox.Input` becomes empty then we can clear the value if we are in single value and in nullable mode. However, this doesn't capture other ways of clearing the `Combobox.Input`, for example when use `cmd+x` or `ctrl+y` in the input. Moving the logic, gives us some of these cases for free. * ensure pressing `escape` also clears the input in nullable, single value mode without an active value * adjust test to ensure we don't have a selected option instead of an active option We still will have an active option (because we default to the first option if nothing is active while the combobox is open). But since we cleared the value when using the `nullable` prop, then it means the `selected` option should be cleared. * ensure `input` event is fired when firing keydown events * ensure `defaultToFirstOption` is always set when going to an option We recently made a Vue improvement that delayed the going to an option, but this also included a bug where the `defaultToFirstOption` was not set at the right time anymore. * update changelog * fix `than` / `then` typo --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/combobox/combobox.test.tsx | 4 +- .../src/components/combobox/combobox.tsx | 48 +++++++++++------ .../src/test-utils/interactions.ts | 4 +- packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/combobox/combobox.test.ts | 4 +- .../src/components/combobox/combobox.ts | 54 ++++++++++++------- .../transitions/utils/transition.ts | 2 +- .../src/test-utils/interactions.ts | 4 +- 9 files changed, 78 insertions(+), 44 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 54259325c..624f22081 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don't assume `` components are available when setting the next index ([#2642](https://github.com/tailwindlabs/headlessui/pull/2642)) - Fix incorrectly focused `Combobox.Input` component on page load ([#2654](https://github.com/tailwindlabs/headlessui/pull/2654)) - Ensure `appear` works using the `Transition` component (even when used with SSR) ([#2646](https://github.com/tailwindlabs/headlessui/pull/2646)) +- Improve resetting values when using the `nullable` prop on the `Combobox` component ([#2660](https://github.com/tailwindlabs/headlessui/pull/2660)) ## [1.7.16] - 2023-07-27 diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index eb57a61e5..e1f4b5580 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -2735,9 +2735,9 @@ describe('Keyboard interactions', () => { await press(Keys.Backspace) expect(getComboboxInput()?.value).toBe('') - // Verify that we don't have an active option anymore since we are in `nullable` mode + // Verify that we don't have an selected option anymore since we are in `nullable` mode assertNotActiveComboboxOption(options[1]) - assertNoActiveComboboxOption() + assertNoSelectedComboboxOption() // Verify that we saw the `null` change coming in expect(handleChange).toHaveBeenCalledTimes(1) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index d551fc920..5e7f38a1e 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -745,6 +745,14 @@ function InputFn< let d = useDisposables() + let clear = useEvent(() => { + actions.onChange(null) + if (data.optionsRef.current) { + data.optionsRef.current.scrollTop = 0 + } + actions.goToOption(Focus.Nothing) + }) + // When a `displayValue` prop is given, we should use it to transform the current selected // option(s) so that the format can be chosen by developers implementing this. This is useful if // your data is an object and you just want to pick a certain property or want to create a dynamic @@ -871,23 +879,6 @@ function InputFn< switch (event.key) { // Ref: https://www.w3.org/WAI/ARIA/apg/patterns/menu/#keyboard-interaction-12 - case Keys.Backspace: - case Keys.Delete: - if (data.mode !== ValueMode.Single) return - if (!data.nullable) return - - let input = event.currentTarget - d.requestAnimationFrame(() => { - if (input.value === '') { - actions.onChange(null) - if (data.optionsRef.current) { - data.optionsRef.current.scrollTop = 0 - } - actions.goToOption(Focus.Nothing) - } - }) - break - case Keys.Enter: isTyping.current = false if (data.comboboxState !== ComboboxState.Open) return @@ -981,6 +972,18 @@ function InputFn< if (data.optionsRef.current && !data.optionsPropsRef.current.static) { event.stopPropagation() } + + if (data.nullable && data.mode === ValueMode.Single) { + // We want to clear the value when the user presses escape if and only if the current + // value is not set (aka, they didn't select anything yet, or they cleared the input which + // caused the value to be set to `null`). If the current value is set, then we want to + // fallback to that value when we press escape (this part is handled in the watcher that + // syncs the value with the input field again). + if (data.value === null) { + clear() + } + } + return actions.closeCombobox() case Keys.Tab: @@ -1001,6 +1004,17 @@ function InputFn< // options while typing won't work at all because we are still in "composing" mode. onChange?.(event) + // When the value becomes empty in a single value mode while being nullable then we want to clear + // the option entirely. + // + // This is can happen when you press backspace, but also when you select all the text and press + // ctrl/cmd+x. + if (data.nullable && data.mode === ValueMode.Single) { + if (event.target.value === '') { + clear() + } + } + // Open the combobox to show the results based on what the user has typed actions.openCombobox() }) diff --git a/packages/@headlessui-react/src/test-utils/interactions.ts b/packages/@headlessui-react/src/test-utils/interactions.ts index 747ef523f..dd0298fd7 100644 --- a/packages/@headlessui-react/src/test-utils/interactions.ts +++ b/packages/@headlessui-react/src/test-utils/interactions.ts @@ -157,7 +157,9 @@ let order: Record< value: element.value.slice(0, -1), }), }) - return fireEvent.keyDown(element, ev) + + fireEvent.keyDown(element, ev) + return fireEvent.input(element, ev) } return fireEvent.keyDown(element, event) diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 0d08b0dad..2613dc8ae 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don't assume `` components are available when setting the next index ([#2642](https://github.com/tailwindlabs/headlessui/pull/2642)) - Improve SSR of the `Disclosure` component ([#2645](https://github.com/tailwindlabs/headlessui/pull/2645)) - Fix incorrectly focused `ComboboxInput` component on page load ([#2654](https://github.com/tailwindlabs/headlessui/pull/2654)) +- Improve resetting values when using the `nullable` prop on the `Combobox` component ([#2660](https://github.com/tailwindlabs/headlessui/pull/2660)) ## [1.7.15] - 2023-07-27 diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index a66d0930a..dedeeaca8 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -4554,9 +4554,9 @@ describe('Keyboard interactions', () => { await press(Keys.Backspace) expect(getComboboxInput()?.value).toBe('') - // Verify that we don't have an active option anymore since we are in `nullable` mode + // Verify that we don't have an selected option anymore since we are in `nullable` mode assertNotActiveComboboxOption(options[1]) - assertNoActiveComboboxOption() + assertNoSelectedComboboxOption() // Verify that we saw the `null` change coming in expect(handleChange).toHaveBeenCalledTimes(1) diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index 18baa4140..a5bec8643 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -281,13 +281,13 @@ export let Combobox = defineComponent({ comboboxState.value = ComboboxStates.Open }, goToOption(focus: Focus, id?: string, trigger?: ActivationTrigger) { + defaultToFirstOption.value = false + if (goToOptionRaf !== null) { cancelAnimationFrame(goToOptionRaf) } goToOptionRaf = requestAnimationFrame(() => { - defaultToFirstOption.value = false - if (props.disabled) return if ( optionsRef.value && @@ -707,6 +707,15 @@ export let ComboboxInput = defineComponent({ expose({ el: api.inputRef, $el: api.inputRef }) + function clear() { + api.change(null) + let options = dom(api.optionsRef) + if (options) { + options.scrollTop = 0 + } + api.goToOption(Focus.Nothing) + } + // When a `displayValue` prop is given, we should use it to transform the current selected // option(s) so that the format can be chosen by developers implementing this. This is useful if // your data is an object and you just want to pick a certain property or want to create a dynamic @@ -837,24 +846,6 @@ export let ComboboxInput = defineComponent({ switch (event.key) { // Ref: https://www.w3.org/WAI/ARIA/apg/patterns/menu/#keyboard-interaction-12 - case Keys.Backspace: - case Keys.Delete: - if (api.mode.value !== ValueMode.Single) return - if (!api.nullable.value) return - - let input = event.currentTarget as HTMLInputElement - requestAnimationFrame(() => { - if (input.value === '') { - api.change(null) - let options = dom(api.optionsRef) - if (options) { - options.scrollTop = 0 - } - api.goToOption(Focus.Nothing) - } - }) - break - case Keys.Enter: isTyping.value = false if (api.comboboxState.value !== ComboboxStates.Open) return @@ -942,6 +933,18 @@ export let ComboboxInput = defineComponent({ if (api.optionsRef.value && !api.optionsPropsRef.value.static) { event.stopPropagation() } + + if (api.nullable.value && api.mode.value === ValueMode.Single) { + // We want to clear the value when the user presses escape if and only if the current + // value is not set (aka, they didn't select anything yet, or they cleared the input which + // caused the value to be set to `null`). If the current value is set, then we want to + // fallback to that value when we press escape (this part is handled in the watcher that + // syncs the value with the input field again). + if (api.value.value === null) { + clear() + } + } + api.closeCombobox() break @@ -963,6 +966,17 @@ export let ComboboxInput = defineComponent({ // options while typing won't work at all because we are still in "composing" mode. emit('change', event) + // When the value becomes empty in a single value mode while being nullable then we want to clear + // the option entirely. + // + // This is can happen when you press backspace, but also when you select all the text and press + // ctrl/cmd+x. + if (api.nullable.value && api.mode.value === ValueMode.Single) { + if (event.target.value === '') { + clear() + } + } + // Open the combobox to show the results based on what the user has typed api.openCombobox() } diff --git a/packages/@headlessui-vue/src/components/transitions/utils/transition.ts b/packages/@headlessui-vue/src/components/transitions/utils/transition.ts index 008a01375..a269ba5cf 100644 --- a/packages/@headlessui-vue/src/components/transitions/utils/transition.ts +++ b/packages/@headlessui-vue/src/components/transitions/utils/transition.ts @@ -86,7 +86,7 @@ export function transition( // then we have some leftovers that should be cleaned. d.add(() => removeClasses(node, ...base, ...from, ...to, ...entered)) - // When we get disposed early, than we should also call the done method but switch the reason. + // When we get disposed early, then we should also call the done method but switch the reason. d.add(() => _done(Reason.Cancelled)) return d.dispose diff --git a/packages/@headlessui-vue/src/test-utils/interactions.ts b/packages/@headlessui-vue/src/test-utils/interactions.ts index 57c2ac7f3..612849023 100644 --- a/packages/@headlessui-vue/src/test-utils/interactions.ts +++ b/packages/@headlessui-vue/src/test-utils/interactions.ts @@ -155,7 +155,9 @@ let order: Record< value: element.value.slice(0, -1), }), }) - return fireEvent.keyDown(element, ev) + + fireEvent.keyDown(element, ev) + return fireEvent.input(element, ev) } return fireEvent.keyDown(element, event)