Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve accessibility of create/edit group pages #8888

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Aug 20, 2024

No description provided.

@seanh
Copy link
Contributor Author

seanh commented Aug 20, 2024

This draft PR just adds a couple of ARIA attrs. I think there's more that can be done but I haven't read up on ARIA yet. Some suggestions from GitHub Copilot (I haven't learned the ARIA/accessibility stuff here so can't yet make a judgement about the quality of these suggestions):

  • Use aria-label or aria-labelledby for form controls
  • Use aria-required for required fields (may be something to implement in the pattern library rather than here)
  • Put aria-busy on the form while in loading state
  • Add role="img" and aria-label to the <CancelIcon /> and <CheckIcon /> components (again may be something we want to implement in the pattern library rather than here)
  • When showing an error message use tabindex to move the keyboard focus to the error message to ensure that it gets read out by screen readers

@seanh seanh marked this pull request as ready for review August 21, 2024 09:55
@robertknight robertknight self-assigned this Aug 21, 2024
@robertknight
Copy link
Member

robertknight commented Aug 21, 2024

Use aria-label or aria-labelledby for form controls
Use aria-required for required fields (may be something to implement in the pattern library rather than here)

This should be handled already, but I'll verify.

Put aria-busy on the form while in loading state

Yes, this will be needed.

Edit: Maybe not. Although it sounds relevant, aria-busy is actually a way of suppressing screen reader notifications. See https://www.tpgi.com/short-note-on-being-busy/.

Add role="img" and aria-label to the and components (again may be something we want to implement in the pattern library rather than here)

A label ("Changes saved") or similar will be need here.

When showing an error message use tabindex to move the keyboard focus to the error message to ensure that it gets read out by screen readers

The way we settled on handling this is to make the error message a description for the field using aria-describedby. The description is then read out when the field itself is focused. The field itself does need to be focused after any async validation error happens for this to be read out.

@robertknight
Copy link
Member

Put aria-busy on the form while in loading state
Add role="img" and aria-label to the and components (again may be something we want to implement in the pattern library rather than here)

This was handled in #8892, which created a component that will likely be moved to the component library in future.

@robertknight
Copy link
Member

I'm going to leave this PR open to track remaining work. Some notes from testing different browsers on macOS this morning:

  • A universal issue in all browsers is that the reason why the input is invalid is not communicated to screen readers until the form is submitted
  • The character counter needs an accessible description
  • In Safari clicking the "Save changes" button does not announce "Changes saved"
  • In Firefox clicking the "Save changes" button reads "Changes saved" twice
  • In Chrome the "Save changes" button announces the changes are being saved correctly

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants