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

Improve screen reader accessibility #2

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

VladyslavBondarenko
Copy link

@VladyslavBondarenko VladyslavBondarenko commented Nov 7, 2022

Ramp task #100143. This allows following WAI-ARIA best practices for combobox elements.
Changes in this PR mostly correspond to the PR to the original react-intl-tel-input.

Although most of the best practices to improve accessibility were implemented, the role="combobox" was not added neither the to the number input (because the input doesn't show/hide the dropdown), nor to the country code button.
Looks like OSx VoiceOver announces everything correctly.

Changes:

  • Add role="listbox" to the countries list
  • Refer the countries list by its Id in the country code button via aria-controls and aria-owns
  • Add role="option" to each country list option
  • Use aria-expanded on the country code button element
  • Refer focused option by its Id in the country code button element via aria-activedescendant to read focused option while navigating by keyboard
  • Add aria-selected="true" to the selected list option
  • Set aria-autocomplete="none" to the country code button element since displayed options do not depend on the number input value
  • Allow passing arbitrary props to the country code button element via flagDropdownProps prop (for example, "aria-labelledby" property can be passed)

Also, this PR makes country names shown in English only.
This solves the problem when texts in other languages do not have a corresponding "lang" attribute. #100025
lang="en" for each country name is added to handle the case
when the package is used on a non-English page.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have used ESLint & Prettier to follow the code style of this project.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@VladyslavBondarenko
Copy link
Author

VladyslavBondarenko commented Nov 7, 2022

To test, I changed https://github.com/salemove/visitor-app/blob/master/package.json#L28 to

"react-intl-tel-input": "github:salemove/react-intl-tel-input#11ade03a43a2c6ce3919385638b446643a9ec6f9",

The idea could be to use the branch name #v5.0.7 in the visitor-app package.json, but due to the node caching the dependency won't be updated on yarn install by itself.
It's safer to use a commit hash, but it needs to be updated in the visitor-app on each new change in this react-intl-tel-input fork.

Merging to v5.0.7 branch to use it instead of master, because master is protected and doesn't allow force-push to downgrade its version from 7.0.1 to 5.0.7.

@VladyslavBondarenko VladyslavBondarenko requested review from AnnaDunajeva and gulagha and removed request for gulagha and AnnaDunajeva November 7, 2022 15:07
@VladyslavBondarenko
Copy link
Author

VladyslavBondarenko commented Nov 7, 2022

Or maybe it's not ready yet

Without any changes here, the phone input in the visitor-app is again failing with

Error: Module build failed: ModuleBuildError: Module build failed: Error: Node Sass does not yet support your current environment: OS X 64-bit with Unsupported runtime (72)
For more information on which environments are supported please see:
https://github.com/sass/node-sass/releases/tag/v4.5.0

(to see the error I add console.error(error) here)

Changing node-sass version in this repository to 7.3.0 doesn't help.
Having dist files built with the same node 14 as dev-space has doesn't help.
Cleaning yarn cache in dev-space doesn't help.
Changing node-sass version in the visitor-app to 7.3.0 leads to Error: Node Sass version 7.0.3 is incompatible with ^4.0.0.. So tried also to update sass-loader to 13.1.0. It asks for node 14. Using node 14 the build fails with

ERROR in ./styles/default.scss
Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
TypeError: this.getOptions is not a function

and I don't think such updates are ever needed.

UPDATE: using dist built with node 8.17.0 is working!
(although, the package in visitor-app dev-space somewhy wasn't updated on changing the react-intl-tel-input dependency in the package.json, but force yarn install with kubectl --context=dev-power -n dev-vladyslav-bondarenko exec deploy/visitor-app -- yarn install --force helped)

@VladyslavBondarenko VladyslavBondarenko force-pushed the v5.0.7 branch 2 times, most recently from 94cc807 to cb3180a Compare November 8, 2022 08:11
Ramp task: https://ramp.accessibleweb.com/app/websites/83ca83c6265c/tasks/2787246f7ae2
This allows following WAI-ARIA best practices  for combobox elements -
https://www.w3.org/WAI/ARIA/apg/patterns/combobox/.
Changes in this PR mostly correspond to the patw0929#405

Although most of the best practices to improve accessibility were
implemented, the role="combobox" was not added neither the to
the number input (because the input doesn't show/hide the dropdown),
nor to the country code button.
Looks like OSx VoiceOver announces everything correctly.

Changes:
- Add role="listbox" to the countries list
- Refer the countries list by its Id in the country code button
  via aria-controls and aria-owns
- Add role="option" to each country list option
- Use aria-expanded on the country code button element
- Refer focused option by its Id in the country code button element
  via aria-activedescendant to read focused option while navigating
  by keyboard
- Add aria-selected="true" to the selected list option
- Set aria-autocomplete="none" to the country code button element
  since displayed options do not depend on the number input value
- Allow passing arbitrary props to the country code button element via
  flagDropdownProps prop (for example, "aria-labelledby" property
  can be passed)

Updates dist files (built with node 8.17.0)
@VladyslavBondarenko
Copy link
Author

The dist files in the repository are needed because they are used in visitor-app, but this package isn't built on visitor-app yarn install.

I tried to make it building on install, but no luck

If I have node scripts/build.js as postinstall script, it fails with
Error: Cannot find module 'copy-webpack-plugin'.
If I have yarn install && node scripts/build.js, it fails with many other errors.

I'd be happy if you have a suggestion on how to avoid committing build files with every change.

@AnnaDunajeva
Copy link

AnnaDunajeva commented Nov 9, 2022

I am also not sure what is a better way to do it. But considering that we want to get rid of this library anyway, I think what you already have is not too problematic. There probably will not be much more changes to this lib anyway and hopefully we migrate to new phone component soon.
I would assign Siim as reviewer as well to take a look at this.

This will solve the problem when texts in other languages do not
have corresponding "lang" attribute.
lang="en" for each country name is added to handle the case
when the package is used in non-english page.
Task: https://ramp.accessibleweb.com/app/websites/83ca83c6265c/tasks/7e2074f1f904
@VladyslavBondarenko VladyslavBondarenko merged commit 37e921d into v5.0.7 Nov 10, 2022
VladyslavBondarenko added a commit that referenced this pull request Nov 11, 2022
This is a fix for #2
The country code button should still have role="combobox".
This role communicates that the component will open a listbox.
This is the case of a Select-Only Combobox
https://www.w3.org/WAI/ARIA/apg/example-index/combobox/combobox-select-only.html
VladyslavBondarenko added a commit that referenced this pull request Nov 11, 2022
This is a fix for #2
The country code button should still have role="combobox".
This role communicates that the component will open a listbox.
This is the case of a Select-Only Combobox
https://www.w3.org/WAI/ARIA/apg/example-index/combobox/combobox-select-only.html

Fix broken tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants