[EuiSelectable] Fix incorrect aria-posinset indices
#6571
Conversation
- I'm doing this because I'll need RTL's `rerender` props setting in the future, which Enzyme's `render` does not support
- this new logic correctly accounts for group labels not at the start of the options array + perf bonus - this array iteration now only happens once per rerender, as opposed to happening on every single item/option render - could likely be further optimized to only rerun when `visibleOptions` or `options` change, but I think this incremental improvement is fine for now
This reverts commit ad05c15.
|
@1Copenut Once staging is up, do you mind giving this a quick screen reader pass to make sure everything's working correctly when you have a quick sec? 🙇 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6571/ |
1 similar comment
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6571/ |
|
I've been making Trevor do a lot of reviews lately, so CCing @Heenawter as well in case she wants to help QA/confirm the fix 😄 |
There was a problem hiding this comment.
Did a quick review of the code and double checked that the the aria-posinset property of each item in the https://eui.elastic.co/pr_6571/#/forms/selectable#the-basics example was set correctly - everything seems correct to me! Thanks so much for tackling this so quickly 🎉
There was a problem hiding this comment.
👍 This passes an axe check on Chrome latest (MacOS). I tested VoiceOver using Safari and Firefox. Safari is handling the counts correctly as outlined in the MDN aria-setsize documentation.
VO + Firefox does some unexpected (to me) counting, but that behavior was observed on production too, so not a regression. It's entirely possible VO + Firefox users are used to this behavior and consider it correct; I don't have enough information to flag it as a separate improvement item.
|
Huzzah!! Thanks for the thorough testing y'all! 🏆 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6571/ |
## Summary `[email protected]` ⏩ `[email protected]` ___ ## [`74.0.2`](https://github.com/elastic/eui/tree/v74.0.2) **Bug fixes** - Fixed `EuiCard` to ensure `onClick` method only runs once when `title` contains a React node ([#6551](elastic/eui#6551)) - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([#6577](elastic/eui#6577)) Co-authored-by: Kibana Machine <[email protected]>
## Summary `[email protected]` ⏩ `[email protected]` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <[email protected]>
## Summary `[email protected]` ⏩ `[email protected]` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([elastic#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([elastic#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([elastic#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([elastic#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([elastic#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([elastic#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <[email protected]>
Summary
closes #6565
This bug was occurring whenever group labels existed in the
optionsarray that were not located at the start of the array.QA
<li>items in the first example.aria-posinsetindices before and after the group label(s)General checklist