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 <malfait.robin@gmail.com>
2022-08-10 12:38:30 -04:00
parent 122eed7dbe
commit b28d177a95
9 changed files with 241 additions and 162 deletions
@@ -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))
@@ -427,38 +427,24 @@ 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>
            </>
          )
@@ -466,23 +452,27 @@ describe('Rendering', () => {

        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', () => {
@@ -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) {
@@ -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
      }
@@ -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))
@@ -389,7 +389,7 @@ describe('Rendering', () => {
    })
  })

-  describe('Combobox.Input', () => {
+  describe('ComboboxInput', () => {
    it(
      'selecting an option puts the value into Combobox.Input when displayValue is not provided',
      suppressConsoleLogs(async () => {
@@ -454,53 +454,51 @@ describe('Rendering', () => {
      })
    )

-    it(
-      'conditionally rendering the input should allow changing the display value',
-      suppressConsoleLogs(async () => {
-        let Example = defineComponent({
-          template: html`
-            <Combobox v-model="value" v-slot="{ open }" nullable>
-              <template v-if="!open">
-                <ComboboxInput :displayValue="(str) => (str?.toUpperCase() ?? '') + ' closed'" />
-                <ComboboxButton>Trigger</ComboboxButton>
-              </template>
-              <template v-else>
-                <ComboboxInput :displayValue="(str) => (str?.toUpperCase() ?? '') + ' open'" />
-                <ComboboxButton>Trigger</ComboboxButton>
-                <ComboboxOptions>
-                  <ComboboxOption value="a">Option A</ComboboxOption>
-                  <ComboboxOption value="b">Option B</ComboboxOption>
-                  <ComboboxOption value="c">Option C</ComboboxOption>
-                </ComboboxOptions>
-              </template>
-            </Combobox>
-          `,
-          setup: () => ({ value: ref(null) }),
-        })
-
-        renderTemplate(Example)
-
-        await nextFrame()
-
-        expect(getComboboxInput()).toHaveValue(' closed')
-
-        await click(getComboboxButton())
-
-        assertComboboxList({ state: ComboboxState.Visible })
-
-        expect(getComboboxInput()).toHaveValue(' open')
-
-        await click(getComboboxOptions()[1])
-
-        expect(getComboboxInput()).toHaveValue('B closed')
-
-        await click(getComboboxButton())
-
-        assertComboboxList({ state: ComboboxState.Visible })
-
-        expect(getComboboxInput()).toHaveValue('B open')
+    it('conditionally rendering the input should allow changing the display value', async () => {
+      let Example = defineComponent({
+        template: html`
+          <Combobox v-model="value" v-slot="{ open }" nullable>
+            <ComboboxInput
+              :displayValue="(str) => (str?.toUpperCase() ?? '') + (suffix ? ' with suffix' : ' no suffix')"
+            />
+            <ComboboxButton>Trigger</ComboboxButton>
+            <ComboboxOptions>
+              <ComboboxOption value="a">Option A</ComboboxOption>
+              <ComboboxOption value="b">Option B</ComboboxOption>
+              <ComboboxOption value="c">Option C</ComboboxOption>
+            </ComboboxOptions>
+            <button @click="suffix = !suffix">Toggle suffix</button>
+          </Combobox>
+        `,
+        setup: () => ({ value: ref(null), suffix: ref(false) }),
      })
-    )
+
+      renderTemplate(Example)
+
+      await nextFrame()
+
+      expect(getComboboxInput()).toHaveValue(' no suffix')
+
+      await click(getComboboxButton())
+
+      expect(getComboboxInput()).toHaveValue(' no suffix')
+
+      await click(getComboboxOptions()[1])
+
+      expect(getComboboxInput()).toHaveValue('B no suffix')
+
+      await click(getByText('Toggle suffix'))
+
+      expect(getComboboxInput()).toHaveValue('B no suffix') // No re-sync yet
+
+      await click(getComboboxButton())
+
+      expect(getComboboxInput()).toHaveValue('B no suffix') // No re-sync yet
+
+      await click(getComboboxOptions()[0])
+
+      expect(getComboboxInput()).toHaveValue('A with suffix')
+    })

    it(
      'should be possible to override the `type` on the input',
@@ -525,6 +523,54 @@ 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 () => {
+        renderTemplate({
+          template: html`
+            <Combobox v-model="value" v-slot="{ open }">
+              <ComboboxInput :displayValue="person => displayValue(person, open)" />
+
+              <ComboboxButton />
+
+              <ComboboxOptions>
+                <ComboboxOption value="alice">alice</ComboboxOption>
+                <ComboboxOption value="bob">bob</ComboboxOption>
+                <ComboboxOption value="charlie">charlie</ComboboxOption>
+              </ComboboxOptions>
+            </Combobox>
+          `,
+          setup: () => ({
+            value: ref('bob'),
+            displayValue(person: string, open: boolean) {
+              return `${person ?? ''} - ${open ? 'open' : 'closed'}`
+            },
+          }),
+        })
+
+        await nextFrame()
+
+        // 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('ComboboxLabel', () => {
@@ -70,7 +70,6 @@ type StateDefinition = {

  compare: (a: unknown, z: unknown) => boolean

-  inputPropsRef: Ref<{ displayValue?: (item: unknown) => string }>
  optionsPropsRef: Ref<{ static: boolean; hold: boolean }>

  labelRef: Ref<HTMLLabelElement | null>
@@ -217,7 +216,6 @@ export let Combobox = defineComponent({
        return activeOptionIndex.value
      }),
      activationTrigger,
-      inputPropsRef: ref<StateDefinition['inputPropsRef']['value']>({ displayValue: undefined }),
      optionsPropsRef,
      closeCombobox() {
        defaultToFirstOption.value = false
@@ -296,19 +294,6 @@ export let Combobox = defineComponent({
        activationTrigger.value = trigger ?? ActivationTrigger.Other
        options.value = adjustedState.options
      },
-      syncInputValue() {
-        let value = api.value.value
-        if (!dom(api.inputRef)) return
-        let displayValue = api.inputPropsRef.value.displayValue
-
-        if (typeof displayValue === 'function') {
-          api.inputRef!.value!.value = displayValue(value) ?? ''
-        } else if (typeof value === 'string') {
-          api.inputRef!.value!.value = value
-        } else {
-          api.inputRef!.value!.value = ''
-        }
-      },
      selectOption(id: string) {
        let option = options.value.find((item) => item.id === id)
        if (!option) return
@@ -332,7 +317,6 @@ export let Combobox = defineComponent({
            },
          })
        )
-        api.syncInputValue()
      },
      selectActiveOption() {
        if (api.activeOptionIndex.value === null) return
@@ -356,7 +340,6 @@ export let Combobox = defineComponent({
            },
          })
        )
-        api.syncInputValue()

        // It could happen that the `activeOptionIndex` stored in state is actually null,
        // but we are getting the fallback active option back instead.
@@ -406,26 +389,6 @@ export let Combobox = defineComponent({
      computed(() => comboboxState.value === ComboboxStates.Open)
    )

-    watch([api.value, api.inputRef, api.inputPropsRef], () => api.syncInputValue(), {
-      immediate: true,
-      deep: true,
-    })
-
-    // Only sync the input value on close as typing into the input will trigger it to open
-    // causing a resync of the input value with the currently stored, stale value that is
-    // one character behind since the input's value has just been updated by the browser
-    watch(
-      api.comboboxState,
-      (state) => {
-        if (state === ComboboxStates.Closed) {
-          api.syncInputValue()
-        }
-      },
-      {
-        immediate: true,
-      }
-    )
-
    // @ts-expect-error Types of property 'dataRef' are incompatible.
    provide(ComboboxContext, api)
    useOpenClosedProvider(
@@ -647,12 +610,44 @@ export let ComboboxInput = defineComponent({
    let api = useComboboxContext('ComboboxInput')
    let id = `headlessui-combobox-input-${useId()}`

-    watchEffect(() => {
-      api.inputPropsRef.value = props
-    })
-
    expose({ el: api.inputRef, $el: api.inputRef })

+    let currentValue = ref(api.value.value as unknown as string)
+
+    let getCurrentValue = () => {
+      let value = api.value.value
+      if (!dom(api.inputRef)) return ''
+
+      if (typeof props.displayValue !== 'undefined') {
+        return props.displayValue(value as unknown) ?? ''
+      } else if (typeof value === 'string') {
+        return value
+      } else {
+        return ''
+      }
+    }
+
+    onMounted(() => {
+      watch([api.value], () => (currentValue.value = getCurrentValue()), {
+        flush: 'sync',
+        immediate: true,
+      })
+
+      watch(
+        [currentValue, api.comboboxState],
+        ([currentValue, state], [oldCurrentValue, oldState]) => {
+          let input = dom(api.inputRef)
+          if (!input) return
+          if (oldState === ComboboxStates.Open && state === ComboboxStates.Closed) {
+            input.value = currentValue
+          } else if (currentValue !== oldCurrentValue) {
+            input.value = currentValue
+          }
+        },
+        { immediate: true }
+      )
+    })
+
    function handleKeyDown(event: KeyboardEvent) {
      switch (event.key) {
        // Ref: https://www.w3.org/TR/wai-aria-practices-1.2/#keyboard-interaction-12
@@ -1,4 +1,4 @@
-import { computed, ComputedRef, ref } from 'vue'
+import { computed, ComputedRef, UnwrapRef, ref } from 'vue'

 export function useControllable<T>(
  controlledValue: ComputedRef<T | undefined>,
@@ -14,7 +14,7 @@ export function useControllable<T>(
      if (isControlled.value) {
        return onChange?.(value as T)
      } else {
-        internalValue.value = value as T
+        internalValue.value = value as UnwrapRef<T>
        return onChange?.(value as T)
      }
    },
@@ -8,8 +8,6 @@ import {
  InjectionKey,
  Ref,
  watch,
-  ref,
-  onBeforeUnmount,
 } from 'vue'

 type OnUpdate = (message: StackMessage, type: string, element: Ref<HTMLElement | null>) => void