bubble Escape event even if Combobox.Options is not rendered at all

If you use `<Combobox.Options static />` it means that you are in control of rendering and in that case we also bubble the `Escape` because you are in control of it. However, if you do something like this: ```js {filteredList.length > 0 && ( <Combobox.Options static> ... </Combobox.Options> )} ``` Then whenever the `filteredList` is empty, the Combobox.Options are not rendered at all which means that we can't look at the `static` prop. To fix this, we also bubble the `Escape` event if we don't have a `Combobox.Options` at all so that the above example works as expected.
tailwindlabs · Feb 11, 2022 · a902dd9 · a902dd9
1 parent 4ed344a
commit a902dd9
Show file tree

Hide file tree

Showing 4 changed files with 215 additions and 139 deletions.
diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx
@@ -1387,71 +1387,6 @@ 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', () => {
@@ -2021,6 +1956,103 @@ describe('Keyboard interactions', () => {
           assertActiveElement(getComboboxInput())
         })
       )
+
+      it(
+        'should bubble escape when using `static` on Combobox.Options',
+        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 the input is focused
+          assertActiveElement(getComboboxInput())
+
+          // Close combobox
+          await press(Keys.Escape)
+
+          // Verify the input is still focused
+          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)
+        })
+      )
+
+      it(
+        'should bubble escape when not using Combobox.Options at all',
+        suppressConsoleLogs(async () => {
+          render(
+            <Combobox value="test" onChange={console.log}>
+              <Combobox.Input onChange={NOOP} />
+              <Combobox.Button>Trigger</Combobox.Button>
+            </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 the input is focused
+          assertActiveElement(getComboboxInput())
+
+          // Close combobox
+          await press(Keys.Escape)
+
+          // Verify the input is still focused
+          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', () => {

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -114,7 +114,11 @@ let reducers: {
   },
   [ActionTypes.GoToOption](state, action) {
     if (state.disabled) return state
-    if (!state.optionsPropsRef.current.static && state.comboboxState === ComboboxStates.Closed)
+    if (
+      state.optionsRef.current &&
+      !state.optionsPropsRef.current.static &&
+      state.comboboxState === ComboboxStates.Closed
+    )
       return state
 
     let activeOptionIndex = calculateActiveIndex(action, {
@@ -480,7 +484,7 @@ let Input = forwardRefWithAs(function Input<
 
         case Keys.Escape:
           event.preventDefault()
-          if (!state.optionsPropsRef.current.static) {
+          if (state.optionsRef.current && !state.optionsPropsRef.current.static) {
             event.stopPropagation()
           }
           return dispatch({ type: ActionTypes.CloseCombobox })
@@ -607,7 +611,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
 
         case Keys.Escape:
           event.preventDefault()
-          if (!state.optionsPropsRef.current.static) {
+          if (state.optionsRef.current && !state.optionsPropsRef.current.static) {
             event.stopPropagation()
           }
           dispatch({ type: ActionTypes.CloseCombobox })

diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts
@@ -1513,74 +1513,6 @@ describe('Keyboard interactions', () => {
           assertActiveElement(getComboboxInput())
         })
       )
-
-      it(
-        'Static options should allow escape to bubble',
-        suppressConsoleLogs(async () => {
-          renderTemplate({
-            template: html`
-              <Combobox v-model="value">
-                <ComboboxInput />
-                <ComboboxButton>Trigger</ComboboxButton>
-                <ComboboxOptions static>
-                  <ComboboxOption value="a">Option A</ComboboxOption>
-                  <ComboboxOption value="b">Option B</ComboboxOption>
-                  <ComboboxOption value="c">Option C</ComboboxOption>
-                </ComboboxOptions>
-              </Combobox>
-            `,
-            setup: () => ({ value: ref(null) }),
-          })
-
-          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', () => {
@@ -2160,6 +2092,109 @@ describe('Keyboard interactions', () => {
           assertActiveElement(getComboboxInput())
         })
       )
+
+      it(
+        'should bubble escape when using `static` on Combobox.Options',
+        suppressConsoleLogs(async () => {
+          renderTemplate({
+            template: html`
+              <Combobox v-model="value">
+                <ComboboxInput />
+                <ComboboxButton>Trigger</ComboboxButton>
+                <ComboboxOptions static>
+                  <ComboboxOption value="a">Option A</ComboboxOption>
+                  <ComboboxOption value="b">Option B</ComboboxOption>
+                  <ComboboxOption value="c">Option C</ComboboxOption>
+                </ComboboxOptions>
+              </Combobox>
+            `,
+            setup: () => ({ value: ref(null) }),
+          })
+
+          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 the input is focused
+          assertActiveElement(getComboboxInput())
+
+          // Close combobox
+          await press(Keys.Escape)
+
+          // Verify the input is still focused
+          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)
+        })
+      )
+
+      it(
+        'should bubble escape when not using Combobox.Options at all',
+        suppressConsoleLogs(async () => {
+          renderTemplate({
+            template: html`
+              <Combobox v-model="value">
+                <ComboboxInput />
+                <ComboboxButton>Trigger</ComboboxButton>
+              </Combobox>
+            `,
+            setup: () => ({ value: ref(null) }),
+          })
+
+          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 the input is focused
+          assertActiveElement(getComboboxInput())
+
+          // Close combobox
+          await press(Keys.Escape)
+
+          // Verify the input is still focused
+          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', () => {