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

fix(PPDSC-2216): add aria-required to form-input-components #257

Merged

Conversation

evgenitsn
Copy link
Contributor

PPDSC-2216

What

Added aria-required for FormInputComponents - Select / Radio / Checkbox when they are set as required.
The story mentions the aria-invalid as well but this one is used only for input fields, so no changes there.

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@evgenitsn evgenitsn added fix This change fixes a bug ready for review Please assist in getting this reviewed labels Jul 4, 2022
@evgenitsn evgenitsn added WIP Work in progress and removed ready for review Please assist in getting this reviewed labels Jul 4, 2022
@evgenitsn
Copy link
Contributor Author

@Vanals Hey, can you take a look at this PR in terms of a11y? The task was to add aria-required to all form-input components but we had some errors with adding aria-required to the Select component because the Select is role="combobox and aria-required is not allowed for combobox role and the a11y testing was failing with aria-allowed-attr.

I talked with Stoyan and he pointed me to this aria-required article and we agreed on instead of using aria-required for all form-input components (InputText, Select, Radio, Checkbox), to use required attribute instead. It looks like the behaviour is the same, we get the required UI for the components, the tests are ok and the screen reader reads these components as "Required".

Just want to confirm that this is ok with you. 🙏

@evgenitsn evgenitsn added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Jul 5, 2022
mstuartf
mstuartf previously approved these changes Jul 5, 2022
src/form/form-input.tsx Outdated Show resolved Hide resolved
@Vanals
Copy link
Contributor

Vanals commented Jul 5, 2022

Thank you @evgenitsn! Everything you listed: tests, Screen reader, etc ; sounds to be happy. So I guess its ok to use the required attribute.

Maybe a bit confused on why wasn't working on the combobox?
In theory, by docs, it is possible: https://www.w3.org/TR/wai-aria/#aria-required

If you want you could also test it on NVDA, using assistive labs. We had bugs coming from DOW requiring us to work with assistive labs. Could also be an occasion to get familiar with it! You can find the credentials in the dev channel!

Here you can find how to use NVDA: https://nidigitalsolutions.jira.com/wiki/spaces/NPP/pages/3579936811/A11Y+guidance+for+Developers

Happy to help

@evgenitsn
Copy link
Contributor Author

@Vanals
The thing is that the voiceover sounds ok even with aria-required it just a not allowed combination between role and aria attributed based on the rules that axe dev tools plugin is using. Not sure why that's different from the w3 link that you sent. Maybe because our select implementation is using a button instead of input https://w3c.github.io/aria/#combobox ? idk, just guessing.

Here is the axe dev tools output when I am using aria-required on a form input select.

Happy to show that to you on a call if you think it's worth it.

image

@evgenitsn evgenitsn merged commit a3c95cd into main Jul 11, 2022
@evgenitsn evgenitsn deleted the fix/PPDSC-2216-form-input-components-dont-have-aria-required branch July 11, 2022 07:11
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
* fix(PPDSC-2216): add aria-required to form-input-components

* fix(PPDSC-2216): update required attribute

* fix(PPDSC-2216): rephrase condition checks

* fix(PPDSC-2216): update required props

* fix(PPDSC-2216): update snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix This change fixes a bug ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants