django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #32820 --- Improved form accessibility on errors.

Open demestav opened this issue 2 years ago • 12 comments

Relevant ticket.

By adding aria-invalid="true", the accessibility of the form is improved. As per the description of the ticket, the attribute allows a screen reader to identify the error on the field.

demestav avatar Oct 30 '21 11:10 demestav

  • There are lots of test failing where a field error is involved. They assert against the outputted HTML which misses the attribute added in this PR, aria-invalid="true".
  • I see that many of the tests are repeated for every field. So far I have added this in the CharField only as a proof-of-concept.

I would greatly appreciate the reviewer's feedback on the above.

demestav avatar Oct 30 '21 16:10 demestav

A couple of initial thoughts are:

  • Does it make sense to have aria-invalid alone? Isn't this just saying 'hey there's an error here". That's not very helpful without an aria-describedby to explain why it is an error?

  • The change as proposed will add this to every input that has an error. Is that correct? If in my own custom template I've made the right association e.g. https://code.djangoproject.com/ticket/32819 then I don't need this as well?

What are your thoughts on these points?

smithdc1 avatar Oct 30 '21 16:10 smithdc1

Thank you for reviewing this.

Does it make sense to have aria-invalid alone? Isn't this just saying 'hey there's an error here". That's not very helpful without an aria-describedby to explain why it is an error?

That is an interesting point but first let me clarify that I tried to implement what the accepted ticket is describing. I am not an expert on web accessibility. Having said that, I am willing to put the necessary effort to see this through in order to have a meaningful impact towards improving Django's accessibility.

The ticket links to ARIA21 document. Indeed, as you describe, aria-invalid indicates that the field has an error without any further information. However, it is still valid to use it on its own, as example 1 shows in the link above.

The change as proposed will add this to every input that has an error. Is that correct?

That is correct.

If in my own custom template I've made the right association e.g. https://code.djangoproject.com/ticket/32819 then I don't need this as well?

You mean the aria-describedby attribute right? ARIA1 document describes this as a way "to provide programmatically determined, descriptive information about a user interface element". It does not mention anything about errors. I guess it can be used like that (as the GOV.UK example given in the ticket) and the users can infer that it is an error based on context.

From the above, I can understand that aria-invalid explicitly indicates an error to a field, while the aria-describedby offers descriptive information that could implicitly indicate an error along with its description. Based on the above, I guess we have three choices:

  1. Just use aria-invalid to indicate the error without description, as this is still valid. Any further improvements can be tackled in another PR

  2. Just use aria-describeby and indicate the error through that similar to GOV.UK example

  3. Use both aria-invalid and aria-describeby, for explicitly indicating the error and associate the error message, respectively. This is shown as example 2 in ARIA21 document.

Personally I vote for 3 without thinking yet about the implementation. What do you think?

demestav avatar Oct 30 '21 19:10 demestav

Hmm it seems to be a bit more complex than that. As I understand it, aria-invalid is only necessary when there is no error message available. However, I think it's good (or at least not harmful) to include it even if there is an error description.

I'm not 100% sure if aria-describedby is the right thing. There are some suggestions for calling attention to errors: https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA18 and possibly https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA19. However, I believe they're both intended for cases where the validation is done on the client side, so I don't think it's for us.

I think the relevant things to look at are:

  • https://www.w3.org/WAI/WCAG21/Understanding/error-identification
  • https://www.w3.org/WAI/WCAG21/Understanding/error-suggestion
  • https://www.w3.org/WAI/WCAG21/Understanding/status-messages - but again, I think this one is intended for changes to the current page - not loading a new page as we do for validation.

knyghty avatar Nov 01 '21 13:11 knyghty

I took a look to the linked documents. I also understand that some of the techniques are intended for client-based validation and correction which does not apply in our case. I think we are left with:

  • https://www.w3.org/WAI/WCAG21/Understanding/error-identification
  • https://www.w3.org/WAI/WCAG21/Understanding/error-suggestion

Starting from "error-suggestion", and "Situation A: If a mandatory field contains no information" directs to ARIA2. The technique's object is "... to provide programmatic indication that a form field (which shown through presentation to be required) is mandatory for successful submission of a form" and introduces the property "aria-required" to achieve this. However, HTML also specifies the "required" property, and modern screen readers seem to support that (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-required_attribute see first note). Since Django already inserts "required" where is needed, we can safely assume that we don't need to change something regarding this.

The "error-identification" document directs to ARIA21 which talks about how to use "aria-invalid" and "aria-describeby" to indicate errors (more details in my previous comment)

