Fix Combobox issues (#1099)

* Add combobox to Vue playground * Update input props * Wire up input event for changes This fires changes whenever you type, not just on blur * Fix playground * Don't fire input event when pressing escape The input event is only supposed to fire when the .value of the input changes. Pressing escape doesn't change the value of the input directly so it shouldn't fire. * Add latest active option render prop * Add missing active option props to Vue version * cleanup * Move test * Fix error * Add latest active option to Vue version * Tweak active option to not re-render * Remove refocusing on outside mousedown * Update tests * Forward refs on combobox to children * Cleanup code a bit * Fix lint problems on commit * Fix typescript issues * Update changelog
tailwindlabs · Feb 8, 2022 · 554d04b · 554d04b
1 parent 6fc28c6
commit 554d04b
Show file tree

Hide file tree

Showing 14 changed files with 749 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047))
+- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
 
 ## [Unreleased - @headlessui/vue]
 
@@ -30,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047))
+- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
 
 ## [@headlessui/react@v1.4.3] - 2022-01-14
 

diff --git a/package.json b/package.json
@@ -28,7 +28,7 @@
     }
   },
   "lint-staged": {
-    "*": "yarn lint-check"
+    "*": "yarn lint"
   },
   "prettier": {
     "printWidth": 100,

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -3588,19 +3588,26 @@ describe('Mouse interactions', () => {
     })
   )
 
