From a09dd84a9e35110c6096eccc0232713297260026 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 23 Nov 2022 15:13:37 +0100 Subject: [PATCH] syncing of the input should happen when the value changes internally or externally I also got rid of the manually dispatching of the change event if the value changes from internally. I think the correct mental model is: - That the `Combobox.Input` value should change if the selected value changes from the outside or from the inside. - Note: It should _not_ do that if you are currently typing (once you choose a new value it will re-sync, once you reset (escape / outside click) it will also sync again). - The `onChange`/`onInput` of the `Combobox.Input` itself should only be called if you as the user type something. Not when the value is "synced" based on the selected value. We were currently manually dispatching events which works (to a certain extend) but smelled a bit fishy to me. The manual dispatching of events tried to solve an issue (https://github.com/tailwindlabs/headlessui/issues/1875), but I think this can be solved in 2 other ways that make a bit more sense: 1. (Today) Use the `onBlur` on the input to reset the `query` value to filter options. 2. (In the future) Use an exposed `onClose` (or similar) event to reset your `query` value. --- .../src/components/combobox/combobox.test.tsx | 50 ------------ .../src/components/combobox/combobox.tsx | 78 +++++++++++-------- .../src/components/combobox/combobox.test.ts | 54 ------------- .../src/components/combobox/combobox.ts | 64 ++++++++++----- 4 files changed, 88 insertions(+), 158 deletions(-) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index d7d5a559b..d4e65258e 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -2965,56 +2965,6 @@ describe('Keyboard interactions', () => { expect(getComboboxInput()?.value).toBe('option-b') }) ) - - it( - 'The onChange handler is fired when the input value is changed internally', - suppressConsoleLogs(async () => { - let currentSearchQuery: string = '' - - render( - - { - currentSearchQuery = event.target.value - }} - /> - Trigger - - Option A - Option B - Option C - - - ) - - // Open combobox - await click(getComboboxButton()) - - // Verify that the current search query is empty - expect(currentSearchQuery).toBe('') - - // Look for "Option C" - await type(word('Option C'), getComboboxInput()) - - // The input should be updated - expect(getComboboxInput()?.value).toBe('Option C') - - // The current search query should reflect the input value - expect(currentSearchQuery).toBe('Option C') - - // Close combobox - await press(Keys.Escape) - - // The input should be empty - expect(getComboboxInput()?.value).toBe('') - - // The current search query should be empty like the input - expect(currentSearchQuery).toBe('') - - // The combobox should be closed - assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) - }) - ) }) describe('`ArrowDown` key', () => { diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 005bd974b..31776faa2 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -490,7 +490,7 @@ function ComboboxFn dispatch({ type: ActionTypes.CloseCombobox }), + () => actions.closeCombobox(), data.comboboxState === ComboboxState.Open ) @@ -522,7 +522,7 @@ function ComboboxFn { if (typeof displayValue === 'function') { return displayValue(data.value as unknown as TType) ?? '' @@ -726,11 +710,26 @@ let Input = forwardRefWithAs(function Input< // displayValue is intentionally left out }, [data.value]) + // Syncing the input value has some rules attached to it to guarantee a smooth and expected user + // experience: + // + // - When a user is not typing in the input field, it is safe to update the input value based on + // the selected option(s). See `currentValue` computation from above. + // - The value can be updated when: + // - The `value` is set from outside of the component + // - The `value` is set when the user uses their keyboard (confirm via enter or space) + // - The `value` is set when the user clicks on a value to select it + // - The value will be reset to the current selected option(s), when: + // - The user is _not_ typing (otherwise you will loose your current state / query) + // - The user cancels the current changes: + // - By pressing `escape` + // - By clicking `outside` of the Combobox useWatch( ([currentValue, state], [oldCurrentValue, oldState]) => { + if (isTyping.current) return if (!data.inputRef.current) return if (oldState === ComboboxState.Open && state === ComboboxState.Closed) { - updateInputAndNotify(currentValue) + data.inputRef.current.value = currentValue } else if (currentValue !== oldCurrentValue) { data.inputRef.current.value = currentValue } @@ -749,6 +748,7 @@ let Input = forwardRefWithAs(function Input< }) let handleKeyDown = useEvent((event: ReactKeyboardEvent) => { + isTyping.current = true switch (event.key) { // Ref: https://www.w3.org/TR/wai-aria-practices-1.2/#keyboard-interaction-12 @@ -770,6 +770,7 @@ let Input = forwardRefWithAs(function Input< break case Keys.Enter: + isTyping.current = false if (data.comboboxState !== ComboboxState.Open) return if (isComposing.current) return @@ -788,6 +789,7 @@ let Input = forwardRefWithAs(function Input< break case Keys.ArrowDown: + isTyping.current = false event.preventDefault() event.stopPropagation() return match(data.comboboxState, { @@ -800,6 +802,7 @@ let Input = forwardRefWithAs(function Input< }) case Keys.ArrowUp: + isTyping.current = false event.preventDefault() event.stopPropagation() return match(data.comboboxState, { @@ -821,11 +824,13 @@ let Input = forwardRefWithAs(function Input< break } + isTyping.current = false event.preventDefault() event.stopPropagation() return actions.goToOption(Focus.First) case Keys.PageUp: + isTyping.current = false event.preventDefault() event.stopPropagation() return actions.goToOption(Focus.First) @@ -835,16 +840,19 @@ let Input = forwardRefWithAs(function Input< break } + isTyping.current = false event.preventDefault() event.stopPropagation() return actions.goToOption(Focus.Last) case Keys.PageDown: + isTyping.current = false event.preventDefault() event.stopPropagation() return actions.goToOption(Focus.Last) case Keys.Escape: + isTyping.current = false if (data.comboboxState !== ComboboxState.Open) return event.preventDefault() if (data.optionsRef.current && !data.optionsPropsRef.current.static) { @@ -853,6 +861,7 @@ let Input = forwardRefWithAs(function Input< return actions.closeCombobox() case Keys.Tab: + isTyping.current = false if (data.comboboxState !== ComboboxState.Open) return if (data.mode === ValueMode.Single) actions.selectActiveOption() actions.closeCombobox() @@ -861,12 +870,14 @@ let Input = forwardRefWithAs(function Input< }) let handleChange = useEvent((event: React.ChangeEvent) => { - if (!shouldIgnoreOpenOnChange) { - actions.openCombobox() - } + actions.openCombobox() onChange?.(event) }) + let handleBlur = useEvent(() => { + isTyping.current = false + }) + // TODO: Verify this. The spec says that, for the input/combobox, the label is the labelling element when present // Otherwise it's the ID of the non-label element let labelledby = useComputed(() => { @@ -899,6 +910,7 @@ let Input = forwardRefWithAs(function Input< onCompositionEnd: handleCompositionEnd, onKeyDown: handleKeyDown, onChange: handleChange, + onBlur: handleBlur, } return render({ diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index 3e8000c3e..8b09e5546 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -3052,60 +3052,6 @@ describe('Keyboard interactions', () => { expect(getComboboxInput()?.value).toBe('option-b') }) ) - - it( - 'The onChange handler is fired when the input value is changed internally', - suppressConsoleLogs(async () => { - let currentSearchQuery: string = '' - - renderTemplate({ - template: html` - - - Trigger - - Option A - Option B - Option C - - - `, - setup: () => ({ - value: ref(null), - onChange: (evt: InputEvent & { target: HTMLInputElement }) => { - currentSearchQuery = evt.target.value - }, - }), - }) - - // Open combobox - await click(getComboboxButton()) - - // Verify that the current search query is empty - expect(currentSearchQuery).toBe('') - - // Look for "Option C" - await type(word('Option C'), getComboboxInput()) - - // The input should be updated - expect(getComboboxInput()?.value).toBe('Option C') - - // The current search query should reflect the input value - expect(currentSearchQuery).toBe('Option C') - - // Close combobox - await press(Keys.Escape) - - // The input should be empty - expect(getComboboxInput()?.value).toBe('') - - // The current search query should be empty like the input - expect(currentSearchQuery).toBe('') - - // The combobox should be closed - assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) - }) - ) }) describe('`ArrowDown` key', () => { diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index ab8437db7..47dda4b54 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -637,10 +637,21 @@ export let ComboboxInput = defineComponent({ let api = useComboboxContext('ComboboxInput') let id = `headlessui-combobox-input-${useId()}` + let isTyping = { value: false } + expose({ el: api.inputRef, $el: api.inputRef }) let currentValue = ref(api.value.value as unknown as string) + // 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 value like `firstName + ' ' + lastName`. + // + // Note: This can also be used with multiple selected options, but this is a very simple transform + // which should always result in a string (since we are filling in the value of the the input), + // you don't have to use this at all, a more common UI is a "tag" based UI, which you can render + // yourself using the selected option(s). let getCurrentValue = () => { let value = api.value.value if (!dom(api.inputRef)) return '' @@ -657,23 +668,6 @@ export let ComboboxInput = defineComponent({ // Workaround Vue bug where watching [ref(undefined)] is not fired immediately even when value is true let __fixVueImmediateWatchBug__ = ref('') - let shouldIgnoreOpenOnChange = false - function updateInputAndNotify(currentValue: string) { - let input = dom(api.inputRef) - if (!input) { - return - } - - input.value = currentValue - - // Fire an input event which causes the browser to trigger the user's `onChange` handler. - // We have to prevent the combobox from opening when this happens. Since these events - // fire synchronously `shouldIgnoreOpenOnChange` will be correct during `handleChange` - shouldIgnoreOpenOnChange = true - input.dispatchEvent(new Event('input', { bubbles: true })) - shouldIgnoreOpenOnChange = false - } - onMounted(() => { watch( [api.value, __fixVueImmediateWatchBug__], @@ -686,13 +680,28 @@ export let ComboboxInput = defineComponent({ } ) + // Syncing the input value has some rules attached to it to guarantee a smooth and expected user + // experience: + // + // - When a user is not typing in the input field, it is safe to update the input value based on + // the selected option(s). See `currentValue` computation from above. + // - The value can be updated when: + // - The `value` is set from outside of the component + // - The `value` is set when the user uses their keyboard (confirm via enter or space) + // - The `value` is set when the user clicks on a value to select it + // - The value will be reset to the current selected option(s), when: + // - The user is _not_ typing (otherwise you will loose your current state / query) + // - The user cancels the current changes: + // - By pressing `escape` + // - By clicking `outside` of the Combobox watch( [currentValue, api.comboboxState], ([currentValue, state], [oldCurrentValue, oldState]) => { + if (isTyping.value) return let input = dom(api.inputRef) if (!input) return if (oldState === ComboboxStates.Open && state === ComboboxStates.Closed) { - updateInputAndNotify(currentValue) + input.value = currentValue } else if (currentValue !== oldCurrentValue) { input.value = currentValue } @@ -712,6 +721,7 @@ export let ComboboxInput = defineComponent({ } function handleKeyDown(event: KeyboardEvent) { + isTyping.value = true switch (event.key) { // Ref: https://www.w3.org/TR/wai-aria-practices-1.2/#keyboard-interaction-12 @@ -734,6 +744,7 @@ export let ComboboxInput = defineComponent({ break case Keys.Enter: + isTyping.value = false if (api.comboboxState.value !== ComboboxStates.Open) return if (isComposing.value) return @@ -752,6 +763,7 @@ export let ComboboxInput = defineComponent({ break case Keys.ArrowDown: + isTyping.value = false event.preventDefault() event.stopPropagation() return match(api.comboboxState.value, { @@ -760,6 +772,7 @@ export let ComboboxInput = defineComponent({ }) case Keys.ArrowUp: + isTyping.value = false event.preventDefault() event.stopPropagation() return match(api.comboboxState.value, { @@ -779,11 +792,13 @@ export let ComboboxInput = defineComponent({ break } + isTyping.value = false event.preventDefault() event.stopPropagation() return api.goToOption(Focus.First) case Keys.PageUp: + isTyping.value = false event.preventDefault() event.stopPropagation() return api.goToOption(Focus.First) @@ -793,16 +808,19 @@ export let ComboboxInput = defineComponent({ break } + isTyping.value = false event.preventDefault() event.stopPropagation() return api.goToOption(Focus.Last) case Keys.PageDown: + isTyping.value = false event.preventDefault() event.stopPropagation() return api.goToOption(Focus.Last) case Keys.Escape: + isTyping.value = false if (api.comboboxState.value !== ComboboxStates.Open) return event.preventDefault() if (api.optionsRef.value && !api.optionsPropsRef.value.static) { @@ -812,6 +830,7 @@ export let ComboboxInput = defineComponent({ break case Keys.Tab: + isTyping.value = false if (api.comboboxState.value !== ComboboxStates.Open) return if (api.mode.value === ValueMode.Single) api.selectActiveOption() api.closeCombobox() @@ -824,12 +843,14 @@ export let ComboboxInput = defineComponent({ } function handleInput(event: Event & { target: HTMLInputElement }) { - if (!shouldIgnoreOpenOnChange) { - api.openCombobox() - } + api.openCombobox() emit('change', event) } + function handleBlur() { + isTyping.value = false + } + let defaultValue = computed(() => { return ( props.defaultValue ?? @@ -858,6 +879,7 @@ export let ComboboxInput = defineComponent({ onKeydown: handleKeyDown, onChange: handleChange, onInput: handleInput, + onBlur: handleBlur, role: 'combobox', type: attrs.type ?? 'text', tabIndex: 0,