Hmm it seems to be a bit more complex than that.

Based on the above, I believe the list of choices I posted previously still stands. It is not clear to me what is missing or, is invalid.

demestav avatar Nov 02 '21 07:11 demestav

I found https://www.w3.org/WAI/WCAG21/working-examples/aria-invalid-data-format/ as a worked example quite helpful. It shows aria-invalid being used when there is no aria-describedby (and visa versa).

I therefore I think option 1 could for this PR, if/when aria-descriedby gets added we can tackle more complex cases then?

A couple of comments.

You'll need to check for usage / examples in docs. There'll be HTML examples of rendered widgets/forms, do they need updating?

I think we need a flag so folk can opt out, similar to use_required_attribute as introduced in https://github.com/django/django/pull/6352 / ticket #22383 for the required attribute. Folk will already be using more advanced aria techniques (example) so I fear a blanket change would be seen as backwards incompatible and/or wrong. It would be good to get a view of a fellow, maybe we could ask @felixxm to comment?

smithdc1 avatar Nov 03 '21 07:11 smithdc1

I'm not sure we really have a way to distinguish between "generic" error messages and error messages that properly describe how to fix the problem. It seems like we encourage (or should encourage) good error messages, though there are some cases where it's not appropriate, such as a login form not telling you exactly what the problem is. I suppose in most cases these are probably non-field errors, though.

So the most appropriate thing might be to only have aria-describedby. That said, it doesn't seem like it's wrong per-se to also use aria-invalid even if there is a good error message.

Maybe @thibaudcolas (ticket opener) has a better idea.

knyghty avatar Nov 03 '21 12:11 knyghty

Sorry I’m only getting to this now. aria-describedby is essential whenever there is an error message, and aria-invalid would be essential whenever there isn’t an error message. Re having them used together – I’ll do more research just to be sure.

It’s worth mentioning that browser and screen reader support for ARIA isn’t perfect, so we need to check what the specs say, but also what’s the current behavior of various common assistive technologies as we make those decisions. For reference here is:

thibaudcolas avatar Jan 05 '22 09:01 thibaudcolas

I’ve just done the additional research over the weekend and (edit: attribution) @demestav’s "option 3" ”aria-invalid ="true" + aria-describedby pointing at error message” seems to me like it would be the best for Django. It’s recommended in the non-normative ARIA21 technique he shared, and by a renowned expert in the field. I found one pattern that seems better than it in some cases, but I don’t think it’s as good of a fit for Django.

Recommendations for the pattern

I could find the following sources recommending aria-invalid="true" usage alongside aria-describedby:

When visible text is used to programmatically identify a failed field and / or convey how the error can be corrected, setting aria-invalid to "true" is not required from a strict compliance standpoint but may still provide helpful information for users.

[…] Or you can use aria-describedby to make the association, like this: […] <input type="text" id="this" aria-describedby="error" ...> […] <p id="error">Don't forget to enter your first name!</p>

You can also use aria-invalid="true" on the input, as you have done, for belt and braces information.

Those two sources indicate aria-describedby pointing at the error message could be sufficient. Using aria-invalid="true" basically just helps unambiguously identifying fields that are in error, rather than relying on the wording of the error messages conveyed via aria-describedby only.

Alternative

I couldn’t find anyone actively recommending against this combination, however the GOV.UK design system decided not to use it. They instead added an "Error:" prefix for all error messages, as visually hidden / screen-reader-only text:

[…]
  <p id="national-insurance-number-error" class="govuk-error-message">
  <span class="govuk-visually-hidden">Error:</span> Enter a National Insurance number in the correct format
  </p>
<input class="govuk-input govuk-input--error" id="national-insurance-number" name="national-insurance-number" type="text" aria-describedby="national-insurance-number-hint national-insurance-number-error">

I can see some clear advantages to this:

  • It’s equally as unambiguous for screen reader users that the fields are in error (compare Django’s This field is required. message or Enter a whole number. with Error: This field is required. or Error: Enter a whole number.).
  • "Error:" is more plain language than "Invalid entry"
  • Since this doesn’t rely on ARIA, screen reader support for this is better than aria-invalid="true"

I can also see some issues:

  • It’s more opinionated markup to maintain / less "by-the-book semantic HTML"
  • Django’s error messages are meant to be customized with no oversight on how implementers will ultimately word them, so adding a prefix wouldn’t always be appropriate.
  • It’s one more string that will need translations (not necessarily a big deal for such a generic word but does constraint the languages with which Django’s forms rendering could be used).

Recommendation for Django

