#32339 closed Bug (fixed)
Accessibility issues with Django forms as_table, as_ul, as_p rendering helpers
| Reported by: | Thibaud Colas | Owned by: | David Smith |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | accessibility, forms, wcag |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | yes |
Description (last modified by )
The Django forms API has three methods available to render whole forms at once: as_table, as_ul, as_p. All of these three have issues of varying severity. Although it’s not always the case that developers rely on those methods when implementing forms, I think it’s important for Django to not provide APIs that make it easy for developers to do the wrong thing – just like with security, Django should have safe defaults.
Like #32338 – those issues are with the vanilla rendering of forms, and also with related documentation code snippets. If you want to troubleshoot this, here is the form I used for testing:
- Live form as_p: http://django-admin-tests.herokuapp.com/forms/example_form/p/
- Live form as_ul: http://django-admin-tests.herokuapp.com/forms/example_form/ul/
- Live form as_table: http://django-admin-tests.herokuapp.com/forms/example_form/table/
- Form fields definitions: https://github.com/thibaudcolas/django_admin_tests/blob/main/django_admin_tests/forms.py
- Template: https://github.com/thibaudcolas/django_admin_tests/blob/main/django_admin_tests/templates/django_admin_tests/example_form.html
as_table
as_table is the most problematic of all form output styles. Although technically tables for layout can be ok (see WebAIM’s Creating Accessible Tables), I think the general consensus is that they should be a thing of the past. Depending on how screen readers interpret the markup, there is a very real chance that they will incorrectly announce the table with tabular navigation, made for tabular data. This will make the form at best very verbose to navigate, at worst plain confusing.
There are well-established workarounds to this keep on using tables like this (like adding role="presentation" to the <table>), but there really is no reason to use tables for layout these days, as identical or better layouts can be done with CSS (for example for a responsive form). I think this can also potentially be a source of confusion for developers – so Django shouldn’t encourage potentially-misused markup.
as_ul
as_ul essentially has the same problem as as_table but to a lesser extent. Wrapping whole forms in list items makes the form more verbose to navigate, and can be a bit confusing, but that’s about it. Semantically it’s also incorrect (unordered lists means the order of items doesn’t matter). I don’t think it’s very problematic, but it’s also not very useful to start with.
as_p
The as_p markup doesn’t cause any accessibility issues that I’m aware of, but it can be problematic nonetheless because p only allows phrasing content (see for example #31189).
From a pure "end-user outcome" perspective there is no problem with this to my knowledge, but again I think Django should err on the side of safe defaults and not encourage patterns that can lead to invalid HTML, when developers use custom widgets that would contain tags invalid in phrasing content (for example a date picker). Even if browsers handle the invalid HTML just fine, it’s very common for accessibility testing tools to implement HTML validation rules, and Django should strive to minimize false positives with those tools if there are obvious alternatives.
Proposed solution
I would recommend removing all three output styles. Even if their output can be tweaked to circumvent those issues, they make it too easy to do the wrong thing. They also discourage developers from using <fieldset> and <legend> to appropriately group related fields. Although this isn’t needed all the time, for larger forms it’s important for there to be clear sections. I think they also have an impact on how likely developers are to properly label fields as either required or optional where relevant – Django’s default output has no such labeling, so developers either have to remember to add a custom label for each field suffixed with (required), or skip those output styles.
If Django wants to provide a shortcut to render whole forms, I think it should be based on <div> (optionally with a class name), so it still allows for layout niceties but works as expect for all users otherwise. I think the documentation should also be updated to cover why it’s also desirable to render fields individually so they can be grouped in sections.
---
I realize this would be a breaking change, with a lengthy deprecation period. During this deprecation period, it would also be great to:
- Document the gotchas with the three shortcuts.
- Recommend marking up
<table>and<ul>with role="presentation", so screen readers ignore the semantics.
Change History (32)
comment:1 by , 4 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 4 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 4 years ago
Thank you Carlton, that makes complete sense to me. #31026 sounds very promising to solve this, particularly the "Use this template to restore the previous behavior" scenario. I’ll take a look and see whether I can provide a more detailed proposal.
Re whether the breakage is worth it or not – I think it’s worth me expressing more nuance between the different methods:
as_pisn’t something I would recommend, and isn’t something I think Django should recommend, but it’s not actively causing big issues for end users that Django could solve by removing it. All the removal would achieve is increased awareness of how to implement forms better, but it’s not like that can’t be achieved otherwise, andas_pbeing gone doesn’t guarantee better patterns will be used.as_tableon the other hand results in forms that are confusing for screen reader users, and would fail an accessibility audit.as_ulis somewhere in-between.
---
For role=presentation – as far as I understand this would be a documentation change only. Is this something you think we could be doing right now? If so I might also spend more time reviewing other potential accessibility improvements to code snippets in the documentation.
comment:4 by , 4 years ago
I think the easiest / quickest / least trouble-causing thing to do here is to put a big warning in the docs along the lines of "These methods are suitable only for quickly testing forms and shouldn't be used in production code because, x, y and z" along with perhaps a note on the role attr as a quick fix for old code.
I agree we probably can't remove them, but we could at least do this as a first step, along with making them more accessible where possible.
In the future? I'm not sure, I defer to Carlton, and I think his suggestion re: the form templates is good. Also perhaps a system check might work? e.g. complain if you use the methods, but it's easy enough for people to disable.
One more extreme thought: perhaps we can do as we did with the old {% trans %} and co tags -- de-document them but leave them there for old code to use, perhaps to be removed in some far future date (or not).
comment:5 by , 4 years ago
I'm strongly in favor of adding a new method as_div() together with configurable CSS classes to the form renderer. Indeed I do this myself on many of my own forms.
We then maybe also should let the __str__()-method use that new as_div() method instead of as_table().
– just my two cents, Jacob
comment:6 by , 3 years ago
There is currently a comment in the Form.as_table() doc string which suggested there was some work to do as there is no semantic division between the forms. This is expected to be removed as part of the move to template based form rendering. See comment
comment:7 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:10 by , 3 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:12 by , 3 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
comment:17 by , 3 years ago
| Patch needs improvement: | unset |
|---|
Hi Thibaud, thanks for this.
Let's accept, with the proviso that I don't think we can really remove/deprecate the
as_p()&co methods — they're just too widely used.We can steer folks to better methods, improve the markup as best we can, and add the
role=presentationI think without issue, but there's just too much code out there (I think) to remove them.One issue we have is breaking peoples existing pages if we change the rendered HTML (so that CSS/JS selectors change for instance).
Can I point you to #31026, which is on my list for early in the 4.0 (i.e. next) cycle. If we can get that in, then we'd be able to document a Use this template ... to restore the previous HTML, which then would give us room to introduce a more significant change.
I hope that makes sense 😀