-  it(
+  // TODO: JSDOM doesn't quite work here
+  // Clicking outside on the body should fire a mousedown (which it does) and then change the active element (which it doesn't)
+  xit(
     'should be possible to click outside of the combobox which should close the combobox',
     suppressConsoleLogs(async () => {
       render(
-        <Combobox value="test" onChange={console.log}>
-          <Combobox.Input onChange={NOOP} />
-          <Combobox.Button>Trigger</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>
+        <>
+          <Combobox value="test" onChange={console.log}>
+            <Combobox.Input onChange={NOOP} />
+            <Combobox.Button>Trigger</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>
+          <div tabIndex={-1} data-test-focusable>
+            after
+          </div>
+        </>
       )
 
       // Open combobox
@@ -3609,13 +3616,13 @@ describe('Mouse interactions', () => {
       assertActiveElement(getComboboxInput())
 
       // Click something that is not related to the combobox
-      await click(document.body)
+      await click(getByText('after'))
 
       // Should be closed now
       assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
 
-      // Verify the input is focused again
-      assertActiveElement(getComboboxInput())
+      // Verify the button is focused
+      assertActiveElement(getByText('after'))
     })
   )
 
@@ -4130,4 +4137,68 @@ describe('Mouse interactions', () => {
       assertNoActiveComboboxOption()
     })
   )
+
+  it(
+    'Combobox preserves the latest known active option after an option becomes inactive',
+    suppressConsoleLogs(async () => {
+      render(
+        <Combobox value="test" onChange={console.log}>
+          {({ open, latestActiveOption }) => (
+            <>
+              <Combobox.Input onChange={NOOP} />
+              <Combobox.Button>Trigger</Combobox.Button>
+              <div id="latestActiveOption">{latestActiveOption}</div>
+              {open && (
+                <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>
+      )
+
+      assertComboboxButton({
+        state: ComboboxState.InvisibleUnmounted,
+        attributes: { id: 'headlessui-combobox-button-2' },
+      })
+      assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
+
+      await click(getComboboxButton())
+
+      assertComboboxButton({
+        state: ComboboxState.Visible,
+        attributes: { id: 'headlessui-combobox-button-2' },
+      })
+      assertComboboxList({ state: ComboboxState.Visible })
+
+      let options = getComboboxOptions()
+
+      // Hover the first item
+      await mouseMove(options[0])
+
+      // Verify that the first combobox option is active
+      assertActiveComboboxOption(options[0])
+      expect(document.getElementById('latestActiveOption')!.textContent).toBe('a')
+
+      // Focus the second item
+      await mouseMove(options[1])
+
+      // Verify that the second combobox option is active
+      assertActiveComboboxOption(options[1])
+      expect(document.getElementById('latestActiveOption')!.textContent).toBe('b')
+
+      // Move the mouse off of the second combobox option
+      await mouseLeave(options[1])
+      await mouseMove(document.body)
+
+      // Verify that the second combobox option is NOT active
+      assertNoActiveComboboxOption()
+
+      // But the last known active option is still recorded
+      expect(document.getElementById('latestActiveOption')!.textContent).toBe('b')
+    })
+  )
 })
diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -16,6 +16,7 @@ import React, {
   MutableRefObject,
   Ref,
   ContextType,
+  useEffect,
 } from 'react'
 
 import { useDisposables } from '../../hooks/use-disposables'
@@ -30,7 +31,6 @@ import { disposables } from '../../utils/disposables'
 import { Keys } from '../keyboard'
 import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
-import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
 import { useWindowEvent } from '../../hooks/use-window-event'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
@@ -181,7 +181,7 @@ ComboboxContext.displayName = 'ComboboxContext'
 function useComboboxContext(component: string) {
   let context = useContext(ComboboxContext)
   if (context === null) {
-    let err = new Error(`<${component} /> is missing a parent <${Combobox.name} /> component.`)
+    let err = new Error(`<${component} /> is missing a parent <Combobox /> component.`)
     if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxContext)
     throw err
   }
@@ -197,7 +197,7 @@ ComboboxActions.displayName = 'ComboboxActions'
 function useComboboxActions() {
   let context = useContext(ComboboxActions)
   if (context === null) {
-    let err = new Error(`ComboboxActions is missing a parent <${Combobox.name} /> component.`)
+    let err = new Error(`ComboboxActions is missing a parent <Combobox /> component.`)
     if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxActions)
     throw err
   }
@@ -216,14 +216,19 @@ interface ComboboxRenderPropArg<T> {
   disabled: boolean
   activeIndex: number | null
   activeOption: T | null
+  latestActiveOption: T | null
 }
 
-export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG, TType = string>(
+let ComboboxRoot = forwardRefWithAs(function Combobox<
+  TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
+  TType = string
+>(
   props: Props<TTag, ComboboxRenderPropArg<TType>, 'value' | 'onChange' | 'disabled'> & {
     value: TType
     onChange(value: TType): void
     disabled?: boolean
-  }
+  },
+  ref: Ref<TTag>
 ) {
   let { value, onChange, disabled = false, ...passThroughProps } = props
 
@@ -282,24 +287,28 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
     if (optionsRef.current?.contains(target)) return
 
     dispatch({ type: ActionTypes.CloseCombobox })
+  })
+
+  let latestActiveOption = useRef<TType | null>(null)
 
-    if (!isFocusableElement(target, FocusableMode.Loose)) {
-      event.preventDefault()
-      inputRef.current?.focus()
+  useEffect(() => {
+    if (activeOptionIndex !== null) {
+      latestActiveOption.current = options[activeOptionIndex].dataRef.current.value as TType
     }
-  })
+  }, [activeOptionIndex])
+
+  let activeOption =
+    activeOptionIndex === null ? null : (options[activeOptionIndex].dataRef.current.value as TType)
 
   let slot = useMemo<ComboboxRenderPropArg<TType>>(
     () => ({
       open: comboboxState === ComboboxStates.Open,
       disabled,
       activeIndex: activeOptionIndex,
-      activeOption:
-        activeOptionIndex === null
-          ? null
-          : (options[activeOptionIndex].dataRef.current.value as TType),
+      activeOption: activeOption,
+      latestActiveOption: activeOption ?? (latestActiveOption.current as TType),
     }),
-    [comboboxState, disabled, options, activeOptionIndex]
+    [comboboxState, disabled, options, activeOptionIndex, latestActiveOption]
   )
 
   let syncInputValue = useCallback(() => {
@@ -359,7 +368,13 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
           })}
         >
           {render({
-            props: passThroughProps,
+            props:
+              ref === null
+                ? passThroughProps
+                : {
+                    ...passThroughProps,
+                    ref,
+                  },
             slot,
             defaultTag: DEFAULT_COMBOBOX_TAG,
             name: 'Combobox',
@@ -368,7 +383,7 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
       </ComboboxContext.Provider>
     </ComboboxActions.Provider>
   )
-}
+})
 
 // ---
 
@@ -392,7 +407,7 @@ let Input = forwardRefWithAs(function Input<
   TTag extends ElementType = typeof DEFAULT_INPUT_TAG,
   // TODO: One day we will be able to infer this type from the generic in Combobox itself.
   // But today is not that day..
-  TType = Parameters<typeof Combobox>[0]['value']
+  TType = Parameters<typeof ComboboxRoot>[0]['value']
 >(
   props: Props<TTag, InputRenderPropArg, InputPropsWeControl> & {
     displayValue?(item: TType): string
@@ -807,7 +822,7 @@ function Option<
   TTag extends ElementType = typeof DEFAULT_OPTION_TAG,
   // TODO: One day we will be able to infer this type from the generic in Combobox itself.
   // But today is not that day..
-  TType = Parameters<typeof Combobox>[0]['value']
+  TType = Parameters<typeof ComboboxRoot>[0]['value']
 >(
   props: Props<TTag, OptionRenderPropArg, ComboboxOptionPropsWeControl | 'value'> & {
     disabled?: boolean
@@ -911,8 +926,10 @@ function Option<
 
 // ---
 
-Combobox.Input = Input
-Combobox.Button = Button
-Combobox.Label = Label
-Combobox.Options = Options
-Combobox.Option = Option
+export let Combobox = Object.assign(ComboboxRoot, {
+  Input,
+  Button,
+  Label,
+  Options,
+  Option,
+})
diff --git a/packages/@headlessui-react/src/test-utils/interactions.ts b/packages/@headlessui-react/src/test-utils/interactions.ts
@@ -92,6 +92,7 @@ let order: Record<
       return fireEvent.keyPress(element, event)
     },
     function input(element, event) {
+      // TODO: This should only fire when the element's value changes
       return fireEvent.input(element, event)
     },
     function keyup(element, event) {
@@ -139,6 +140,17 @@ let order: Record<
       return fireEvent.keyUp(element, event)
     },
   ],
+  [Keys.Escape.key!]: [
+    function keydown(element, event) {
+      return fireEvent.keyDown(element, event)
+    },
+    function keypress(element, event) {
+      return fireEvent.keyPress(element, event)
+    },
+    function keyup(element, event) {
+      return fireEvent.keyUp(element, event)
+    },
+  ],
 }
 
 export async function type(events: Partial<KeyboardEvent>[], element = document.activeElement) {