Based on this, for Django, I’d still recommend aria-invalid="true" wherever possible, and aria-describedby for the error message wherever possible. So both together, wherever possible. It’s a more established pattern than bespoke error message prefixes, and lower maintenance. aria-invalid="true" support isn’t as good, but good enough for something that’s used in addition to aria-describedby.

Caveats

  • This is based on a11ysupport.io data, which only tested support on <input type="text"> and <select>.
  • I only looked at aria-invalid="true". There are other aria-invalid values, which are harder to use and I wouldn’t recommend at this time.

Breaking changes

From @smithdc1:

I think we need a flag so folk can opt out, similar to use_required_attribute as introduced in #6352 / ticket #22383 for the required attribute. Folk will already be using more advanced aria techniques (example) so I fear a blanket change would be seen as backwards incompatible and/or wrong. It would be good to get a view of a fellow, maybe we could ask @felixxm to comment?

This changes how form fields are presented to end users, which would be critical functionality for a lot if implementers, so in principle I like the idea of it being a breaking change you can opt out from. In practice, the only case I can think of where this change would be undesirable is when implementers already use "Error:" prefixes for error messages and aria-describedby. The crispy-forms-gds template pack doesn’t currently have those prefixes, so it’d still be a desirable enhancement in this case. Even if they updated their pattern to have those prefixes, the only issue is that screen readers would redundantly communicate the state twice ("invalid entry", then "Error: […]").

thibaudcolas avatar Jan 10 '22 10:01 thibaudcolas

Thank you @thibaudcolas for the detailed breakdown.

I’ve just done the additional research over the weekend and @smithdc1’s "option 3" ”aria-invalid ="true" + aria-describedby pointing at error message” seems to me like it would be the best for Django.

To make sure, are you referring to this comment? (couldn't find any "option 3" by @smithdc1)

demestav avatar Jan 12 '22 19:01 demestav

@demestav yes, option 3 by… you! 😄 sorry!

thibaudcolas avatar Jan 12 '22 21:01 thibaudcolas

In order to implement aria-describedby part, we need to associate the input field with the field error list using the HTML attribute id; the unique id of the error list element is the value of the aria-describedby attribute of the input element. For example:

<ul id="errorlist_surname" class="errorlist"><li>This is the error message</li></ul>
<input type="text" name="surname" required aria-invalid="true" id="id_surname" aria-describedby="errorlist_surname">

The error list and input field are rendered in different places in code and they both need to be able to generate the same unique id. One way I can think of, is to use the name of the input field as in the example above. However, since the field error list is generated by the ErrorList class which (if I understand the forms internals correctly) is agnostic to the field that is bound to, this may not be possible without modigfying the ErrorList class.

I would appreciate the reviewers' comment on this.

demestav avatar Jan 30 '22 10:01 demestav

Hi, we recently have had an accessibility audit for a project for the French government, and the lack of both aria-invalid=”true” for fields in error and aria-describedby="xxx" (both for hint and error messages) has been reported as a major violation of the RGAA criterion "11.11 - Dans chaque formulaire, le contrôle de saisie est-il accompagné, si nécessaire, de suggestions facilitant la correction des erreurs de saisie ?". Sorry for the French, but to my knowledge the RGAA 4.1 is not yet available in English (I can put an extract of the audit report if needed but it's in French too)

If I understand correctly, the aria-describedby part has been solved in https://github.com/django/django/pull/14515 and this would solve the aria-invalid=”true” part. Is there any way I can help with this issue?

Ash-Crow avatar Oct 07 '22 08:10 Ash-Crow

@Ash-Crow thanks for this. It's useful to have information like this on why accessibility is important for the framework.

I think the best way you can help on this specific issue is to read through the code and the comments on this PR, and try to suggest an implementation that works regarding @demestav's last comment. I think this is the final tricky bit that needs to be solved before this can be given a full review and merged.

knyghty avatar Oct 07 '22 09:10 knyghty

@Ash-Crow thanks for the feedback, really useful and I believe it validates what was discussed so far in this thread.

I will check out the pull request you linked, and see if we can built upon it, to progress this one.

EDIT: It seems that the linked PR is Closed and not merged to main.

demestav avatar Oct 07 '22 12:10 demestav

@demestav oh yes you are right.

Ash-Crow avatar Oct 17 '22 13:10 Ash-Crow

Just a heads’up this is being completed on #16933. It makes the same change as here, but is further ahead with tests and documentation.

thibaudcolas avatar Jun 04 '23 21:06 thibaudcolas

Closing as per @thibaudcolas's comment

smithdc1 avatar Jun 04 '23 22:06 smithdc1