-
Notifications
You must be signed in to change notification settings - Fork 839
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
[Feature Branch] EuiSelectable accessibility #3169
Conversation
* swap react-virtualize for react-window and react-virtualized-auto-sizer * improve basic use case a11y * update prop names
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
…essible (#3234) * Moves many aria-* props from the List<ul> and onto the <input /> or surrounding <div> * [Breaking] EuiSelectableSearch has 2 new required props and EuiSelectableList has 1
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
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.
Keyboard shortcuts are awesome! With all the work that has gone into the EuiSelectable component itself, is the hope that we can just re-use this in EuiComboBox instead of re-doing all the work in there? I do see you made some edits to combobox but it doesn't have the full features like the keyboard shortcuts.
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.
To comment on of your todos:
I'd really rather not tie this component to other specific components. I like the flexibility of being able to place the list and the search box where they need to be. We can absolutely ensure that our examples work appropriately with screen-readers, but I don't think it should be part of the component. |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
@cchaos I rephrased that TODO to be vaguer. We can either bring the combobox/filtergroup/etc patterns into Selectable or rework those components to use Selectable under the hood. Either way works, I just wrote down my preference/what I thought would be less work.
Is this something we have today? Because that's not something I see on the demo page and isn't something I've tested. |
Kind of but not quite. It's more that they both live in a popover. Or like the Flyout example has the input in the header and the list in the body. But this is how I setup the search in the K8 POC (need to get it back up and running) but you can see the code here: https://github.com/cchaos/kibana-8-nav/blob/master/src/components/kibana/chrome/search/search.tsx |
Ah... I didn't think through the possibility of passing in not the list in there... I haven't tested that but I think it should "just work"... If it doesn't, it shouldn't be much work to fix, and shouldn't be a breaking change either, so I'd still like to merge this as is as it's already quite large and getting old. Also, a note for anyone doing any testing on this, the support is a little mixed on Mac so I defaulted to the best practices/spec suggestions often. Searchable Selectables work best in VO+Safari, but list Selectables work best in VO+Chrome. This is another one of those components I'll want to test in Windows at some point soon before doing more work on polishing. |
Yep, we can followup with that particular use case which will most likely be on the consuming side (pattern). Can you update the checklist with items you've completed? |
Just wanted to talk about the screen reader experience a little more because listboxes and comboboxes (what Selectable is called in ARIA) are tricky components. Testing this PR in VoiceOver may seem like it yields some mixed results:
The reason for both of those is because it more closely aligns to what's in the aria spec. The spec enables more screen reader magic which is intended to make a complex component more approachable and standardized (though it does come with the potential downside of another layer where bugs may show up and being less approachable to those with less screen reader experience). Because we have limited capacity to do real world testing, I think we should be more closely aligning to the spec. When I have time, I have it on my roadmap to come back to this component to do further Windows testing but I think that's out of scope for this PR. (With more testing, we can polish the experience past "this meets spec expectations" to "this works in user environments.") My full testing notes comparing what's in EUI today and this PREuiSelectable today
EuiSelectable after this PR
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
@cchaos @chandlerprall This is ready for a final review again! |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
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.
Code changes look good. Tested selectable's example in Chrome, FF, Safari, and Android Chrome without issue.
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.
Tested locally and reviewed the code. LGTM!
src/components/selectable/selectable_list/_selectable_list.scss
Outdated
Show resolved
Hide resolved
Preview documentation changes for this PR: https://eui.elastic.co/pr_3169/ |
Closes #1789 and closes #2200
PRs
searchable
use casearia-setsize
andaria-posinset
attributes to optionsaria-describedby
for messagessetActiveOptionIndex
prop toEuiSelectableList
(breaking change 4)aria-label
instructions for exclusionsBreaking changes
virtualizedProps
on EuiSelectableOptionsList renamed towindowProps
which is now passed intoreact-window
instead ofreact-virtualized
rootId
and addedmakeOptionId
andlistId
to EuiSelectableListlistId
to EuiSelectableSearchsetActiveOptionIndex
to EuiSelectableListoptions
passed into EuiSelectable cannot have anid
onChange
to be passed intoEuiSelectableSearch
Future TODOs
Checklist
Added documentation examples