Skip to content

Commit 554d04b

Browse files
thecrypticacethecrypticace
authored
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
1 parent 6fc28c6 commit 554d04b

File tree

14 files changed

+749
-53
lines changed

14 files changed

+749
-53
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1717

1818
### Added
1919

20-
- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047))
20+
- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
2121

2222
## [Unreleased - @headlessui/vue]
2323

@@ -30,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3030

3131
### Added
3232

33-
- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047))
33+
- Add `Combobox` component ([#1047](https://github.com/tailwindlabs/headlessui/pull/1047), [#1099](https://github.com/tailwindlabs/headlessui/pull/1099))
3434

3535
## [@headlessui/react@v1.4.3] - 2022-01-14
3636

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
}
2929
},
3030
"lint-staged": {
31-
"*": "yarn lint-check"
31+
"*": "yarn lint"
3232
},
3333
"prettier": {
3434
"printWidth": 100,

‎packages/@headlessui-react/src/components/combobox/combobox.test.tsx‎

Lines changed: 84 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3588,19 +3588,26 @@ describe('Mouse interactions', () => {
35883588
})
35893589
)
35903590

3591-
it(
3591+
// TODO: JSDOM doesn't quite work here
3592+
// Clicking outside on the body should fire a mousedown (which it does) and then change the active element (which it doesn't)
3593+
xit(
35923594
'should be possible to click outside of the combobox which should close the combobox',
35933595
suppressConsoleLogs(async () => {
35943596
render(
3595-
<Combobox value="test" onChange={console.log}>
3596-
<Combobox.Input onChange={NOOP} />
3597-
<Combobox.Button>Trigger</Combobox.Button>
3598-
<Combobox.Options>
3599-
<Combobox.Option value="alice">alice</Combobox.Option>
3600-
<Combobox.Option value="bob">bob</Combobox.Option>
3601-
<Combobox.Option value="charlie">charlie</Combobox.Option>
3602-
</Combobox.Options>
3603-
</Combobox>
3597+
<>
3598+
<Combobox value="test" onChange={console.log}>
3599+
<Combobox.Input onChange={NOOP} />
3600+
<Combobox.Button>Trigger</Combobox.Button>
3601+
<Combobox.Options>
3602+
<Combobox.Option value="alice">alice</Combobox.Option>
3603+
<Combobox.Option value="bob">bob</Combobox.Option>
3604+
<Combobox.Option value="charlie">charlie</Combobox.Option>
3605+
</Combobox.Options>
3606+
</Combobox>
3607+
<div tabIndex={-1} data-test-focusable>
3608+
after
3609+
</div>
3610+
</>
36043611
)
36053612

36063613
// Open combobox
@@ -3609,13 +3616,13 @@ describe('Mouse interactions', () => {
36093616
assertActiveElement(getComboboxInput())
36103617

36113618
// Click something that is not related to the combobox
3612-
await click(document.body)
3619+
await click(getByText('after'))
36133620

36143621
// Should be closed now
36153622
assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
36163623

3617-
// Verify the input is focused again
3618-
assertActiveElement(getComboboxInput())
3624+
// Verify the button is focused
3625+
assertActiveElement(getByText('after'))
36193626
})
36203627
)
36213628

@@ -4130,4 +4137,68 @@ describe('Mouse interactions', () => {
41304137
assertNoActiveComboboxOption()
41314138
})
41324139
)
4140+
4141+
it(
4142+
'Combobox preserves the latest known active option after an option becomes inactive',
4143+
suppressConsoleLogs(async () => {
4144+
render(
4145+
<Combobox value="test" onChange={console.log}>
4146+
{({ open, latestActiveOption }) => (
4147+
<>
4148+
<Combobox.Input onChange={NOOP} />
4149+
<Combobox.Button>Trigger</Combobox.Button>
4150+
<div id="latestActiveOption">{latestActiveOption}</div>
4151+
{open && (
4152+
<Combobox.Options>
4153+
<Combobox.Option value="a">Option A</Combobox.Option>
4154+
<Combobox.Option value="b">Option B</Combobox.Option>
4155+
<Combobox.Option value="c">Option C</Combobox.Option>
4156+
</Combobox.Options>
4157+
)}
4158+
</>
4159+
)}
4160+
</Combobox>
4161+
)
4162+
4163+
assertComboboxButton({
4164+
state: ComboboxState.InvisibleUnmounted,
4165+
attributes: { id: 'headlessui-combobox-button-2' },
4166+
})
4167+
assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
4168+
4169+
await click(getComboboxButton())
4170+
4171+
assertComboboxButton({
4172+
state: ComboboxState.Visible,
4173+
attributes: { id: 'headlessui-combobox-button-2' },
4174+
})
4175+
assertComboboxList({ state: ComboboxState.Visible })
4176+
4177+
let options = getComboboxOptions()
4178+
4179+
// Hover the first item
4180+
await mouseMove(options[0])
4181+
4182+
// Verify that the first combobox option is active
4183+
assertActiveComboboxOption(options[0])
4184+
expect(document.getElementById('latestActiveOption')!.textContent).toBe('a')
4185+
4186+
// Focus the second item
4187+
await mouseMove(options[1])
4188+
4189+
// Verify that the second combobox option is active
4190+
assertActiveComboboxOption(options[1])
4191+
expect(document.getElementById('latestActiveOption')!.textContent).toBe('b')
4192+
4193+
// Move the mouse off of the second combobox option
4194+
await mouseLeave(options[1])
4195+
await mouseMove(document.body)
4196+
4197+
// Verify that the second combobox option is NOT active
4198+
assertNoActiveComboboxOption()
4199+
4200+
// But the last known active option is still recorded
4201+
expect(document.getElementById('latestActiveOption')!.textContent).toBe('b')
4202+
})
4203+
)
41334204
})

