Opened 3 months ago
Last modified 2 months ago
#35706 assigned Cleanup/optimization
Improve admin add/change form top level errors for screen readers
Reported by: | Sarah Boyce | Owned by: | Sanjeev Holla S |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | accessibility |
Cc: | Marijke Luttekes, Thibaud Colas, Tom Carrick, Sarah Abderemane | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
I was looking into best practices for "top level" form errors and according to the W3C Design system we can make the following improvements...
Prefix the word “Error:” to the document’s <title>
Confirmed that this is the first thing announced by screen readers when the page loads.
I think this is especially useful considering we have a "Save and add another" button, given you were on the add page and submit using "Save and add another" going to the add page is the "correct" state.
Have a summary of the errors at the top of the page
https://design-system.w3.org/styles/form-errors.html has an example of the top level note. This is similar to our "Please correct the errors below"
{% if errors %} <p class="errornote"> {% blocktranslate count counter=errors|length %}Please correct the error below.{% plural %}Please correct the errors below.{% endblocktranslate %} </p> {{ adminform.form.non_field_errors }} {% endif %}
We could (should?) update this to:
- be above the form title
- have
tabindex="-1"
to be the first thing read on the page - be in a div with
role="alert"
with anaria-labelledby
attribute set for the title - also include the individual form errors (which link to the form elements) in this summary box
I hope the accessibility team can confirm if any of these are not necessary or not wanted
This a bit related to #32819
Change History (16)
comment:1 by , 3 months ago
Type: | Uncategorized → Cleanup/optimization |
---|
comment:2 by , 3 months ago
Triage Stage: | Unreviewed → Accepted |
---|---|
UI/UX: | set |
Version: | 5.0 → dev |
comment:3 by , 3 months ago
For me:
Yes:
- title prefix seems reasonable and useful for everyone
- Using
role
andaria-labbeledby
also seem obvious improvements - Summary box - again seems useful and no obvious harm other than needing to scroll more to see the form - but I think this is more than made up for by the jump to field link, which just seems like a very nice UX improvement
Maybe:
- Moving the form above the title - not sure. If that's what W3C are doing for themselves, I assume this is backed by something and I'm inclined to say, yes, sure.
- Usually it's not a good idea to mess with
tabindex
as it breaks people's expectations. I'm not sure if this is a good exception or not, will ask around. Again, inclined to say that W3C have probably done their homework.
comment:4 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
follow-up: 6 comment:5 by , 3 months ago
Hey
I have started to work on this issue. To give an update, I have completed working on
- Prefix the word “Error:” to the document’s <title>
- Add the error message above form title (below the breadcrumbs).
- I have included all the error messages in the summary box.
Also one of my suggestion is to improve the default error message (which is used when the field is required). As of now the error message shows "This field is required" for any field. But I am thinking of changing this to "Enter a %(field_title)s", so that it will be more informative in the summary box. I have made this change as of now. Let me know if its not required.
follow-ups: 8 9 comment:6 by , 3 months ago
Replying to Sanjeev Holla S:
Also one of my suggestion is to improve the default error message (which is used when the field is required). As of now the error message shows "This field is required" for any field. But I am thinking of changing this to "Enter a %(field_title)s", so that it will be more informative in the summary box. I have made this change as of now. Let me know if its not required.
I think updating the error message might not be the way to go as some people might be relying on it in tests. We probably want something a bit like django/forms/templates/django/forms/errors/dict/ul.html
(specifically {% for field, error in errors %}<li>{{ field }}{{ error }}</li>{% endfor %}
as it gets a field and error) but probably requires some tweaks
If you have done the bit prefixing the word “Error:” to the document’s <title>, you can add a test and raise a PR with "Refs #35706" as these two bits can be merged seperatedly
comment:7 by , 2 months ago
Has patch: | set |
---|---|
Needs tests: | set |
The PR for the 1st bit is https://github.com/django/django/pull/18527
comment:8 by , 2 months ago
Replying to Sarah Boyce:
Replying to Sanjeev Holla S:
Also one of my suggestion is to improve the default error message (which is used when the field is required). As of now the error message shows "This field is required" for any field. But I am thinking of changing this to "Enter a %(field_title)s", so that it will be more informative in the summary box. I have made this change as of now. Let me know if its not required.
I think updating the error message might not be the way to go as some people might be relying on it in tests. We probably want something a bit like
django/forms/templates/django/forms/errors/dict/ul.html
(specifically{% for field, error in errors %}<li>{{ field }}{{ error }}</li>{% endfor %}
as it gets a field and error) but probably requires some tweaks
If you have done the bit prefixing the word “Error:” to the document’s <title>, you can add a test and raise a PR with "Refs #35706" as these two bits can be merged seperatedly
Hey! Got the point. I have created a PR for the first bit. Please check it and suggest changes if required.
comment:9 by , 2 months ago
Replying to Sarah Boyce:
Replying to Sanjeev Holla S:
Also one of my suggestion is to improve the default error message (which is used when the field is required). As of now the error message shows "This field is required" for any field. But I am thinking of changing this to "Enter a %(field_title)s", so that it will be more informative in the summary box. I have made this change as of now. Let me know if its not required.
I think updating the error message might not be the way to go as some people might be relying on it in tests. We probably want something a bit like
django/forms/templates/django/forms/errors/dict/ul.html
(specifically{% for field, error in errors %}<li>{{ field }}{{ error }}</li>{% endfor %}
as it gets a field and error) but probably requires some tweaks
If you have done the bit prefixing the word “Error:” to the document’s <title>, you can add a test and raise a PR with "Refs #35706" as these two bits can be merged seperatedly
I will create a PR for the 2nd bit as well. So I will show the list of errors in the summary box in the same way as suggested (without changing the default error message and using {% for field, error in errors %}<li>{{ field }}{{ error }}</li>{% endfor %}
). Even this should be added in all the templates wherever the prefix "Error:" was added in the title I guess.
comment:11 by , 2 months ago
Has patch: | unset |
---|---|
Needs tests: | unset |
comment:12 by , 2 months ago
I have created a PR for replacing the '_(...)' with translate tag in template. Please check it.
PR - https://github.com/django/django/pull/18533
comment:14 by , 2 months ago
So for showing the list of errors in the summary box, I thought of using field.field.label
(To show which field it is) and field.errors
to show the errors of that field. Since field.errors includes HTML tags like <ul>, directly displaying it might not look clean (in summary box). Therefore, I thought of using {{ field.errors|striptags }}
to remove the HTML tags and present only the plain text error messages, ensuring the summary box looks neat.
Is this fine?
follow-up: 16 comment:15 by , 2 months ago
It might be enough/simpler to have something more generic like "Field X has validation errors." (where 'Field X' is a link to take you to the field), or "The following fields have errors:" (and a list of fields with links).
comment:16 by , 2 months ago
Replying to Sarah Boyce:
It might be enough/simpler to have something more generic like "Field X has validation errors." (where 'Field X' is a link to take you to the field), or "The following fields have errors:" (and a list of fields with links).
Okay then its fine. I have few doubts regarding this, I have mentioned them in the PR
PR - https://github.com/django/django/pull/18537
Thank you Sarah for all the details! Accepting following the advice from the shared links.