Opened 4 years ago

Closed 3 years ago

Last modified 3 weeks ago

#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 Thibaud Colas)

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:

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 Thibaud Colas, 4 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 4 years ago

Triage Stage: UnreviewedAccepted

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=presentation I 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 😀

comment:3 by Thibaud Colas, 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_p isn’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, and as_p being gone doesn’t guarantee better patterns will be used.
  • as_table on the other hand results in forms that are confusing for screen reader users, and would fail an accessibility audit.
  • as_ul is 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 Tom Carrick, 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).

Last edited 4 years ago by Tom Carrick (previous) (diff)

comment:5 by Jacob Rief, 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 David Smith, 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 David Smith, 3 years ago

Owner: changed from nobody to David Smith
Status: newassigned

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In cb82ded4:

Refs #32339 -- Added rendering tests for forms with CheckboxSelectMultiple and SelectMultiple widgets.

comment:9 by Carlton Gibson, 3 years ago

Has patch: set

comment:10 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Carlton Gibson <carlton@…>, 3 years ago

In c8459708:

Refs #32339 -- Added use_fieldset to Widget.

comment:12 by Carlton Gibson, 3 years ago

Triage Stage: Ready for checkinAccepted

comment:13 by Carlton Gibson, 3 years ago

Patch needs improvement: set
Last edited 3 years ago by Carlton Gibson (previous) (diff)

comment:14 by Carlton Gibson <carlton.gibson@…>, 3 years ago

In 8320964:

Refs #32339 -- Added base form renderer to docs.

comment:15 by Carlton Gibson <carlton.gibson@…>, 3 years ago

In 476d4d50:

Refs #32339 -- Allowed renderer to specify default form and formset templates.

Co-authored-by: David Smith <smithdc@…>

comment:16 by Carlton Gibson <carlton@…>, 3 years ago

In fde946da:

Refs #32339 -- Restructured outputting HTML form docs.

In the topic doc, promoted the Reusable form templates section.

In the reference, re-grouped and promoted the default str()
rendering path, and then gathered the various as_*() helpers
subsequently.

Thanks to David Smith for review.

comment:17 by Carlton Gibson, 3 years ago

Patch needs improvement: unset

comment:18 by Carlton Gibson <carlton@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In ec565938:

Fixed #32339 -- Added div.html form template.

comment:19 by Carlton Gibson <carlton.gibson@…>, 3 years ago

In d126eba:

Refs #32339 -- Deprecated default.html form template.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:20 by GitHub <noreply@…>, 2 years ago

In 9ac97e7e:

Refs #32339 -- Updated Form API docs to prefer as_div() output style.

comment:21 by Carlton Gibson <carlton.gibson@…>, 2 years ago

In 7713370:

[4.1.x] Refs #32339 -- Updated Form API docs to prefer as_div() output style.

Backport of 9ac97e7eb5a74f813012715c7598c8608e78e178 from main

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 23 months ago

In 98756c68:

Refs #32339 -- Changed default form and formset rendering style to div-based.

Per deprecation timeline.

This also removes "django/forms/default.html" and
"django/forms/formsets/default.html" templates.

comment:23 by GitHub <noreply@…>, 23 months ago

In b209518:

Refs #32339 -- Deprecated transitional form renderers.

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In 4038a8d:

Refs #32339 -- Doc'd BaseFormSet.as_div()

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In ce10686:

[4.2.x] Refs #32339 -- Doc'd BaseFormSet.as_div()

Backport of 4038a8df0b8c20624ba826cf9af8f532e5a51aaa from main

comment:26 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In 7fd69e5:

[4.1.x] Refs #32339 -- Doc'd BaseFormSet.as_div()

Backport of 4038a8df0b8c20624ba826cf9af8f532e5a51aaa from main.

comment:27 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In 232b60a:

Refs #32339 -- Updated docs to reflect default <div> style form rendering in Django 5.0.

Follow up to 98756c685ee173bbd43f21ed0553f808be835ce5.

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In 3cc7a92:

Refs #32339 -- Doc'd setting a form's template_name is recomended over using as_* methods.

comment:29 by GitHub <noreply@…>, 19 months ago

In 4a5753fb:

Refs #32339 -- Fixed super() call in deprecated renderers.

Missing function call () leads to:

TypeError: descriptor 'init' of 'super' object needs an argument

Regression in b209518089131c6b4afd18b1d9c320ba3521c5ab.

comment:30 by nessita <124304+nessita@…>, 3 months ago

In 7e6e1c83:

Refs #32339 -- Adjusted deprecation warning stacklevel in transitional form renderers.

comment:31 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In c56e127:

Refs #32339 -- Updated formset docs to reflect default rendering as as_div.

comment:32 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In 4c65aec:

[5.1.x] Refs #32339 -- Updated formset docs to reflect default rendering as as_div.

Backport of c56e1273a9d87ffad3a84fb597550f79b9820281 from main.

Note: See TracTickets for help on using tickets.
Back to Top