Skip to content

Commit 6897d2c

Browse files
RobinMalfaitRobinMalfait
authored
Fix required double ArrowDown requirement (#1281)
* fix double arrow down requirement If the `activeOptionIndex` is set to `null`, then we default to the very first non-disabled option. This data is _not_ stored in state because if you as the user go to a specific option, then start searching then we will maintain the active option. This means that we have to **update** the `activeOptionIndex` when options are moving around. While making the first option the active one, we can't store that in state directly otherwise the very first option becomes the active one. If we then inject combobox options _before_ the current one then all of a sudden your active option would jump around a bit. We don't want this jumping to happen, we want the very first option to be the one that's active no matter which option it is. Since this is not stored in state, our keydown handler was a bit borked. Internally it thinks we are still at `activeOptionIndex === null` therefore pressing arrow down would move us to `activeOptionIndex === 0`. To go to the second option, we can press down again which would move us to `activeOptionIndex === 1`. The only issue is that visually we were already at `0`. This fixes that by making sure that if we have `activeOptionIndex === null` that we fallback to the very first non disabled option _before_ we execute the `goToOption()` code. ### Before: **Open combobox**, `activeOptionIndex === null` | Combobox | | ----------------------- | | **Option A** _(active)_ | | Option B | | Option C | **Arrow Down**, `activeOptionIndex === 0` | Combobox | | ----------------------- | | **Option A** _(active)_ | | Option B | | Option C | **Arrow Down**, `activeOptionIndex === 1` | Combobox | | ----------------------- | | Option A | | **Option B** _(active)_ | | Option C | ### After: **Open combobox**, `activeOptionIndex === null` | Combobox | | ----------------------- | | **Option A** _(active)_ | | Option B | | Option C | **Arrow Down**, `activeOptionIndex === 1` | Combobox | | ----------------------- | | Option A | | **Option B** _(active)_ | | Option C | * update changelog
1 parent 419ffda commit 6897d2c

File tree

5 files changed

+116
-41
lines changed

5 files changed

+116
-41
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3333
- Properly merge incoming props ([#1265](https://github.com/tailwindlabs/headlessui/pull/1265))
3434
- Fix incorrect closing while interacting with third party libraries in `Dialog` component ([#1268](https://github.com/tailwindlabs/headlessui/pull/1268))
3535
- Mimic browser select on focus when navigating via `Tab` ([#1272](https://github.com/tailwindlabs/headlessui/pull/1272))
36-
- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279))
36+
- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279), [#1281](https://github.com/tailwindlabs/headlessui/pull/1281))
3737

3838
### Added
3939

@@ -69,7 +69,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
6969
- Fix incorrect closing while interacting with third party libraries in `Dialog` component ([#1268](https://github.com/tailwindlabs/headlessui/pull/1268))
7070
- Mimic browser select on focus when navigating via `Tab` ([#1272](https://github.com/tailwindlabs/headlessui/pull/1272))
7171
- Resolve `initialFocusRef` correctly ([#1276](https://github.com/tailwindlabs/headlessui/pull/1276))
72-
- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279))
72+
- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279), [#1281](https://github.com/tailwindlabs/headlessui/pull/1281))
7373

7474
### Added
7575

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

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -615,9 +615,6 @@ describe('Rendering', () => {
615615

616616
let options = getComboboxOptions()
617617

618-
// Focus the first item
619-
await press(Keys.ArrowDown)
620-
621618
// Verify that the first combobox option is active
622619
assertActiveComboboxOption(options[0])
623620

@@ -666,19 +663,10 @@ describe('Rendering composition', () => {
666663

667664
let options = getComboboxOptions()
668665

669-
// Verify correct classNames
670-
expect('' + options[0].classList).toEqual(
671-
JSON.stringify({ active: true, selected: false, disabled: false })
672-
)
673-
expect('' + options[1].classList).toEqual(
674-
JSON.stringify({ active: false, selected: false, disabled: true })
675-
)
676-
expect('' + options[2].classList).toEqual('no-special-treatment')
677-
678-
// Make the first option active
679-
await press(Keys.ArrowDown)
666+
// Verify that the first combobox option is active
667+
assertActiveComboboxOption(options[0])
680668

681-
// Verify the classNames
669+
// Verify correct classNames
682670
expect('' + options[0].classList).toEqual(
683671
JSON.stringify({ active: true, selected: false, disabled: false })
684672
)
@@ -687,9 +675,6 @@ describe('Rendering composition', () => {
687675
)
688676
expect('' + options[2].classList).toEqual('no-special-treatment')
689677

690-
// Double check that the first option is the active one
691-
assertActiveComboboxOption(options[0])
692-
693678
// Let's go down, this should go to the third option since the second option is disabled!
694679
await press(Keys.ArrowDown)
695680

@@ -1846,7 +1831,6 @@ describe('Keyboard interactions', () => {
18461831

18471832
// Select the 2nd option
18481833
await press(Keys.ArrowDown)
1849-
await press(Keys.ArrowDown)
18501834

18511835
// Tab to the next DOM node
18521836
await press(Keys.Tab)
@@ -1899,7 +1883,6 @@ describe('Keyboard interactions', () => {
18991883

19001884
// Select the 2nd option
19011885
await press(Keys.ArrowDown)
1902-
await press(Keys.ArrowDown)
19031886

19041887
// Tab to the next DOM node
19051888
await press(shift(Keys.Tab))
@@ -2274,17 +2257,14 @@ describe('Keyboard interactions', () => {
22742257

22752258
// We should be able to go down once
22762259
await press(Keys.ArrowDown)
2277-
assertActiveComboboxOption(options[0])
2278-
2279-
// We should be able to go down again
2280-
await press(Keys.ArrowDown)
22812260
assertActiveComboboxOption(options[1])
22822261

22832262
// We should be able to go down again
22842263
await press(Keys.ArrowDown)
22852264
assertActiveComboboxOption(options[2])
22862265

2287-
// We should NOT be able to go down again (because last option). Current implementation won't go around.
2266+
// We should NOT be able to go down again (because last option).
2267+
// Current implementation won't go around.
22882268
await press(Keys.ArrowDown)
22892269
assertActiveComboboxOption(options[2])
22902270
})
@@ -2324,7 +2304,7 @@ describe('Keyboard interactions', () => {
23242304

23252305
// We should be able to go down once
23262306
await press(Keys.ArrowDown)
2327-
assertActiveComboboxOption(options[1])
2307+
assertActiveComboboxOption(options[2])
23282308
})
23292309
)
23302310

@@ -2367,6 +2347,43 @@ describe('Keyboard interactions', () => {
23672347
assertActiveComboboxOption(options[2])
23682348
})
23692349
)
2350+
2351+
it(
2352+
'should be possible to go to the next item if no value is set',
2353+
suppressConsoleLogs(async () => {
2354+
render(
2355+
<Combobox value={null} onChange={console.log}>
2356+
<Combobox.Input onChange={NOOP} />
2357+
<Combobox.Button>Trigger</Combobox.Button>
2358+
<Combobox.Options>
2359+
<Combobox.Option value="a">Option A</Combobox.Option>
2360+
<Combobox.Option value="b">Option B</Combobox.Option>
2361+
<Combobox.Option value="c">Option C</Combobox.Option>
2362+
</Combobox.Options>
2363+
</Combobox>
2364+
)
2365+
2366+
assertComboboxButton({
2367+
state: ComboboxState.InvisibleUnmounted,
2368+
attributes: { id: 'headlessui-combobox-button-2' },
2369+
})
2370+
assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
2371+
2372+
// Open combobox
2373+
await click(getComboboxButton())
2374+
2375+
let options = getComboboxOptions()
2376+
2377+
// Verify that we are on the first option
2378+
assertActiveComboboxOption(options[0])
2379+
2380+
// Go down once
2381+
await press(Keys.ArrowDown)
2382+
2383+
// We should be on the next item
2384+
assertActiveComboboxOption(options[1])
2385+
})
2386+
)
23702387
})
23712388

23722389
describe('`ArrowUp` key', () => {
@@ -3440,7 +3457,6 @@ describe('Keyboard interactions', () => {
34403457

34413458
let options: ReturnType<typeof getComboboxOptions>
34423459

3443-
await press(Keys.ArrowDown)
34443460
await press(Keys.ArrowDown)
34453461

34463462
// Person B should be active

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,20 @@ let reducers: {
191191
}
192192

193193
let adjustedState = adjustOrderedState(state)
194+
195+
// It's possible that the activeOptionIndex is set to `null` internally, but
196+
// this means that we will fallback to the first non-disabled option by default.
197+
// We have to take this into account.
198+
if (adjustedState.activeOptionIndex === null) {
199+
let localActiveOptionIndex = adjustedState.options.findIndex(
200+
(option) => !option.dataRef.current.disabled
201+
)
202+
203+
if (localActiveOptionIndex !== -1) {
204+
adjustedState.activeOptionIndex = localActiveOptionIndex
205+
}
206+
}
207+
194208
let activeOptionIndex = calculateActiveIndex(action, {
195209
resolveItems: () => adjustedState.options,
196210
resolveActiveIndex: () => adjustedState.activeOptionIndex,

‎packages/@headlessui-vue/src/components/combobox/combobox.test.ts‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -732,9 +732,6 @@ describe('Rendering', () => {
732732

733733
let options = getComboboxOptions()
734734

735-
// Focus the first item
736-
await press(Keys.ArrowDown)
737-
738735
// Verify that the first combobox option is active
739736
assertActiveComboboxOption(options[0])
740737

@@ -1987,7 +1984,6 @@ describe('Keyboard interactions', () => {
19871984

19881985
// Select the 2nd option
19891986
await press(Keys.ArrowDown)
1990-
await press(Keys.ArrowDown)
19911987

19921988
// Tab to the next DOM node
19931989
await press(Keys.Tab)
@@ -2035,7 +2031,6 @@ describe('Keyboard interactions', () => {
20352031

20362032
// Select the 2nd option
20372033
await press(Keys.ArrowDown)
2038-
await press(Keys.ArrowDown)
20392034

20402035
// Tab to the next DOM node
20412036
await press(shift(Keys.Tab))
@@ -2437,17 +2432,14 @@ describe('Keyboard interactions', () => {
24372432

24382433
// We should be able to go down once
24392434
await press(Keys.ArrowDown)
2440-
assertActiveComboboxOption(options[0])
2441-
2442-
// We should be able to go down again
2443-
await press(Keys.ArrowDown)
24442435
assertActiveComboboxOption(options[1])
24452436

24462437
// We should be able to go down again
24472438
await press(Keys.ArrowDown)
24482439
assertActiveComboboxOption(options[2])
24492440

2450-
// We should NOT be able to go down again (because last option). Current implementation won't go around.
2441+
// We should NOT be able to go down again (because last option).
2442+
// Current implementation won't go around.
24512443
await press(Keys.ArrowDown)
24522444
assertActiveComboboxOption(options[2])
24532445
})
@@ -2488,7 +2480,7 @@ describe('Keyboard interactions', () => {
24882480

24892481
// We should be able to go down once
24902482
await press(Keys.ArrowDown)
2491-
assertActiveComboboxOption(options[1])
2483+
assertActiveComboboxOption(options[2])
24922484
})
24932485
)
24942486

@@ -2530,6 +2522,46 @@ describe('Keyboard interactions', () => {
25302522
assertActiveComboboxOption(options[2])
25312523
})
25322524
)
2525+
2526+
it(
2527+
'should be possible to go to the next item if no value is set',
2528+
suppressConsoleLogs(async () => {
2529+
renderTemplate({
2530+
template: html`
2531+
<Combobox v-model="value">
2532+
<ComboboxInput />
2533+
<ComboboxButton>Trigger</ComboboxButton>
2534+
<ComboboxOptions>
2535+
<ComboboxOption value="a">Option A</ComboboxOption>
2536+
<ComboboxOption value="b">Option B</ComboboxOption>
2537+
<ComboboxOption value="c">Option C</ComboboxOption>
2538+
</ComboboxOptions>
2539+
</Combobox>
2540+
`,
2541+
setup: () => ({ value: ref(null) }),
2542+
})
2543+
2544+
assertComboboxButton({
2545+
state: ComboboxState.InvisibleUnmounted,
2546+
attributes: { id: 'headlessui-combobox-button-2' },
2547+
})
2548+
assertComboboxList({ state: ComboboxState.InvisibleUnmounted })
2549+
2550+
// Open combobox
2551+
await click(getComboboxButton())
2552+
2553+
let options = getComboboxOptions()
2554+
2555+
// Verify that we are on the first option
2556+
assertActiveComboboxOption(options[0])
2557+
2558+
// Go down once
2559+
await press(Keys.ArrowDown)
2560+
2561+
// We should be on the next item
2562+
assertActiveComboboxOption(options[1])
2563+
})
2564+
)
25332565
})
25342566

25352567
describe('`ArrowUp` key', () => {
@@ -3631,7 +3663,6 @@ describe('Keyboard interactions', () => {
36313663

36323664
let options: ReturnType<typeof getComboboxOptions>
36333665

3634-
await press(Keys.ArrowDown)
36353666
await press(Keys.ArrowDown)
36363667

36373668
// Person B should be active

‎packages/@headlessui-vue/src/components/combobox/combobox.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,20 @@ export let Combobox = defineComponent({
234234
}
235235

236236
let adjustedState = adjustOrderedState()
237+
238+
// It's possible that the activeOptionIndex is set to `null` internally, but
239+
// this means that we will fallback to the first non-disabled option by default.
240+
// We have to take this into account.
241+
if (adjustedState.activeOptionIndex === null) {
242+
let localActiveOptionIndex = adjustedState.options.findIndex(
243+
(option) => !option.dataRef.disabled
244+
)
245+
246+
if (localActiveOptionIndex !== -1) {
247+
adjustedState.activeOptionIndex = localActiveOptionIndex
248+
}
249+
}
250+
237251
let nextActiveOptionIndex = calculateActiveIndex(
238252
focus === Focus.Specific
239253
? { focus: Focus.Specific, id: id! }

0 commit comments

Comments
 (0)