Address accessibility issues in management apps #45024
Conversation
alisonelizabeth
added
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
|
retest |
cjcenizal
added
Feature:ILM
Feature:Index Management
There was a problem hiding this comment.
Thanks for doing this @alisonelizabeth! I found a couple things that I think we need to fix. @barlowm Would you please review this and make sure the issues you found have been resolved?
| scope="row" | ||
| data-test-subj={`policyTableCell-${fieldName}`} | ||
| > | ||
| <div className={`euiTableCellContent policyTable__content----${fieldName}`}> |
There was a problem hiding this comment.
I think policyTable__content---- contains a typo and should be policyTable__content--.
There was a problem hiding this comment.
good catch, fixed!
There was a problem hiding this comment.
@cjcenizal I really can't give a call on if the issues have been resolved at this point. If you want to have a conference call where I can look at a screen share of the app running I'd be able to give a better call on that. Hit me up on slack if you'd like to do that.
|
|
||
| if (fieldName === 'name') { | ||
| return ( | ||
| <th |
There was a problem hiding this comment.
Ideally, EUI would publish a component we can use here, or provide props on EuiTableRowCell that allow it to provide the accessibility affordances you're adding here. Would you mind creating an issue and cross-linking to this code as an example?
There was a problem hiding this comment.
Yeah, agreed. I was looking through the existing EUI issues before I went ahead and opened one and it looks like you created a similar/related issue in the past 😀
See: elastic/eui#1030
There was a problem hiding this comment.
Hmm, I'm not sure if that issue covers the requirements you're meeting with this particular change. If I understand correctly, that issue is about announcing each column header before reading the content of a particular cell. This helps users understand the meaning of the cell's content. For example, if a cell contained an address "123 ABC Street", and that column header was "Address", the screen-reader would announce "Address: 123 ABC Street".
But I believe the problem raised in #43842 was that the "Name" column (or some other identifying column) is also used to identify each row. This provides additional context so that you know which entity each cell is referring to. In my example above, if there was also a "Name" column and the content for that row was "Anastasia Romanov" then it sounds like somehow the screen-reader would connect "Anastasia Romanov" and "123 ABC Street". But to be honest, I'm not sure I understand how that works.
@barlowm Could you please clarify this for us?
There was a problem hiding this comment.
@cjcenizal - You're correct. Screen readers (such as JAWS and NVDA) provide the ability to navigate to individual colums randomly. And without a "scope=row" (which as of HTML5 can only be applied to a TH tag) the screenreader user won't know what row a given cell is associated with. See WebAIM's article on Creating Accessible Tables.
There was a problem hiding this comment.
@cjcenizal @barlowm thanks for the explanation! Opened elastic/eui#2335
| role="status" | ||
| aria-relevant="text" | ||
| aria-live="polite" | ||
| className="euiScreenReaderOnly" |
There was a problem hiding this comment.
Is it possible to use the EuiScreenReaderOnly component here instead of using the CSS class? The comment applies in other places this classname is used.
There was a problem hiding this comment.
yep, good call. fixed.
...k/legacy/plugins/index_management/public/sections/home/index_list/index_table/index_table.js
Show resolved
Hide resolved
| aria-label={i18n.translate('xpack.idxMgmt.indexTable.selectIndexAriaLabel', { | ||
| defaultMessage: 'Select this row' | ||
| })} | ||
| title={i18n.translate('xpack.idxMgmt.indexTable.selectIndexTitle', { |
There was a problem hiding this comment.
No, I don't think so. I removed it (can't remember why I originally added it).
| @@ -52,6 +53,9 @@ export const CronDaily = ({ | |||
| <EuiSelect | |||
| options={hourOptions} | |||
| value={hour} | |||
| label={i18n.translate('esUi.cronEditor.cronDaily.hourSelectLabel', { | |||
There was a problem hiding this comment.
I think this needs to be aria-label for it to be read aloud by a screen-reader. I used VoiceOver to test this, and it won't announce these values unless I change them to aria-label.
There was a problem hiding this comment.
Good catch! I changed this to aria-label and tested with voiceover.
💚 Build Succeeded |
|
@cjcenizal I believe I addressed all of your comments. Mind taking another look when you get a chance? Thanks! |
💔 Build Failed |
💔 Build Failed |
|
Retest |
|
@cjcenizal @barlowm per our discussion, I have opened the following issues in EUI: |
|
@elasticmachine merge upstream |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
This PR addresses accessibility issues across multiple management apps.
Fixes #43842
<caption>to indices table<caption>to index lifecycle policies tableOther things to note:
<td>in a table row to a<th>withscope=“row”. AFAIK this is not possible with the<EuiInMemoryTable/>. I addressed this for the indices and index lifecycle policies table (different setup than other apps), but I’m also wondering if this is necessary? I don’t have enough knowledge in this area, so would appreciate any thoughts. I can open up an issue against EUI if we feel it’s something we should do.