Fix FirstOptionSelectionMode:selected clearing on typing#82
Conversation
iansan5653
left a comment
iansan5653
left a comment
There was a problem hiding this comment.
Looks great - just two suggestions to cover potential edge cases
| @@ -141,10 +141,11 @@ export default class Combobox { | |||
| for (const el of this.list.querySelectorAll('[aria-selected="true"]')) { | |||
| el.removeAttribute('aria-selected') | |||
There was a problem hiding this comment.
I think we should probably also remove data- attribute that indicates the 'active' default option here:
| el.removeAttribute('aria-selected') | |
| el.removeAttribute('aria-selected') | |
| el.removeAttribute('data-combobox-option-default') |
There was a problem hiding this comment.
Totally, that's a good catch, will add!
There was a problem hiding this comment.
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.
|
|
||
| if ((focusIndex === els.length - 1 && indexDiff === 1) || (focusIndex === 0 && indexDiff === -1)) { | ||
| this.clearSelection() | ||
| this.resetSelection() |
There was a problem hiding this comment.
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.
| this.resetSelection() | |
| this.clearSelection() |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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
2 participants
When the
FirstOptionSelectionModeprop is set toselected, this would correctly set the first element to selected on open, but on typing, remove this state.This was easily reproducible with a test-case that I should have had in my prior PR here: #81
To encourage a simple code path that's shared with the
activestate forFirstOptionSelectionMode, I refactored the code a bit and also cleaned up some call paths that were not necessary on closing flows.This PR adds:
resetSelectionto be distinct fromclearSelection. Why?clearSelectionalso handled the "state reset" for active/selected state, but this broken in the case for "active" as it would try to navigate when the list was not visible, asclearSelectionwas called on.stop,.destroyetc. This flow didn't make sense, but didn't throw errors for theactivestate as there was no navigations on option lists but rather attribute settings.resetSelectionwhen appropriate, which is, on non-special character typing, and when we navigate through the last element (this should cycle back). (First open is applicable, but we callindicateDefaultOptiondirectly.