-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Inline Errors Improvements #16850
Inline Errors Improvements #16850
Conversation
0d2db0e
to
9c63ef1
Compare
Took this to accessibility office hours today. As mentioned, this has accessibility anti patterns due to the submit buttons being disabled by default. Thus, with the acknowledgment that a fully accessible approach would include the submit buttons being fixed, this is a step in the right direction. Side questions and answers:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and it works as expected!! ✨
// We need to toggle from two non-breaking spaces to one non-breaking space | ||
// because VoiceOver does not detect the empty string as a change. | ||
this.suffix = this.suffix === '\u00A0\u00A0' ? '\u00A0' : '\u00A0\u00A0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ 🪄 ✨
Description
This PR creates some re-usuable components for when we want inline messages used to describe inputs. Errors, warnings, and captions. These are messages that should be associated by an
aria-describedby
that currently are not in GitHub Desktop (or not consistently).This a POC towards implementing inline errors that we can use consistently across Desktop in our forms and that are accessible.
Accessibility design considerations:
aria-described
and therefor announced.GitHub Desktop Form submissions state:
Current GitHub Desktop Messaging:
Lastly.. there are some other more one off messaging types like the warning near the commit button about protected branches, warnings about bidi characters, and warning about applying stashes. But, all those do not share a form submission process and I think could be treated and assessed in one off ways (tho could be reevaluated to see if there is any shared approach).
Notes about types of errors/warnings in GitHub Desktop.
aria-live
in combination with input debouncing and adding non-visible characters to increase probability that the error is read. We could revisit this approach if/when we switch the form submission in dialogs to always have the submit button enabled so that the screen reader user is only bothered by the errors when they attempt to submit.Some issues:
In the above, there are two related issues going on and somewhat competing. Form submission buttons being disabled and how errors/warnings are displayed.
Currently, a change to this is not required for audit remediation. However, for the current tickets about the inline warnings that should be errors, the quick fix would be to throw them up in the dialog banner. But, that still leaves us with the aforementioned problems and is a little bit of regression. I am proposing that we become consistent with error/warning handling so we don't make a change now than then implement another one down the road when we are focusing on less immediate audit remediation.
Since enabling the submit forms is a much more disruptive and three are 42 instances of disabled buttons we would need to comb through. I am proposing that we don't change that approach now, but improve our errors.
Improving them by:
Inline Errors/local validation:
aria-live
on user input to drastically increase chance that a screen reader user will be made aware of the error or warning.aria-described
by and will be announced on input focus.Server side validation/not inline:
6. Separately to the inline work, make it so that the welcome flow and dialog banner/flashes look and behave consistently with focus management and being screen read on submission.
No change to commit button tooltip error management.
Questions.
.... probably will have more.. just trying to capture everything I currently have here.
Some reference material/discussion:
To be continued...
<link>
create a repository</link>
instead? Be a caption not in error that is shown beneath the error in this particular use case?aria-describedby
similar to how our labels are automatically tied to our inputs? Primer React does it with having aFormControl
component with FormControl.Validation children. https://primer.style/react/FormControl