Skip to content

[EuiSelectable] Focus option after search value change#3670

Merged
myasonik merged 6 commits intoelastic:feature/selectable-a11yfrom
myasonik:selectable-a11y/search-value-changes
Jul 6, 2020
Merged

[EuiSelectable] Focus option after search value change#3670
myasonik merged 6 commits intoelastic:feature/selectable-a11yfrom
myasonik:selectable-a11y/search-value-changes

Conversation

@myasonik
Copy link
Copy Markdown
Contributor

@myasonik myasonik commented Jun 29, 2020 •
edited
Loading

Summary

Fixes bug report from @cchaos:

The only thing I felt odd in the functionality was when the list was filtered down from the search box, and an option is selected it loses focus where as it would normally stay focused.

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 an id (an id is generated internally)
    • This feels kind of risky but this wasn't documented as a possibility before so it shouldn't be too bad (out of 23 uses in Kibana, none passed in an id)
  • Requires an onChange to be passed into EuiSelectableSearch
    • Not meant to be used outside of EUI and isn't documented individually though is exported so probably won't have much impact
    • The component doesn't do anything without it, so simplified the types to expected usage

@myasonik myasonik added breaking change PRs with breaking changes. (Don't delete - used for automation) accessibility labels Jun 29, 2020
@myasonik myasonik requested review from cchaos and chandlerprall June 29, 2020 20:20
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 29, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Jun 30, 2020

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"?

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 30, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jun 30, 2020

enter works as expected, but now it appears clicking on an option moves focus to the first selected option.

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jun 30, 2020

@cchaos
The VoiceOver experience is a definitely a mixed bag right now. I want to keep that discussion out of this PR and focus on just these two bugs.

I added some comments about the screen reader experience to the feature branch PR.

@chandlerprall
Sorry about the low hanging bug! Fixed (I hope)!

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 30, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jul 1, 2020

Mouse+focus interaction is still weird. If the selectable doesn't have focus, mousedown moves focus to the first selected item, and only after mouseup/click is the focus moved to the freshly selected option.

mouse focus

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jul 1, 2020

Third time's the charm 🤞

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 1, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Jul 1, 2020

I'm still getting the issue where using the keyboard to select an item on a filtered list, removes the focus from that item.

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jul 1, 2020

🤔 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?

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 1, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Jul 2, 2020

Unfortunately it does not. Steps to reproduce:

  1. Type d in the search
  2. Arrow down to "Dione"
  3. enter to select the option
  4. Notice that the focus is not gone from that option and you have to arrow down to it again.

Screen Recording 2020-07-02 at 03 38 PM

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jul 2, 2020

Thanks for the screencast @cchaos! Got it fixed!

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 2, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3670/

cchaos
cchaos approved these changes Jul 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a 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!

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jul 2, 2020 •
edited
Loading

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.

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jul 6, 2020

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.

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Jul 6, 2020

Got the OK from @chandlerprall to merge over slack

Screenshot of Slack where Chandler Prall says 'Sounds good to me! If it's a punt, feel free to merge'

@myasonik myasonik merged commit 2431824 into elastic:feature/selectable-a11y Jul 6, 2020
@myasonik myasonik deleted the selectable-a11y/search-value-changes branch July 6, 2020 16:13
@myasonik myasonik mentioned this pull request Jul 6, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chandlerprall chandlerprall Awaiting requested review from chandlerprall

1 more reviewer

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accessibility breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@myasonik @kibanamachine @cchaos @chandlerprall