‎packages/@headlessui-react/src/components/combobox/combobox.tsx‎

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import React, {
1616
MutableRefObject,
1717
Ref,
1818
ContextType,
19+
useEffect,
1920
} from 'react'
2021

2122
import { useDisposables } from '../../hooks/use-disposables'
@@ -30,7 +31,6 @@ import { disposables } from '../../utils/disposables'
3031
import { Keys } from '../keyboard'
3132
import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
3233
import { isDisabledReactIssue7711 } from '../../utils/bugs'
33-
import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
3434
import { useWindowEvent } from '../../hooks/use-window-event'
3535
import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
3636
import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
@@ -181,7 +181,7 @@ ComboboxContext.displayName = 'ComboboxContext'
181181
function useComboboxContext(component: string) {
182182
let context = useContext(ComboboxContext)
183183
if (context === null) {
184-
let err = new Error(`<${component} /> is missing a parent <${Combobox.name} /> component.`)
184+
let err = new Error(`<${component} /> is missing a parent <Combobox /> component.`)
185185
if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxContext)
186186
throw err
187187
}
@@ -197,7 +197,7 @@ ComboboxActions.displayName = 'ComboboxActions'
197197
function useComboboxActions() {
198198
let context = useContext(ComboboxActions)
199199
if (context === null) {
200-
let err = new Error(`ComboboxActions is missing a parent <${Combobox.name} /> component.`)
200+
let err = new Error(`ComboboxActions is missing a parent <Combobox /> component.`)
201201
if (Error.captureStackTrace) Error.captureStackTrace(err, useComboboxActions)
202202
throw err
203203
}
@@ -216,14 +216,19 @@ interface ComboboxRenderPropArg<T> {
216216
disabled: boolean
217217
activeIndex: number | null
218218
activeOption: T | null
219+
latestActiveOption: T | null
219220
}
220221

221-
export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG, TType = string>(
222+
let ComboboxRoot = forwardRefWithAs(function Combobox<
223+
TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
224+
TType = string
225+
>(
222226
props: Props<TTag, ComboboxRenderPropArg<TType>, 'value' | 'onChange' | 'disabled'> & {
223227
value: TType
224228
onChange(value: TType): void
225229
disabled?: boolean
226-
}
230+
},
231+
ref: Ref<TTag>
227232
) {
228233
let { value, onChange, disabled = false, ...passThroughProps } = props
229234

@@ -282,24 +287,28 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
282287
if (optionsRef.current?.contains(target)) return
283288

284289
dispatch({ type: ActionTypes.CloseCombobox })
290+
})
291+
292+
let latestActiveOption = useRef<TType | null>(null)
285293

286-
if (!isFocusableElement(target, FocusableMode.Loose)) {
287-
event.preventDefault()
288-
inputRef.current?.focus()
294+
useEffect(() => {
295+
if (activeOptionIndex !== null) {
296+
latestActiveOption.current = options[activeOptionIndex].dataRef.current.value as TType
289297
}
290-
})
298+
}, [activeOptionIndex])
299+
300+
let activeOption =
301+
activeOptionIndex === null ? null : (options[activeOptionIndex].dataRef.current.value as TType)
291302

