From 706f42b9d771e2594f0f8199528780aceccc09ea Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 11 Feb 2022 15:24:30 +0100 Subject: [PATCH] Bubble Escape event even if `Combobox.Options` is not rendered at all (#1104) * bubble Escape event even if `Combobox.Options` is not rendered at all If you use `` 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 && ( ... )} ``` 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. * update changelog --- CHANGELOG.md | 5 +- .../src/components/combobox/combobox.test.tsx | 162 ++++++++++------- .../src/components/combobox/combobox.tsx | 10 +- .../src/components/combobox/combobox.test.ts | 171 +++++++++++------- .../src/components/combobox/combobox.ts | 11 +- 5 files changed, 218 insertions(+), 141 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f26976eb5e..ad5c714843 100644 --- 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), [#1101](https://github.com/tailwindlabs/headlessui/pull/1101)) +- 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), [#1104](https://github.com/tailwindlabs/headlessui/pull/1104)) ## [Unreleased - @headlessui/vue] @@ -32,7 +32,8 @@ 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), [#1101](https://github.com/tailwindlabs/headlessui/pull/1101)) +- 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), - Bubble Escape event even if `Combobox.Options` is not rendered at all () + ) ## [@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 index 964c1a89e5..f68da594d1 100644 --- 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( - - - Trigger - - Option A - Option B - Option C - - - ) - - 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( + + + Trigger + + Option A + Option B + Option C + + + ) + + 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( + + + Trigger + + ) + + 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 index a03e06f75c..a867283ed3 100644 --- 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 { assertActiveElement(getComboboxInput()) }) ) - - it( - 'Static options should allow escape to bubble', - suppressConsoleLogs(async () => { - renderTemplate({ - template: html` - - - Trigger - - Option A - Option B - Option C - - - `, - 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` + + + Trigger + + Option A + Option B + Option C + + + `, + 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` + + + Trigger + + `, + 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', () => { diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index 285f238182..a869914d78 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -130,7 +130,12 @@ export let Combobox = defineComponent({ }, goToOption(focus: Focus, id?: string) { if (props.disabled) return - if (!optionsPropsRef.value.static && comboboxState.value === ComboboxStates.Closed) return + if ( + optionsRef.value && + !optionsPropsRef.value.static && + comboboxState.value === ComboboxStates.Closed + ) + return let nextActiveOptionIndex = calculateActiveIndex( focus === Focus.Specific @@ -363,7 +368,7 @@ export let ComboboxButton = defineComponent({ case Keys.Escape: event.preventDefault() - if (!api.optionsPropsRef.value.static) { + if (api.optionsRef.value && !api.optionsPropsRef.value.static) { event.stopPropagation() } api.closeCombobox() @@ -483,7 +488,7 @@ export let ComboboxInput = defineComponent({ case Keys.Escape: event.preventDefault() - if (!api.optionsPropsRef.value.static) { + if (api.optionsRef.value && !api.optionsPropsRef.value.static) { event.stopPropagation() } api.closeCombobox()