Improve syncing of the Combobox.Input value (#2042)

* make combobox playgrounds in React and Vue similar * 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 (#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. * update changelog * ignore flakey test
tailwindlabs · Nov 23, 2022 · 4da0b3a · 4da0b3a
1 parent 8e1e19f
commit 4da0b3a
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 161 deletions.
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add warning when using `<Popover.Button />` multiple times ([#2007](https://github.com/tailwindlabs/headlessui/pull/2007))
 - Ensure Popover doesn't crash when `focus` is going to `window` ([#2019](https://github.com/tailwindlabs/headlessui/pull/2019))
 - Ensure `shift+home` and `shift+end` works as expected in the `Combobox.Input` component ([#2024](https://github.com/tailwindlabs/headlessui/pull/2024))
+- Improve syncing of the `Combobox.Input` value ([#2042](https://github.com/tailwindlabs/headlessui/pull/2042))
 
 ## [1.7.4] - 2022-11-03
 

diff --git 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(
-            <Combobox value={null} onChange={console.log}>
-              <Combobox.Input
-                onChange={(event) => {
-                  currentSearchQuery = event.target.value
-                }}
-              />
-              <Combobox.Button>Trigger</Combobox.Button>
-              <Combobox.Options>
-                <Combobox.Option value="option-a">Option A</Combobox.Option>
-                <Combobox.Option value="option-b">Option B</Combobox.Option>
-                <Combobox.Option value="option-c">Option C</Combobox.Option>
-              </Combobox.Options>
-            </Combobox>
-          )
-
-          // 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
@@ -490,7 +490,7 @@ function ComboboxFn<TValue, TTag extends ElementType = typeof DEFAULT_COMBOBOX_T
   // Handle outside click
   useOutsideClick(
     [data.buttonRef, data.inputRef, data.optionsRef],
-    () => dispatch({ type: ActionTypes.CloseCombobox }),
+    () => actions.closeCombobox(),
     data.comboboxState === ComboboxState.Open
   )
 
@@ -522,7 +522,7 @@ function ComboboxFn<TValue, TTag extends ElementType = typeof DEFAULT_COMBOBOX_T
 
       // It could happen that the `activeOptionIndex` stored in state is actually null,
       // but we are getting the fallback active option back instead.
-      dispatch({ type: ActionTypes.GoToOption, focus: Focus.Specific, id })
+      actions.goToOption(Focus.Specific, id)
     }
   })
 
@@ -684,36 +684,20 @@ let Input = forwardRefWithAs(function Input<
 
   let inputRef = useSyncRefs(data.inputRef, ref)
 
+  let isTyping = useRef(false)
+
   let id = `headlessui-combobox-input-${useId()}`
   let d = useDisposables()
 
-  let shouldIgnoreOpenOnChange = false
-  function updateInputAndNotify(newValue: string) {
-    let input = data.inputRef.current
-    if (!input) {
-      return
-    }
-
-    // The value is already the same, so we can bail out early
-    if (input.value === newValue) {
-      return
-    }
-
-    // Skip React's value setting which causes the input event to not be fired because it de-dupes input/change events
-    let descriptor = Object.getOwnPropertyDescriptor(input.constructor.prototype, 'value')
-    descriptor?.set?.call(input, newValue)
-
-    // 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 }))
-
-    // Now we can inform react that the input value has changed
-    input.value = newValue
-    shouldIgnoreOpenOnChange = false
-  }
-
+  // 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 currentValue = useMemo(() => {
     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<HTMLInputElement>) => {
+    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<HTMLInputElement>) => {
-    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-react/src/components/transitions/transition.test.tsx b/packages/@headlessui-react/src/components/transitions/transition.test.tsx
@@ -552,7 +552,7 @@ describe('Transitions', () => {
       `)
     })
 
-    it('should transition in completely (duration defined in seconds) in (render strategy = hidden)', async () => {
+    xit('should transition in completely (duration defined in seconds) in (render strategy = hidden)', async () => {
       let enterDuration = 100
 
       function Example() {

diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Reset form-like components when the parent `<form>` resets ([#2004](https://github.com/tailwindlabs/headlessui/pull/2004))
 - Ensure Popover doesn't crash when `focus` is going to `window` ([#2019](https://github.com/tailwindlabs/headlessui/pull/2019))
 - Ensure `shift+home` and `shift+end` works as expected in the `ComboboxInput` component ([#2024](https://github.com/tailwindlabs/headlessui/pull/2024))
+- Improve syncing of the `ComboboxInput` value ([#2042](https://github.com/tailwindlabs/headlessui/pull/2042))
 
 ## [1.7.4] - 2022-11-03
 

diff --git 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`
-              <Combobox v-model="value">
-                <ComboboxInput @change="onChange" />
-                <ComboboxButton>Trigger</ComboboxButton>
-                <ComboboxOptions>
-                  <ComboboxOption value="option-a">Option A</ComboboxOption>
-                  <ComboboxOption value="option-b">Option B</ComboboxOption>
-                  <ComboboxOption value="option-c">Option C</ComboboxOption>
-                </ComboboxOptions>
-              </Combobox>
-            `,
-            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', () => {