Combobox improvements (#1101)

* ensure combobox option gets activated on hover (while static) * rename combobox test file * remove leftover `horizontal` prop * remove unnecessary handleLeave calls These are implemented on the `Combobox.Option` instead of the `Combobox.Options`. This allows you to have additional visual padding between `Combobox.Options` and `Combobox.Option` and if you hover over that area then the option becomes inactive. If we implement it on the `Combobox.Options` instead then this isn't _that_ easy to do. We can do it by checking the target and whether or not it is inside a headlessui-combobox-option. This would only have a single listener instead of `N` listeners though. Potential improvements! * implement `hold` in favor of `latestActiveOption` * update changelog * Allow Escape to bubble when options is static You’ve taken control of the open/close state yourself in which case this should be allowed to be handled by other event handlers Co-authored-by: Jordan Pittman <[email protected]>
tailwindlabs · Feb 10, 2022 · 4ed344a · 4ed344a
1 parent dcf2f75
commit 4ed344a
Show file tree

Hide file tree

Showing 5 changed files with 298 additions and 132 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,7 +18,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), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
+- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099), [#1101](https://github.com/tailwindlabs/headlessui/pull/1101))
 
 ## [Unreleased - @headlessui/vue]
 
@@ -32,7 +32,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), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
+- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099), [#1101](https://github.com/tailwindlabs/headlessui/pull/1101))
 
 ## [@headlessui/react@v1.4.3] - 2022-01-14
 

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -1387,6 +1387,71 @@ describe('Keyboard interactions', () => {
           assertActiveElement(getComboboxInput())
         })
       )
+
+      it(
+        'Static options should allow escape to bubble',
+        suppressConsoleLogs(async () => {
+          render(
+            <Combobox value="test" onChange={console.log}>
+              <Combobox.Input onChange={NOOP} />
+              <Combobox.Button>Trigger</Combobox.Button>
+              <Combobox.Options static>
+                <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>
+          )
+
+          let spy = jest.fn()
+
+          window.addEventListener(
+            'keydown',
+            (evt) => {
+              if (evt.key === 'Escape') {
+                spy()
+              }
+            },
+            { capture: true }
+          )
+
+          window.addEventListener('keydown', (evt) => {
+            if (evt.key === 'Escape') {
+              spy()
+            }
+          })
+
+          // Open combobox
+          await click(getComboboxButton())
+
+          // Verify it is visible
+          assertComboboxButton({ state: ComboboxState.Visible })
+          assertComboboxList({
+            state: ComboboxState.Visible,
+            attributes: { id: 'headlessui-combobox-options-3' },
+          })
+          assertActiveElement(getComboboxInput())
+          assertComboboxButtonLinkedWithCombobox()
+
+          // Re-focus the button
+          getComboboxButton()?.focus()
+          assertActiveElement(getComboboxButton())
+
+          // Close combobox
+          await press(Keys.Escape)
+
+          // TODO: Verify it is rendered — with static it's not visible or invisible from an assert perspective
+          // assertComboboxButton({ state: ComboboxState.InvisibleUnmounted })
+          // assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
+
+          // Verify the input is focused again
+          assertActiveElement(getComboboxInput())
+
+          // The external event handler should've been called twice
+          // Once in the capture phase and once in the bubble phase
+          expect(spy).toHaveBeenCalledTimes(2)
+        })
+      )
     })
 
     describe('`ArrowDown` key', () => {
@@ -3778,6 +3843,36 @@ describe('Mouse interactions', () => {
     })
   )
 
+  it(
+    'should be possible to hover an option and make it active when using `static`',
+    suppressConsoleLogs(async () => {
+      render(
+        <Combobox value="test" onChange={console.log}>
+          <Combobox.Input onChange={NOOP} />
+          <Combobox.Button>Trigger</Combobox.Button>
+          <Combobox.Options static>
+            <Combobox.Option value="alice">alice</Combobox.Option>
+            <Combobox.Option value="bob">bob</Combobox.Option>
+            <Combobox.Option value="charlie">charlie</Combobox.Option>
+          </Combobox.Options>
+        </Combobox>
+      )
+
+      let options = getComboboxOptions()
+      // We should be able to go to the second option
+      await mouseMove(options[1])
+      assertActiveComboboxOption(options[1])
+
+      // We should be able to go to the first option
+      await mouseMove(options[0])
+      assertActiveComboboxOption(options[0])
+
+      // We should be able to go to the last option
+      await mouseMove(options[2])
+      assertActiveComboboxOption(options[2])
+    })
+  )
+
   it(
     'should make a combobox option active when you move the mouse over it',
     suppressConsoleLogs(async () => {
@@ -4139,24 +4234,17 @@ describe('Mouse interactions', () => {
   )
 
   it(
-    'Combobox preserves the latest known active option after an option becomes inactive',
+    'should be possible to hold the last active option',
     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 value="test" onChange={console.log} hold>
+          <Combobox.Input onChange={NOOP} />
+          <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>
       )
 
@@ -4181,24 +4269,19 @@ describe('Mouse interactions', () => {
 
       // 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')
+      // Verify that the second combobox option is still active
+      assertActiveComboboxOption(options[1])
     })
   )
 })