292303
let slot = useMemo<ComboboxRenderPropArg<TType>>(
293304
() => ({
294305
open: comboboxState === ComboboxStates.Open,
295306
disabled,
296307
activeIndex: activeOptionIndex,
297-
activeOption:
298-
activeOptionIndex === null
299-
? null
300-
: (options[activeOptionIndex].dataRef.current.value as TType),
308+
activeOption: activeOption,
309+
latestActiveOption: activeOption ?? (latestActiveOption.current as TType),
301310
}),
302-
[comboboxState, disabled, options, activeOptionIndex]
311+
[comboboxState, disabled, options, activeOptionIndex, latestActiveOption]
303312
)
304313

305314
let syncInputValue = useCallback(() => {
@@ -359,7 +368,13 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
359368
})}
360369
>
361370
{render({
362-
props: passThroughProps,
371+
props:
372+
ref === null
373+
? passThroughProps
374+
: {
375+
...passThroughProps,
376+
ref,
377+
},
363378
slot,
364379
defaultTag: DEFAULT_COMBOBOX_TAG,
365380
name: 'Combobox',
@@ -368,7 +383,7 @@ export function Combobox<TTag extends ElementType = typeof DEFAULT_COMBOBOX_TAG,
368383
</ComboboxContext.Provider>
369384
</ComboboxActions.Provider>
370385
)
371-
}
386+
})
372387

373388
// ---
374389

@@ -392,7 +407,7 @@ let Input = forwardRefWithAs(function Input<
392407
TTag extends ElementType = typeof DEFAULT_INPUT_TAG,
393408
// TODO: One day we will be able to infer this type from the generic in Combobox itself.
394409
// But today is not that day..
395-
TType = Parameters<typeof Combobox>[0]['value']
410+
TType = Parameters<typeof ComboboxRoot>[0]['value']
396411
>(
397412
props: Props<TTag, InputRenderPropArg, InputPropsWeControl> & {
398413
displayValue?(item: TType): string
@@ -807,7 +822,7 @@ function Option<
807822
TTag extends ElementType = typeof DEFAULT_OPTION_TAG,
808823
// TODO: One day we will be able to infer this type from the generic in Combobox itself.
809824
// But today is not that day..
810-
TType = Parameters<typeof Combobox>[0]['value']
825+
TType = Parameters<typeof ComboboxRoot>[0]['value']
811826
>(
812827
props: Props<TTag, OptionRenderPropArg, ComboboxOptionPropsWeControl | 'value'> & {
813828
disabled?: boolean
@@ -911,8 +926,10 @@ function Option<
911926

912927
// ---
913928

914-
Combobox.Input = Input
915-
Combobox.Button = Button
916-
Combobox.Label = Label
917-
Combobox.Options = Options
918-
Combobox.Option = Option
929+
export let Combobox = Object.assign(ComboboxRoot, {
930+
Input,
931+
Button,
932+
Label,
933+
Options,
934+
Option,
935+
})

‎packages/@headlessui-react/src/test-utils/interactions.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ let order: Record<
9292
return fireEvent.keyPress(element, event)
9393
},
9494
function input(element, event) {
95+
// TODO: This should only fire when the element's value changes
9596
return fireEvent.input(element, event)
9697
},
9798
function keyup(element, event) {
@@ -139,6 +140,17 @@ let order: Record<
139140
return fireEvent.keyUp(element, event)
140141
},
141142
],
143+
[Keys.Escape.key!]: [
144+
function keydown(element, event) {
145+
return fireEvent.keyDown(element, event)
146+
},
147+
function keypress(element, event) {
148+
return fireEvent.keyPress(element, event)
149+
},
150+
function keyup(element, event) {
151+
return fireEvent.keyUp(element, event)
152+
},
153+
],
142154
}
143155

144156
export async function type(events: Partial<KeyboardEvent>[], element = document.activeElement) {

0 commit comments

Comments
 (0)