[EuiDataGrid] Fix/improve screen reader accessibility for column headers #6034
Conversation
- Switch to hint text that an action (clicking, aka enter/space key) is necessary - move to `aria-describedby` over `aria-label` for 2 reasons: 1. to give more pause before announcing the new hint 2. in combo with the `hidden` attribute, this prevents the extra hint text from being announced as part of the column header name when screen reader navigating between cells - requires adding fragment wrapper (turn off whitespace diffs)
- for easier testing and dev readability purposes + perf improvement - DRY out unnecessary new Set() in favor of sortedColumn & a useMemo() around .find() - update unit tests to use hook helper instead of having to wrap context provider
- for better pause/screen reader behavior - for header cells without actions, this can't be in an aria-describedby because VO reads the describedby BEFORE the header, not after :| moving it to a SR only item after the column text appears to work
- now that we no longer need to set an aria-describedby on the topmost grid parent, this can be just ariaSort instead of ariaProps - add comment documentation for somewhat unintuitive aria-sort behavior - simplify to a nested ternary (fight me irl) - add easier to read boolean flags (which will be used more later)
Minor - Make var name a little more explicit - useMemo for perf Major - Fix header cells w/ actions not reading out `aria-sort` (because focus is on the child button, not on the grid parent) by adding a new branching path - Fix sort text not being i18n'ed correctly (requires switching to `EuiI18n`, since hooks can't be called in map callbacks or conditionally :T) - Fix copy to read out 'ascending' and 'descending' fully as words, + add ending period for better screen reader flow
- Remove `data_grid.test.tsx` mounted unit tests in favor of more fleshed out `data_grid_header_cell.test.tsx` tests
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6034/ |
- textContent now includes hidden describedby text, using a querySelector to specify the text content we want fixes it - typescript was complaining about `data`, not sure why
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6034/ |
There was a problem hiding this comment.
Looks (and sounds) great to me!
The aria-describedby mechanism adds a lot of rich information when I'm sorting by one or multiple columns.
In this case FF does a better job of retaining focus on the button after the button actions popover is closed, but both browsers announce the updated description content perfectly.
| if (direction === 'asc') { | ||
| return ( | ||
| <EuiI18n | ||
| token="euiDataGridHeaderCell.sortedByAscendingSingle" |
There was a problem hiding this comment.
I wonder if these shouldn't be a single (or more probably two, a singular sort and a multi sort) i18n tokens which map to a function. It's generally better to give more flexibility to the localization function as its hard to predict the needs & structure of other languages. In this case, the assumption that subsequent columns would be simple concatenations to the end of the string could likely make some translations awkward if not incorrect.
There was a problem hiding this comment.
I disagree based on what I understand of Kibana's translators/localizers. They're not receiving functions or any complex logic, they get basic/static strings to translate. Additionally, Kibana doesn't actually use functions in it's i18n lib like EUI does, as far as I've used i18n in Kibana it's been string interpolation only (see docs).
I think this approach (which I agree will not be perfect for all languages) is nevertheless the easiest approach for Kibana's translators based on my understanding of the languages Kibana currently supports (Chinese, Japanese, and French), and we should wait to see if there are any actual issues in translated output before revisiting them.
There was a problem hiding this comment.
After thinking more, I think we're both conjecturing a lot here, so I'm going to ping the Kibana i18n team on Slack to get their recommendation!
There was a problem hiding this comment.
@chandlerprall Just heard back from @Bamieh! Copying and pasting their Slack comment:
Just had a look at the PR, looks fine 👍 i’ll throw a note to the translators to make sure they understand the requirements here
With that approval from the i18n team, are we good to go on this PR?
Summary
closes #5333
Fixes:
Improvements:
Click to view header actions) and additionally also comes after a pause instead of being mashed together with the column title{Title} Header Actions. Thanks to switching toaria-describedby, SRs now only announce the column title.describedbydoes not work for those cells and they're using SR only text, the extra text comes along for the rideScreencap
Note: GitHub auto-mutes videos, you'll have to unmute to hear VO.
Screen.Recording.2022-07-07.at.12.13.36.AM.mp4
Checklist
Chrome, Safari,Edge, and Firefoxaria-sortwhile Safari does- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Updated the Figma library counterpart