Skip to content
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

EuiTableHeaderCell should read aloud text content instead of aria-label #1378

Closed
cjcenizal opened this issue Dec 14, 2018 · 5 comments
Closed

Comments

@cjcenizal
Copy link
Contributor

cjcenizal commented Dec 14, 2018

The current implementation uses aria-label to override the default screen reader behavior of reading aloud the text contents: https://github.com/elastic/eui/blob/master/src/components/table/table_header_cell.js#L61

This presents two problems:

  • It means that providing a node as children will cause the screen reader to read something like "Sort [Object object] ascending".
  • As the user traverses the cells in the table body, the screen reader reads aloud the associated column header to help the user orient themselves. This means the user hears "Sort {column title} ascending/descending" for each cell they navigate to, which is confusing to hear.

I think one solution would be to use the sortIcon to provide screen-reader-specific sort information. Instead of conditionally rendering it, we could always render it but wrap it in EuiScreenReaderOnly when the column isn't being sorted on. We could then put aria-label={Click to sort ascending/descending} on the sort icon, which will be read aloud after the column text content is read.

@thompsongl
Copy link
Contributor

thompsongl commented Jan 10, 2019

Removing aria-label from column headers appears to be the right direction. A few things to consider after doing some research:

  1. aria-label is currently derived from a prop (ariaLabel) that would likely no longer have a use should we rely on the cell text. Removing this prop and aria attr could be considered a breaking change.
    Regardless, aria-label is likely not the correct attribute for table cells ("If the label text is visible on screen, authors SHOULD use aria-labelledby and SHOULD NOT use aria-label." -W3C).
  2. aria-sort may provide sufficient information about table sorting state to avoid adding aria attributes the button or icon.
  3. We likely want to add role="grid" to the parent table when columns are sortable. It's implied that the cells are non-interactive with the default role.

@thompsongl
Copy link
Contributor

Taking another look, ariaLabel is of course just a pass-through HTML attribute. That being said, we'd still be removing it from the EuiTableHeaderCell API

@chandlerprall
Copy link
Contributor

I think taking a look at aria-sort and role="grid" is the right direction. Getting more alignment with the WAI-ARIA guidelines would be a good thing.

@thompsongl
Copy link
Contributor

aria-sort does not appear to be supported by VoiceOver. It's possible we'll want to do a combination of aria-sort and some kind of label on the icon decorator to be comprehensive

@thompsongl
Copy link
Contributor

Resolved by #1426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants