-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix #6223 - ♿ Accessibility improvements: Month and Year dropdowns #6232
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,12 @@ export default class MonthDropdown extends Component< | |
| renderSelectOptions = (monthNames: string[]): React.ReactElement[] => | ||
| monthNames.map<React.ReactElement>( | ||
| (m: string, i: number): React.ReactElement => ( | ||
| <option key={m} value={i}> | ||
| <option | ||
| key={m} | ||
| value={i} | ||
| aria-label={`Select Month ${m}`} | ||
| aria-selected={i === this.props.month ? "true" : "false"} | ||
| > | ||
| {m} | ||
| </option> | ||
| ), | ||
|
|
@@ -47,6 +52,7 @@ export default class MonthDropdown extends Component< | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| > | ||
| {this.renderSelectOptions(monthNames)} | ||
| </select> | ||
|
|
@@ -62,8 +68,14 @@ export default class MonthDropdown extends Component< | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would make the element always announce "Select month" even when a month has been selected. |
||
| aria-expanded={this.state.dropdownVisible} | ||
| aria-haspopup="listbox" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| > | ||
| <span className="react-datepicker__month-read-view--down-arrow" /> | ||
| <span | ||
| className="react-datepicker__month-read-view--down-arrow" | ||
| aria-hidden="true" | ||
| /> | ||
| <span className="react-datepicker__month-read-view--selected-month"> | ||
| {monthNames[this.props.month]} | ||
| </span> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,7 @@ export default class MonthDropdownOptions extends Component<MonthDropdownOptions | |
| } | ||
| }} | ||
| role="button" | ||
| aria-label={`Select Month ${month}`} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| tabIndex={0} | ||
| className={ | ||
| this.isSelectedMonth(i) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.