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

DatePicker a11y fixes #1486

Merged
merged 13 commits into from
Aug 16, 2023
Merged

DatePicker a11y fixes #1486

merged 13 commits into from
Aug 16, 2023

Conversation

xman343
Copy link
Contributor

@xman343 xman343 commented Aug 14, 2023

Changes

#1148 - All examples are currently failing. Any rule violations that can be resolved just by modifying the examples will be implemented. If it is impossible to resolve a rule violation at the example level, then a change to the component may be necessary.

  • Basic, DateRange, Disabled, Localized, Main, WithCombinedTime, WithTime, and WithYear are violating the aria-input-field-name rule, due to the date table within the calendar not having an accessible name. ("ARIA input fields must have an accessible name.")
    • Passing fix: Implemented useId() and aria-labeledby in component so that the date table is labeled by the currently selected month in the month header.
  • WithCombinedTime and WithTime are violating the color-contrast rule, due to the background color of each selectable TimePicker list item being the same as the background color of the TimePicker. ("Elements must meet minimum color contrast ratio thresholds.")
  • Menu is violating the button-name rule due to the Date Picker Button not having a proper label. ("Buttons must have discernible text.")
    • Passing fix: Implemented aria-label in DatePickerMenuExample for IconButton so that the screen reader will correctly identify the button's role as the date picker button.

Testing

Testing will be conducted through the Cypress web a11y script. Any breaking changes will be evaluated through unit testing & visual tests.

Docs

TBD

@xman343 xman343 added the a11y Accessibility issues (keyboard navigation, color contrast, assistive technologies, semantics, etc) label Aug 14, 2023
@xman343 xman343 self-assigned this Aug 14, 2023
@xman343
Copy link
Contributor Author

xman343 commented Aug 14, 2023

Something I noticed while testing the component with NVDA is that the day-of-the-week columns are read out by the phonetic reading of their abbreviations ("Su, Mon, Tu" are read out as "Soo, Mon, Two"), which could be confusing. On the other hand, this example date picker has NVDA read out the letters of the abbreviation ("Su, Mo, Tu" become "S.U., M.O., T.U."), which at least more accurately reflects what is supposed to be communicated.

I think it would be a good idea to modify the component so that the days of the week are read out like the latter example.

@xman343
Copy link
Contributor Author

xman343 commented Aug 14, 2023

Another note: since the color-contrast rule is being violated by the TimePicker component, I imagine issues pertaining to TimePicker would be best handled in its own PR.

@xman343 xman343 marked this pull request as ready for review August 14, 2023 20:41
@xman343 xman343 requested review from a team as code owners August 14, 2023 20:41
@xman343 xman343 requested review from mayank99 and r100-stack and removed request for a team August 14, 2023 20:41
@xman343
Copy link
Contributor Author

xman343 commented Aug 14, 2023

Changeset coming shortly.

@xman343
Copy link
Contributor Author

xman343 commented Aug 14, 2023

Changeset coming shortly.

Added.

packages/itwinui-react/src/core/DatePicker/DatePicker.tsx Outdated Show resolved Hide resolved
.changeset/pink-shirts-trade.md Outdated Show resolved Hide resolved
examples/DatePicker.menu.tsx Outdated Show resolved Hide resolved
@mayank99
Copy link
Contributor

Something I noticed while testing the component with NVDA is that the day-of-the-week columns are read out by the phonetic reading of their abbreviations ("Su, Mon, Tu" are read out as "Soo, Mon, Two"), which could be confusing. On the other hand, this example date picker has NVDA read out the letters of the abbreviation ("Su, Mo, Tu" become "S.U., M.O., T.U."), which at least more accurately reflects what is supposed to be communicated.

I think it would be a good idea to modify the component so that the days of the week are read out like the latter example.

i just tested the APG example myself and this is what i found:

  • in VoiceOver+Safari (Mac), navigating through the heading cells announces them phonetically, same as iTwinUI DatePicker. but navigating through the grid announces the full name of the day ("Monday").
    • see screen recording
      Screen.Recording.2023-08-15.at.3.02.42.PM.mov
  • in NVDA+Firefox (Windows), sometimes it announces the abbreviations, while other times it announces them phonetically. (not obvious in screen recording)
    • see screen recording
      Screen.Recording.2023-08-15.at.3.09.48.PM.mov

in any case, we cannot control how a screen-reader is going to pronounce something; we can only change the text itself. i think it would be best if it always pronounced the full name of the day. we can achieve this using a combination of aria-hidden (on abbreviated names) and VisuallyHidden (on full names).

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. i'm ok with handling #1486 (comment) in a separate PR

@xman343 xman343 added this pull request to the merge queue Aug 16, 2023
Merged via the queue into dev with commit b3c62e6 Aug 16, 2023
15 of 16 checks passed
@xman343 xman343 deleted the xander-datepicker-a11y-fixes branch August 16, 2023 15:20
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility issues (keyboard navigation, color contrast, assistive technologies, semantics, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants