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 FirstOptionSelectionMode:selected clearing on typing #82

Merged
merged 5 commits into from
Feb 26, 2024
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
14 changes: 8 additions & 6 deletions src/index.ts
Copy link
Member

@iansan5653 iansan5653 Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't comment on unchanged lines (maybe one day 🙏) but I think maybe we should replace the indicateDefaultOption call on line 82 with resetSelection, just to make sure the state is cleared when the menu opens. It's shouldn't be necessary since selection is cleared on close, but it feels safer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7182ef5

Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export default class Combobox {
this.input.addEventListener('input', this.inputHandler)
;(this.input as HTMLElement).addEventListener('keydown', this.keyboardEventHandler)
this.list.addEventListener('click', commitWithElement)
this.indicateDefaultOption()
this.resetSelection()
}

stop(): void {
Expand Down Expand Up @@ -138,13 +138,15 @@ export default class Combobox {

clearSelection(): void {
this.input.removeAttribute('aria-activedescendant')
for (const el of this.list.querySelectorAll('[aria-selected="true"]')) {
for (const el of this.list.querySelectorAll('[aria-selected="true"], [data-combobox-option-default="true"]')) {
el.removeAttribute('aria-selected')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably also remove data- attribute that indicates the 'active' default option here:

Suggested change
el.removeAttribute('aria-selected')
el.removeAttribute('aria-selected')
el.removeAttribute('data-combobox-option-default')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, that's a good catch, will add!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7182ef5 🙇

el.removeAttribute('data-combobox-option-default')
}
}

if (this.firstOptionSelectionMode === 'active') {
this.indicateDefaultOption()
}
resetSelection(): void {
this.clearSelection()
this.indicateDefaultOption()
}
}

Expand Down Expand Up @@ -189,7 +191,7 @@ function keyboardBindings(event: KeyboardEvent, combobox: Combobox) {
break
default:
if (event.ctrlKey) break
combobox.clearSelection()
combobox.resetSelection()
}
}

Expand Down
47 changes: 43 additions & 4 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,15 @@ describe('combobox-nav', function () {
assert.equal(document.querySelector('[data-combobox-option-default]'), options[0])
})

it('first item remains active when typing', () => {
const text = 'R2-D2'
for (let i = 0; i < text.length; i++) {
press(input, text[i])
}

assert.equal(document.querySelector('[data-combobox-option-default]'), options[0])
})

it('applies default option on Enter', () => {
let commits = 0
document.addEventListener('combobox-commit', () => commits++)
Expand All @@ -299,12 +308,18 @@ describe('combobox-nav', function () {
assert.equal(document.querySelectorAll('[data-combobox-option-default]').length, 0)
})

it('resets default indication when selection cleared', () => {
it('resets default indication when selection reset', () => {
combobox.navigate(1)
combobox.clearSelection()
combobox.resetSelection()
assert.equal(document.querySelectorAll('[data-combobox-option-default]').length, 1)
})

it('removes default indication when selection cleared', () => {
combobox.navigate(1)
combobox.clearSelection()
assert.equal(document.querySelectorAll('[data-combobox-option-default]').length, 0)
})

it('does not error when no options are visible', () => {
assert.doesNotThrow(() => {
document.getElementById('list-id').style.display = 'none'
Expand All @@ -325,8 +340,6 @@ describe('combobox-nav', function () {
<li><del>BB-8</del></li>
<li id="hubot" role="option">Hubot</li>
<li id="r2-d2" role="option">R2-D2</li>
<li id="johnny-5" hidden role="option">Johnny 5</li>
<li id="wall-e" role="option" aria-disabled="true">Wall-E</li>
<li><a href="#link" role="option" id="link">Link</a></li>
</ul>
`
Expand All @@ -349,6 +362,32 @@ describe('combobox-nav', function () {
assert.equal(list.children[0].getAttribute('aria-selected'), 'true')
})

it('first item remains selected when typing', () => {
const text = 'R2-D2'
for (let i = 0; i < text.length; i++) {
press(input, text[i])
}

assert.equal(list.children[0].getAttribute('aria-selected'), 'true')
})

it('pressing key down off the last item will have no items selected', () => {
// Get all visible options in the list
const options = document.querySelectorAll('[role=option]:not([aria-hidden=true])')
// Key press down for each item and ensure the next is selected
for (let i = 0; i < options.length; i++) {
if (i > 0) {
assert.equal(options[i - 1].getAttribute('aria-selected'), null)
}

assert.equal(options[i].getAttribute('aria-selected'), 'true')
press(input, 'ArrowDown')
}

const selected = document.querySelectorAll('[aria-selected]')
assert.equal(selected.length, 0)
})

it('indicates first option when restarted', () => {
combobox.stop()
combobox.start()
Expand Down
Loading