Fix #6223 - ♿ Accessibility improvements: Month and Year dropdowns#6232
Fix #6223 - ♿ Accessibility improvements: Month and Year dropdowns#6232balajis-qb wants to merge 2 commits intoHacker0x01:mainfrom
Conversation
balajis-qb
force-pushed
the
issue-6223/fix/month-dropdown-accessibility
branch
from
5cbdd4d to
7f61d82
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6232 +/- ##
=======================================
Coverage 99.29% 99.29%
=======================================
Files 30 30
Lines 3822 3824 +2
Branches 1648 1667 +19
=======================================
+ Hits 3795 3797 +2
Misses 26 26
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
halvorsanden
left a comment
halvorsanden
left a comment
There was a problem hiding this comment.
I’m glad to see this being worked on. I think this would benefit from taking a step back and simplifying and considering the elements, roles and context too, so it doesn’t end up with more aria than necessary.
My comments are mainly from experience with making comboboxes, which aren’t that different from this. I find https://www.w3.org/WAI/ARIA/apg/patterns/ very useful, and I also recommend inspecting the solution in the accessibility tree and, if you’re not doing it already, test it with your OS screen reader.
I commented on the month section, but the same goes for the year.
| key={m} | ||
| value={i} | ||
| aria-label={`Select Month ${m}`} | ||
| aria-selected={i === this.props.month ? "true" : "false"} |
There was a problem hiding this comment.
aria-label is not necessary here, the value is already inside this element and the context this appears in already communicates that this is an option that can be selected.
The same with aria-selected I believe, the option element is inside a select element, both have built-in roles and meaning stating what is selected.
Adding aria to elements like these that already have the semantics and meaning built-in can make things less accessible because it is announced differently than the user might expect when encountering the native HTML elements.
| value={this.props.month} | ||
| className="react-datepicker__month-select" | ||
| onChange={(e) => this.onChange(parseInt(e.target.value))} | ||
| aria-label="Select Month" |
There was a problem hiding this comment.
This is what the referred issue points to, and also what I’m after, but this should be a prop so it can use i18n. If a default is needed, I would recommend only "Month" since "select" is already given by the element type. The entire date dialog is also announced as "choose date", so there is no need to repeat "select/choose" too many places.
| style={{ visibility: visible ? "visible" : "hidden" }} | ||
| className="react-datepicker__month-read-view" | ||
| onClick={this.toggleDropdown} | ||
| aria-label="Select Month" |
There was a problem hiding this comment.
This would make the element always announce "Select month" even when a month has been selected.
| onClick={this.toggleDropdown} | ||
| aria-label="Select Month" | ||
| aria-expanded={this.state.dropdownVisible} | ||
| aria-haspopup="listbox" |
There was a problem hiding this comment.
This also needs a reference to what element it opens and what element has keyboard focus when the element is open. An aria-controls that takes the id of the related element and an aria-activedescendant with the focused element.
| } | ||
| }} | ||
| role="button" | ||
| aria-label={`Select Month ${month}`} |
There was a problem hiding this comment.
The role of this element should most likely be changed to "option" rather than adding an aria-label, if the parent element is a listbox. I would also change the parent to a ul element and this from a div to a li.
2 participants
Description
Linked issue: #6223
Problem
As mentioned in the attached ticket, the month select dropdown (
showMonthDropdown) and the year select drop down (showYearDropdown) lacks ARIA attributes.Changes
dropdownMode) of scroll and selectContribution checklist