fix(Dropdown): accessibility problem (screen readers) #1836
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1836 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 144 144
Lines 2463 2463
=======================================
Hits 2457 2457
Misses 6 6
Continue to review full report at Codecov.
|
layershifter
changed the title
src/modules/Dropdown/Dropdown.js
Outdated
| } else { | ||
| finalRenderedText = <div className={classes}>{_text}</div> | ||
| } | ||
| return finalRenderedText |
There was a problem hiding this comment.
I think that we can make it simplier:
if (classes === 'text') return <div className={classes} role='alert' aria-live='polite'>{_text}</div>
return <div className={classes}>{_text}</divThere was a problem hiding this comment.
Ups...I had a mistake...I'll patch it
src/modules/Dropdown/Dropdown.js
Outdated
|
|
||
| return <div className={classes}>{_text}</div> | ||
| let finalRenderedText = null | ||
| if (classes === 'text') { |
There was a problem hiding this comment.
Can you clarify why we don't add aria attributes in other cases?
There was a problem hiding this comment.
I don't add them because I don't know when are used the other cases. If you can explain me them, I can notice if they are needed or not, and also, which ones. If I know in which examples from the documentation they are rendered...I can try also to improve them.
There was a problem hiding this comment.
I updated the code. Now returns always the rendered text with the aria-labels suggested. I tested the documentation page, and all dropdown added work as in the main documentation page (http://react.semantic-ui.com)
- patched an error. Forgotten '}'
src/modules/Dropdown/Dropdown.js
Outdated
|
|
||
| if (classes === 'text') { | ||
| return <div className={classes} role='alert' aria-live='polite'>{_text}</div> | ||
| } |
There was a problem hiding this comment.
I believe classes === 'text' is the same as the final return case here. When none of the above conditions match, the text is rendered as:
<div class="text">File</div>
Is there a reason not to add role='alert' aria-live='polite' to the final return value instead?
There was a problem hiding this comment.
@levithomason we only need to mark as 'alert' a DIV which text changes and the user should know about the update. In the dropdowns case, the DIV which displays the value selected by the user uses the class 'Text'.
To the best of my knowledge, renderText returns a div which can be marked with classes: text , default and filtered. Looking at the examples of the documentation, I noticed than 'text' is always used to the DIV which contains the value selected, but I don't know when the other two options are used. Can you give me examples of them to decide if we need to add the alert tag in the three cases?
There was a problem hiding this comment.
I updated the code. Now returns always the rendered text with the aria-labels suggested. I tested the documentation page, and all dropdown added work as in the main documentation page (http://react.semantic-ui.com)
- Dropdown.js->renderTex--> now always returns the text rendered with aria labels: role='alert' aria-live='polite'
|
Let's merge this as is for now and update should we hit any future issues. Thanks for the work and thoughtfulness here! |
|
Released in |
Fixes #1834
Screen readers read aloud the different options.