Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix required double ArrowDown requirement #1281

Merged
merged 2 commits into from
Mar 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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])

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

let options = getComboboxOptions()

// Verify correct 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')

// Make the first option active
await press(Keys.ArrowDown)
// Verify that the first combobox option is active
assertActiveComboboxOption(options[0])

// Verify the classNames
// Verify correct classNames
expect('' + options[0].classList).toEqual(
JSON.stringify({ active: true, selected: false, disabled: false })
)
Expand All @@ -687,9 +675,6 @@ describe('Rendering composition', () => {
)
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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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])
})
Expand Down Expand Up @@ -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])
})
)

Expand Down Expand Up @@ -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(
<Combobox value={null} onChange={console.log}>
<Combobox.Input onChange={NOOP} />
<Combobox.Button>Trigger</Combobox.Button>
<Combobox.Options>
<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>
)

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', () => {
Expand Down Expand Up @@ -3440,7 +3457,6 @@ describe('Keyboard interactions', () => {

let options: ReturnType<typeof getComboboxOptions>

await press(Keys.ArrowDown)
await press(Keys.ArrowDown)

// Person B should be active
Expand Down
14 changes: 14 additions & 0 deletions packages/@headlessui-react/src/components/combobox/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
55 changes: 43 additions & 12 deletions packages/@headlessui-vue/src/components/combobox/combobox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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])
})
Expand Down Expand Up @@ -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])
})
)

Expand Down Expand Up @@ -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`
<Combobox v-model="value">
<ComboboxInput />
<ComboboxButton>Trigger</ComboboxButton>
<ComboboxOptions>
<ComboboxOption value="a">Option A</ComboboxOption>
<ComboboxOption value="b">Option B</ComboboxOption>
<ComboboxOption value="c">Option C</ComboboxOption>
</ComboboxOptions>
</Combobox>
`,
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', () => {
Expand Down Expand Up @@ -3631,7 +3663,6 @@ describe('Keyboard interactions', () => {

let options: ReturnType<typeof getComboboxOptions>

await press(Keys.ArrowDown)
await press(Keys.ArrowDown)

// Person B should be active
Expand Down
14 changes: 14 additions & 0 deletions packages/@headlessui-vue/src/components/combobox/combobox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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! }
Expand Down