search

primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.09k stars 533 forks source link

Should `FormControl.Validation` have `aria-live="polite"` by default? #1892

Open iansan5653 opened 2 years ago

iansan5653 commented 2 years ago

When a user is using a screen reader to input data into a FormControl, they need to be notified of validation error messages. However, error messages within FormControl.Validation do not have aria-live set by default, so the screen reader may not read them until the user leaves the input.

I am not an expert in this area (I don't use a screen reader), so I'm not totally sure about this - just something that should maybe be considered.

inkblotty commented 2 years ago

cc @primer/accessibility 👀

mperrotti commented 2 years ago

Thanks for the feedback @iansan5653

I think it makes sense to use aria-live="polite" to support inputs that validate as the user makes changes. I'd love to hear if somebody from the accessibility team has some feedback.

github-actions[bot] commented 2 years ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

iansan5653 commented 2 years ago

Bump

github-actions[bot] commented 1 year ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

iansan5653 commented 1 year ago

🙃

lesliecdubs commented 1 year ago

Thanks for the patience on this one 🙏🏻

@TylerJDev when you have a moment, could you take a look at this proposal and let us know if you would advise moving forward with it?

TylerJDev commented 1 year ago

@iansan5653 👋 - Thank you for bringing this up! I totally understand where you're coming from in regards to the error being announced. Currently we utilize aria-describedby to link the input to the error warning. In my opinion, I typically see live regions utilized for when the errors are listed in bulk, (e.g. on submit, a group of errors found might be announced via a live region), whereas for inline errors like the ones in FormControl.Validation a live region might not be as needed. This is mainly because the errors should still be announced through that linkage, aria-describedby. The one con to the aria-live is that it might add some extra verbosity, mainly depending on when the error appears.

I think that the current implementation is in a decent state for the errors, though we might want to do some discovery on this topic though during this component's a11y review. I have not looked at this component too deeply, but would love to hear your thoughts and if you had any opinions on this? 🤔

iansan5653 commented 1 year ago

🤔 I guess I can see where it would make sense to not have aria-live if this is used in a traditional-style form where the errors only appear after clicking the submit button. I was picturing an 'as you type' validation event where the accessible description would not automatically be re-read because you are already focused on the input. In that case, aria-live is in fact necessary or the error will likely never be read. But maybe the existence of both cases means we shouldn't enable this by default after all.

TylerJDev commented 1 year ago

@iansan5653 - I understand what you mean, there's definitely some discovery we'll need to do around this during the a11y engineer review for this component. Even if it's not by default, it might be a good thing to allow. I'd love to keep this issue open until the a11y engineer review is finished, if that's okay with you? I think this is something that we can definitely consider adding, even if it's not by default. Thanks a bunch for the input too!

github-actions[bot] commented 11 months ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

github-actions[bot] commented 5 months ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

TylerJDev commented 5 months ago

I believe this PR should resolve this issue: https://github.com/primer/react/pull/4445. Essentially, we could rely on just the aria-describedby to link the errors, as most screen readers that we test with should announce the change, but this isn't exactly the case for VoiceOver in some browsers 🙃.

For now, we'll include the live region to ensure that the change in validation status (i.e. when there's an error) is announced to AT.