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.

Sign up for GitHub

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

Jump to bottom

Fix FirstOptionSelectionMode:selected clearing on typing #82

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Fix FirstOptionSelectionMode:selected clearing on typing #82

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fc59f27
Added new tests
Feb 23, 2024
dfbf7c5
Spike on separation of concerns
Feb 23, 2024
4365fb9
More changes
Feb 23, 2024
7182ef5
PR feedback
Feb 26, 2024
dbf90b5
Small test
Feb 26, 2024
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
11 changes: 6 additions & 5 deletions src/index.ts
View file
Open in desktop
Copy link
Member

@iansan5653 iansan5653 Feb 23, 2024 •
edited
Loading

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 @@ -108,7 +108,7 @@ export default class Combobox {
const focusIndex = els.indexOf(focusEl)

if ((focusIndex === els.length - 1 && indexDiff === 1) || (focusIndex === 0 && indexDiff === -1)) {
this.clearSelection()
this.resetSelection()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is the case where the user reaches the end of the options in either direction. Based on current behavior I think we do want to clear the selection in this case, not just reset it:

Screen.Recording.2024-02-26.at.9.35.13.AM.mov

Might be good to add a test for this, but it's not a blocker IMO.

Suggested change
this.resetSelection()
this.clearSelection()

Copy link
Author

Choose a reason for hiding this comment

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

If we clear the selection here instead; this will also be a regression in behaviour for the active option which previously set the default attribute, in this case, we wouldn't reset that attribute and that may break the option being selected on enter press? 🤔

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 that's an acceptable change - if the first option is active by default but then the user uses the arrow keys to navigate through all the options, it's fine to deselect the first option at that point. That's actually probably preferrable to the current behavior

Copy link
Author

@anleac anleac Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense! Updated in dbf90b5

this.input.focus()
return
}
Expand Down Expand Up @@ -141,10 +141,11 @@ export default class Combobox {
for (const el of this.list.querySelectorAll('[aria-selected="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 🙇

}
}

if (this.firstOptionSelectionMode === 'active') {
this.indicateDefaultOption()
}
resetSelection(): void {
this.clearSelection()
this.indicateDefaultOption()
}
anleac marked this conversation as resolved.
Show resolved Hide resolved
}

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

Expand Down
28 changes: 26 additions & 2 deletions test/test.js
View file
Open in desktop
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 Down Expand Up @@ -349,6 +364,15 @@ 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('indicates first option when restarted', () => {
combobox.stop()
combobox.start()
Expand Down
Loading