-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiSelectable] Focus option after search value change #3670
[EuiSelectable] Focus option after search value change #3670
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
Sorry I wasn't super clear in my bug report. The click handles it just fine, but if I'm using the keyboard and use enter to select/deselect the option, the focus is removed from that item. In fact, I just started testing this with VO on and I'm really confused about what is supposed to happen. It doesn't read out the options as I arrow through them. It only sometimes(?) reads selected options and only ever 1 of them. Why the text read out of "completion" and not "selected"? |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
enter works as expected, but now it appears clicking on an option moves focus to the first selected option. |
@cchaos I added some comments about the screen reader experience to the feature branch PR. @chandlerprall |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
Third time's the charm 🤞 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
I'm still getting the issue where using the keyboard to select an item on a filtered list, removes the focus from that item. |
🤔 I'm having trouble reproducing again. I did find a case where it would set index to the wrong option though so I fixed that. @cchaos Can you see if that fixed the issue for you? If not, could you take a screencast of the problem? |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
Thanks for the screencast @cchaos! Got it fixed! |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yep that finally fixed it!
One last thing: pressing any key that isn't ⬆ or ⬇️ shifts focus to the first select option. This is better than before this PR, which shifted focus to an unknown element on keypress. The correct solution should be to match native selection/option boxes and skip to the entry matching typed characters, but that is outside the scope of this PR. If it's easy, can we make keypresses do nothing? Otherwise, these changes look good. |
I'm not really sure if that is the answer. We have our own filtering setup for the results when you type so trying to preempt the results of the filter by jumping to the first result which starts with the letter typed seems like competing ranking systems. I tried looking through the spec for any "official" guidance one way or the other and couldn't find anything concrete. I got to the current implementation of focusing on the first selected item if available and first item in the results if nothing is selected from the multi-select listbox keyboard interactions description. So though this is in a combobox now which could change what best practices are, I think this is a fine thing to ship with. If we ever get to the point of doing real user testing, this would be a good candidate to test out. |
Got the OK from @chandlerprall to merge over slack |
Summary
Fixes bug report from @cchaos:
Also fixes follow-up bug also found by @cchaos of selecting an option after searching would lose focus.
Breaking changes
options
passed into EuiSelectable cannot have anid
(anid
is generated internally)id
)onChange
to be passed into EuiSelectableSearch