From 6897d2ccf13a93944000169f0d199ffacec4070d Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 29 Mar 2022 18:52:44 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 4 +- .../src/components/combobox/combobox.test.tsx | 70 ++++++++++++------- .../src/components/combobox/combobox.tsx | 14 ++++ .../src/components/combobox/combobox.test.ts | 55 +++++++++++---- .../src/components/combobox/combobox.ts | 14 ++++ 5 files changed, 116 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d2601a..209d7f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Properly merge incoming props ([#1265](https://github.com/tailwindlabs/headlessui/pull/1265)) - Fix incorrect closing while interacting with third party libraries in `Dialog` component ([#1268](https://github.com/tailwindlabs/headlessui/pull/1268)) - Mimic browser select on focus when navigating via `Tab` ([#1272](https://github.com/tailwindlabs/headlessui/pull/1272)) -- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279)) +- 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)) ### Added @@ -69,7 +69,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix incorrect closing while interacting with third party libraries in `Dialog` component ([#1268](https://github.com/tailwindlabs/headlessui/pull/1268)) - Mimic browser select on focus when navigating via `Tab` ([#1272](https://github.com/tailwindlabs/headlessui/pull/1272)) - Resolve `initialFocusRef` correctly ([#1276](https://github.com/tailwindlabs/headlessui/pull/1276)) -- Ensure that there is always an active option in the `Combobox` ([#1279](https://github.com/tailwindlabs/headlessui/pull/1279)) +- 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)) ### Added diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index b7a3920..b5a2d70 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -615,9 +615,6 @@ describe('Rendering', () => { let options = getComboboxOptions() - // Focus the first item - await press(Keys.ArrowDown) - // Verify that the first combobox option is active assertActiveComboboxOption(options[0]) @@ -666,6 +663,9 @@ describe('Rendering composition', () => { let options = getComboboxOptions() + // Verify that the first combobox option is active + assertActiveComboboxOption(options[0]) + // Verify correct classNames expect('' + options[0].classList).toEqual( JSON.stringify({ active: true, selected: false, disabled: false }) @@ -675,21 +675,6 @@ describe('Rendering composition', () => { ) expect('' + options[2].classList).toEqual('no-special-treatment') - // Make the first option active - await press(Keys.ArrowDown) - - // Verify the classNames - expect('' + options[0].classList).toEqual( - JSON.stringify({ active: true, selected: false, disabled: false }) - ) - expect('' + options[1].classList).toEqual( - JSON.stringify({ active: false, selected: false, disabled: true }) - ) - expect('' + options[2].classList).toEqual('no-special-treatment') - - // Double check that the first option is the active one - assertActiveComboboxOption(options[0]) - // Let's go down, this should go to the third option since the second option is disabled! await press(Keys.ArrowDown) @@ -1846,7 +1831,6 @@ describe('Keyboard interactions', () => { // Select the 2nd option await press(Keys.ArrowDown) - await press(Keys.ArrowDown) // Tab to the next DOM node await press(Keys.Tab) @@ -1899,7 +1883,6 @@ describe('Keyboard interactions', () => { // Select the 2nd option await press(Keys.ArrowDown) - await press(Keys.ArrowDown) // Tab to the next DOM node await press(shift(Keys.Tab)) @@ -2274,17 +2257,14 @@ describe('Keyboard interactions', () => { // We should be able to go down once await press(Keys.ArrowDown) - assertActiveComboboxOption(options[0]) - - // We should be able to go down again - await press(Keys.ArrowDown) assertActiveComboboxOption(options[1]) // We should be able to go down again await press(Keys.ArrowDown) assertActiveComboboxOption(options[2]) - // We should NOT be able to go down again (because last option). Current implementation won't go around. + // We should NOT be able to go down again (because last option). + // Current implementation won't go around. await press(Keys.ArrowDown) assertActiveComboboxOption(options[2]) }) @@ -2324,7 +2304,7 @@ describe('Keyboard interactions', () => { // We should be able to go down once await press(Keys.ArrowDown) - assertActiveComboboxOption(options[1]) + assertActiveComboboxOption(options[2]) }) ) @@ -2367,6 +2347,43 @@ describe('Keyboard interactions', () => { assertActiveComboboxOption(options[2]) }) ) + + it( + 'should be possible to go to the next item if no value is set', + suppressConsoleLogs(async () => { + render( + + + Trigger + + Option A + Option B + Option C + + + ) + + assertComboboxButton({ + state: ComboboxState.InvisibleUnmounted, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) + + // Open combobox + await click(getComboboxButton()) + + let options = getComboboxOptions() + + // Verify that we are on the first option + assertActiveComboboxOption(options[0]) + + // Go down once + await press(Keys.ArrowDown) + + // We should be on the next item + assertActiveComboboxOption(options[1]) + }) + ) }) describe('`ArrowUp` key', () => { @@ -3440,7 +3457,6 @@ describe('Keyboard interactions', () => { let options: ReturnType - await press(Keys.ArrowDown) await press(Keys.ArrowDown) // Person B should be active diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index aac2bf7..d277298 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -191,6 +191,20 @@ let reducers: { } let adjustedState = adjustOrderedState(state) + + // It's possible that the activeOptionIndex is set to `null` internally, but + // this means that we will fallback to the first non-disabled option by default. + // We have to take this into account. + if (adjustedState.activeOptionIndex === null) { + let localActiveOptionIndex = adjustedState.options.findIndex( + (option) => !option.dataRef.current.disabled + ) + + if (localActiveOptionIndex !== -1) { + adjustedState.activeOptionIndex = localActiveOptionIndex + } + } + let activeOptionIndex = calculateActiveIndex(action, { resolveItems: () => adjustedState.options, resolveActiveIndex: () => adjustedState.activeOptionIndex, diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index 55d6e11..427fcae 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -732,9 +732,6 @@ describe('Rendering', () => { let options = getComboboxOptions() - // Focus the first item - await press(Keys.ArrowDown) - // Verify that the first combobox option is active assertActiveComboboxOption(options[0]) @@ -1987,7 +1984,6 @@ describe('Keyboard interactions', () => { // Select the 2nd option await press(Keys.ArrowDown) - await press(Keys.ArrowDown) // Tab to the next DOM node await press(Keys.Tab) @@ -2035,7 +2031,6 @@ describe('Keyboard interactions', () => { // Select the 2nd option await press(Keys.ArrowDown) - await press(Keys.ArrowDown) // Tab to the next DOM node await press(shift(Keys.Tab)) @@ -2437,17 +2432,14 @@ describe('Keyboard interactions', () => { // We should be able to go down once await press(Keys.ArrowDown) - assertActiveComboboxOption(options[0]) - - // We should be able to go down again - await press(Keys.ArrowDown) assertActiveComboboxOption(options[1]) // We should be able to go down again await press(Keys.ArrowDown) assertActiveComboboxOption(options[2]) - // We should NOT be able to go down again (because last option). Current implementation won't go around. + // We should NOT be able to go down again (because last option). + // Current implementation won't go around. await press(Keys.ArrowDown) assertActiveComboboxOption(options[2]) }) @@ -2488,7 +2480,7 @@ describe('Keyboard interactions', () => { // We should be able to go down once await press(Keys.ArrowDown) - assertActiveComboboxOption(options[1]) + assertActiveComboboxOption(options[2]) }) ) @@ -2530,6 +2522,46 @@ describe('Keyboard interactions', () => { assertActiveComboboxOption(options[2]) }) ) + + it( + 'should be possible to go to the next item if no value is set', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + + Trigger + + Option A + Option B + Option C + + + `, + setup: () => ({ value: ref(null) }), + }) + + assertComboboxButton({ + state: ComboboxState.InvisibleUnmounted, + attributes: { id: 'headlessui-combobox-button-2' }, + }) + assertComboboxList({ state: ComboboxState.InvisibleUnmounted }) + + // Open combobox + await click(getComboboxButton()) + + let options = getComboboxOptions() + + // Verify that we are on the first option + assertActiveComboboxOption(options[0]) + + // Go down once + await press(Keys.ArrowDown) + + // We should be on the next item + assertActiveComboboxOption(options[1]) + }) + ) }) describe('`ArrowUp` key', () => { @@ -3631,7 +3663,6 @@ describe('Keyboard interactions', () => { let options: ReturnType - await press(Keys.ArrowDown) await press(Keys.ArrowDown) // Person B should be active diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index a90b3a3..d807388 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -234,6 +234,20 @@ export let Combobox = defineComponent({ } let adjustedState = adjustOrderedState() + + // It's possible that the activeOptionIndex is set to `null` internally, but + // this means that we will fallback to the first non-disabled option by default. + // We have to take this into account. + if (adjustedState.activeOptionIndex === null) { + let localActiveOptionIndex = adjustedState.options.findIndex( + (option) => !option.dataRef.disabled + ) + + if (localActiveOptionIndex !== -1) { + adjustedState.activeOptionIndex = localActiveOptionIndex + } + } + let nextActiveOptionIndex = calculateActiveIndex( focus === Focus.Specific ? { focus: Focus.Specific, id: id! }