Skip to content

Commit

Permalink
Bubble Escape event even if Combobox.Options is not rendered at all (
Browse files Browse the repository at this point in the history
…#1104)

* 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.

* update changelog
  • Loading branch information
RobinMalfait authored Feb 11, 2022
1 parent 4ed344a commit 706f42b
Show file tree
Hide file tree
Showing 5 changed files with 218 additions and 141 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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

Expand Down
162 changes: 97 additions & 65 deletions packages/@headlessui-react/src/components/combobox/combobox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
10 changes: 7 additions & 3 deletions packages/@headlessui-react/src/components/combobox/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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 })
Expand Down
Loading

0 comments on commit 706f42b

Please sign in to comment.