Added support for Django 5.2. #200
Conversation
|
|
||
| form.helper.form_show_errors = False | ||
| html = render_crispy_form(form) | ||
| assert html.count("error") == 0 | ||
| if django.VERSION < (5, 2): | ||
| assert html.count("error") == 0 |
There was a problem hiding this comment.
This one concerns me. When this option (form_show_errors) is enabled Django will add the 'aria_describedby' pointing at a error message that doesn't exist. The docs for this feature say:
Default set to True. It decides whether to render or not form errors. If set to False, form.errors will not be visible even if they happen. You have to manually render them customizing your template. This allows you to customize error output.
The emphasis is mine. Maybe we need to trust that folk doing this know what they are doing? I'm not sure if there's anything else we can do to help here.
Needs some thought. Opinions welcome.
There was a problem hiding this comment.
Perhaps pointing to something like ErrorList.field_id
Default set to True. It decides whether to render or not form errors. If set to False, form.errors will not be visible even if they happen. You have to manually render them customizing your template. This allows you to customize error output.
Note that from Django 5.2,aria-describedbywill point to the expected error id (id="{{ errors.field_id }}_error") when errors should be rendered. Be sure to have the correct id on your customized template. SeeErrorList.field_idfor details.
Something something.
| @@ -1,7 +1,7 @@ | |||
| <form class="form-horizontal" method="post"> | |||
| <div id="div_id_is_company" class="mb-3 form-check form-switch row"> | |||
| <div class="offset-lg-2 col-lg-8"> | |||
| <input type="checkbox" name="is_company" class="checkboxinput form-check-input" role="checkbox" id="id_is_company" /> | |||
| <input type="checkbox" aria-describedby="id_is_company_helptext" name="is_company" class="checkboxinput form-check-input" role="checkbox" id="id_is_company" /> | |||
There was a problem hiding this comment.
This one is a bit odd. aria-describedby for help text has been about for a while. Needs some investigation to understand why this is changing for us in 5.2.
There was a problem hiding this comment.
bisected to django/django@1e05431
There was a problem hiding this comment.
Previous to django/django@1e05431 Django looked at the field's help text (self.field.help_text), but this got changed to looking at the BoundField's help text (self.help_text). That's impacting this test here as the help text is set like this:
form["is_company"].help_text = "is_company help text"Rather than when setting the field on the form:
class MyForm(forms.Form):
my_field = forms.CharField(help_text="...", ...)I think, that's ok as we would expect help text to be referenced with an aria_describedby, but it is a change that I had not spotted when working on the patch to Django.
@sarahboyce -- sorry for the ping. I spotted this change, and would appreciate your thought with your fellow hat on. I think it's likely ok but wondered if you could give a second opinion on if we need to do anything further at django/django for this behaviour change?
There was a problem hiding this comment.
I think, that's ok as we would expect help text to be referenced with an aria_describedby, but it is a change that I had not spotted when working on the patch to Django.
I think this is now more correct.
I don't think we need to add further docs, we could add a test for this explicitly so we do not revert this behavior
| @@ -1,6 +1,6 @@ | |||
| <form method="post" > | |||
| <div id="div_id_text_area" class="form-floating mb-3"> | |||
| <textarea name="text_area" cols="40" rows="10" placeholder="text_area" class="textarea form-control is-invalid" required id="id_text_area" aria-invalid="true"> | |||
| <textarea aria-describedby="id_text_area_error" name="text_area" cols="40" rows="10" placeholder="text_area" class="textarea form-control is-invalid" required id="id_text_area" aria-invalid="true"> | |||
There was a problem hiding this comment.
These ones adding references to errors I'm happy with. That was a new feature in 5.2.
There was a problem hiding this comment.
https://github.com/django-crispy-forms/crispy-bootstrap5/blob/main/crispy_bootstrap5/templates/bootstrap5/layout/field_errors.html This (and the block version) will need updating so that the new aria-describedby has a target to reference.
See error when testing with the 'wave' accessibility checker:
smithdc1
force-pushed
the
django52
branch
3 times, most recently
from
2bb2f51 to
75ed880
Compare
Django 5.2 added aria-describedby for errors. A <div> with an 'id' is added so that errors can be read by screen reader users.
smithdc1
force-pushed
the
django52
branch
2 times, most recently
from
2e6369c to
61b9f6d
Compare
smithdc1
changed the title
smithdc1
force-pushed
the
django52
branch
3 times, most recently
from
f834182 to
7a8f00e
Compare
| <div id="{{field.errors.field_id}}_error"> | ||
| {% else %} | ||
| <div id="{{field.auto_id}}_error"> | ||
| {% endif %} | ||
| {% for error in field.errors %} | ||
| <span id="error_{{ forloop.counter }}_{{ field.auto_id }}" class="invalid-feedback"><strong>{{ error }}</strong></span> |
There was a problem hiding this comment.
invalid-feedback needs to move up to the div.
No description provided.