Fix displayValue syncing problem (#1755)

* ensure `syncInputValue` is updated correctly * WIP * WIP * Don’t resync on open * Fix react value syncing update * Add comment * Port new setup over to Vue * Remove `inputPropsRef` We hardly knew ye * Remove repro * Cleanup * Update changelog Co-authored-by: Robin Malfait <[email protected]>
tailwindlabs · Aug 10, 2022 · b28d177 · b28d177
1 parent 122eed7
commit b28d177
Show file tree

Hide file tree

Showing 9 changed files with 231 additions and 152 deletions.
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
@@ -19,7 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
-- Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679))
+- Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679), [#1755](https://github.com/tailwindlabs/headlessui/pull/1755))
 - Ensure controlled `Tabs` don't change automagically ([#1680](https://github.com/tailwindlabs/headlessui/pull/1680))
 - Don't scroll lock when a Transition + Dialog is mounted but hidden ([#1681](https://github.com/tailwindlabs/headlessui/pull/1681))
 - Improve outside click on Safari iOS ([#1712](https://github.com/tailwindlabs/headlessui/pull/1712))

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -427,62 +427,52 @@ describe('Rendering', () => {
       suppressConsoleLogs(async () => {
         function Example() {
           let [value, setValue] = useState(null)
+          let [suffix, setSuffix] = useState(false)
 
           return (
             <>
               <Combobox value={value} onChange={setValue} nullable>
-                {({ open }) => {
-                  if (!open) {
-                    return (
-                      <>
-                        <Combobox.Input
-                          onChange={NOOP}
-                          displayValue={(str?: string) => `${str?.toUpperCase() ?? ''} closed`}
-                        />
-                        <Combobox.Button>Trigger</Combobox.Button>
-                      </>
-                    )
+                <Combobox.Input
+                  onChange={NOOP}
+                  displayValue={(str?: string) =>
+                    `${str?.toUpperCase() ?? ''} ${suffix ? 'with suffix' : 'no suffix'}`
                   }
-
-                  return (
-                    <>
-                      <Combobox.Input
-                        onChange={NOOP}
-                        displayValue={(str?: string) => `${str?.toUpperCase() ?? ''} open`}
-                      />
-                      <Combobox.Button>Trigger</Combobox.Button>
-                      <Combobox.Options>
-                        <Combobox.Option value="a">Option A</Combobox.Option>
-                        <Combobox.Option value="b">Option B</Combobox.Option>
-                        <Combobox.Option value="c">Option C</Combobox.Option>
-                      </Combobox.Options>
-                    </>
-                  )
-                }}
+                />
+                <Combobox.Button>Trigger</Combobox.Button>
+                <Combobox.Options>
+                  <Combobox.Option value="a">Option A</Combobox.Option>
+                  <Combobox.Option value="b">Option B</Combobox.Option>
+                  <Combobox.Option value="c">Option C</Combobox.Option>
+                </Combobox.Options>
+                <button onClick={() => setSuffix((v) => !v)}>Toggle suffix</button>
               </Combobox>
             </>
           )
         }
 
         render(<Example />)
 
-        expect(getComboboxInput()).toHaveValue(' closed')
+        expect(getComboboxInput()).toHaveValue(' no suffix')
 
         await click(getComboboxButton())
 
-        assertComboboxList({ state: ComboboxState.Visible })
-
-        expect(getComboboxInput()).toHaveValue(' open')
+        expect(getComboboxInput()).toHaveValue(' no suffix')
 
         await click(getComboboxOptions()[1])
 
-        expect(getComboboxInput()).toHaveValue('B closed')
+        expect(getComboboxInput()).toHaveValue('B no suffix')
+
+        await click(getByText('Toggle suffix'))
+
+        expect(getComboboxInput()).toHaveValue('B no suffix') // No re-sync yet
 
         await click(getComboboxButton())
 
-        assertComboboxList({ state: ComboboxState.Visible })
+        expect(getComboboxInput()).toHaveValue('B no suffix') // No re-sync yet
 
-        expect(getComboboxInput()).toHaveValue('B open')
+        await click(getComboboxOptions()[0])
+
+        expect(getComboboxInput()).toHaveValue('A with suffix')
       })
     )
 
@@ -510,6 +500,58 @@ describe('Rendering', () => {
         expect(getComboboxInput()).toHaveAttribute('type', 'search')
       })
     )
+
+    xit(
+      'should reflect the value in the input when the value changes and when you are typing',
+      suppressConsoleLogs(async () => {
+        function Example() {
+          let [value, setValue] = useState('bob')
+          let [_query, setQuery] = useState('')
+
+          return (
+            <Combobox value={value} onChange={setValue}>
+              {({ open }) => (
+                <>
+                  <Combobox.Input
+                    onChange={(event) => setQuery(event.target.value)}
+                    displayValue={(person) => `${person ?? ''} - ${open ? 'open' : 'closed'}`}
+                  />
+
+                  <Combobox.Button />
+
+                  <Combobox.Options>
+                    <Combobox.Option value="alice">alice</Combobox.Option>
+                    <Combobox.Option value="bob">bob</Combobox.Option>
+                    <Combobox.Option value="charlie">charlie</Combobox.Option>
+                  </Combobox.Options>
+                </>
+              )}
+            </Combobox>
+          )
+        }
+
+        render(<Example />)
+
+        // Check for proper state sync
+        expect(getComboboxInput()).toHaveValue('bob - closed')
+        await click(getComboboxButton())
+        expect(getComboboxInput()).toHaveValue('bob - open')
+        await click(getComboboxButton())
+        expect(getComboboxInput()).toHaveValue('bob - closed')
+
+        // Check if we can still edit the input
+        for (let _ of Array(' - closed'.length)) {
+          await press(Keys.Backspace, getComboboxInput())
+        }
+        getComboboxInput()?.select()
+        await type(word('alice'), getComboboxInput())
+        expect(getComboboxInput()).toHaveValue('alice')
+
+        // Open the combobox and choose an option
+        await click(getComboboxOptions()[2])
+        expect(getComboboxInput()).toHaveValue('charlie - closed')
+      })
+    )
   })
 
   describe('Combobox.Label', () => {

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -41,6 +41,7 @@ import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-cl
 
 import { Keys } from '../keyboard'
 import { useControllable } from '../../hooks/use-controllable'
+import { useWatch } from '../../hooks/use-watch'
 
 enum ComboboxState {
   Open,
@@ -262,9 +263,6 @@ let ComboboxDataContext = createContext<
       isSelected(value: unknown): boolean
       __demoMode: boolean
 
-      inputPropsRef: MutableRefObject<{
-        displayValue?(item: unknown): string
-      }>
       optionsPropsRef: MutableRefObject<{
         static: boolean
         hold: boolean
@@ -352,7 +350,6 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
   let defaultToFirstOption = useRef(false)
 
   let optionsPropsRef = useRef<_Data['optionsPropsRef']['current']>({ static: false, hold: false })
-  let inputPropsRef = useRef<_Data['inputPropsRef']['current']>({ displayValue: undefined })
 
   let labelRef = useRef<_Data['labelRef']['current']>(null)
   let inputRef = useRef<_Data['inputRef']['current']>(null)
@@ -382,7 +379,6 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
     () => ({
       ...state,
       optionsPropsRef,
-      inputPropsRef,
       labelRef,
       inputRef,
       buttonRef,
@@ -440,32 +436,17 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
     [data, disabled, value]
   )
 
-  let syncInputValue = useCallback(() => {
-    if (!data.inputRef.current) return
-    let displayValue = inputPropsRef.current.displayValue
-
-    if (typeof displayValue === 'function') {
-      data.inputRef.current.value = displayValue(value) ?? ''
-    } else if (typeof value === 'string') {
-      data.inputRef.current.value = value
-    } else {
-      data.inputRef.current.value = ''
-    }
-  }, [value, data.inputRef, inputPropsRef.current?.displayValue])
-
   let selectOption = useEvent((id: string) => {
     let option = data.options.find((item) => item.id === id)
     if (!option) return
 
     onChange(option.dataRef.current.value)
-    syncInputValue()
   })
 
   let selectActiveOption = useEvent(() => {
     if (data.activeOptionIndex !== null) {
       let { dataRef, id } = data.options[data.activeOptionIndex]
       onChange(dataRef.current.value)
-      syncInputValue()
 
       // It could happen that the `activeOptionIndex` stored in state is actually null,
       // but we are getting the fallback active option back instead.
@@ -531,13 +512,6 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
     []
   )
 
-  useIsoMorphicEffect(() => {
-    if (data.comboboxState !== ComboboxState.Closed) return
-    syncInputValue()
-  }, [syncInputValue, data.comboboxState])
-
-  // Ensure that we update the inputRef if the value changes
-  useIsoMorphicEffect(syncInputValue, [syncInputValue])
   let ourProps = ref === null ? {} : { ref }
 
   return (
@@ -612,14 +586,33 @@ let Input = forwardRefWithAs(function Input<
   let actions = useActions('Combobox.Input')
 
   let inputRef = useSyncRefs(data.inputRef, ref)
-  let inputPropsRef = data.inputPropsRef
 
   let id = `headlessui-combobox-input-${useId()}`
   let d = useDisposables()
 
-  useIsoMorphicEffect(() => {
-    inputPropsRef.current.displayValue = displayValue
-  }, [displayValue, inputPropsRef])
+  let currentValue = useMemo(() => {
+    if (typeof displayValue === 'function') {
+      return displayValue(data.value as unknown as TType) ?? ''
+    } else if (typeof data.value === 'string') {
+      return data.value
+    } else {
+      return ''
+    }
+
+    // displayValue is intentionally left out
+  }, [data.value])
+
+  useWatch(
+    ([currentValue, state], [oldCurrentValue, oldState]) => {
+      if (!data.inputRef.current) return
+      if (oldState === ComboboxState.Open && state === ComboboxState.Closed) {
+        data.inputRef.current.value = currentValue
+      } else if (currentValue !== oldCurrentValue) {
+        data.inputRef.current.value = currentValue
+      }
+    },
+    [currentValue, data.comboboxState]
+  )
 
   let handleKeyDown = useEvent((event: ReactKeyboardEvent<HTMLInputElement>) => {
     switch (event.key) {

diff --git a/packages/@headlessui-react/src/hooks/use-watch.ts b/packages/@headlessui-react/src/hooks/use-watch.ts
@@ -1,15 +1,20 @@
 import { useEffect, useRef } from 'react'
 import { useEvent } from './use-event'
 
-export function useWatch<T>(cb: (values: T[]) => void | (() => void), dependencies: T[]) {
-  let track = useRef<typeof dependencies>([])
+export function useWatch<T extends any[]>(
+  cb: (newValues: [...T], oldValues: [...T]) => void | (() => void),
+  dependencies: [...T]
+) {
+  let track = useRef<typeof dependencies>([] as unknown as [...T])
   let action = useEvent(cb)
 
   useEffect(() => {
+    let oldValues = [...track.current] as unknown as [...T]
+
     for (let [idx, value] of dependencies.entries()) {
       if (track.current[idx] !== value) {
         // At least 1 item changed
-        let returnValue = action(dependencies)
+        let returnValue = action(dependencies, oldValues)
         track.current = dependencies
         return returnValue
       }

diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md
@@ -19,7 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667))
 - Close `Menu` component when using `tab` key ([#1673](https://github.com/tailwindlabs/headlessui/pull/1673))
-- Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679))
+- Resync input when display value changes ([#1679](https://github.com/tailwindlabs/headlessui/pull/1679), [#1755](https://github.com/tailwindlabs/headlessui/pull/1755))
 - Ensure controlled `Tabs` don't change automagically ([#1680](https://github.com/tailwindlabs/headlessui/pull/1680))
 - Improve outside click on Safari iOS ([#1712](https://github.com/tailwindlabs/headlessui/pull/1712))
 - Improve event handler merging ([#1715](https://github.com/tailwindlabs/headlessui/pull/1715))