Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18484 closed Uncategorized (fixed)

csrfmiddlewaretoken enclosed in redundant invisible div

Reported by: hedleyroos@… Owned by: nobody
Component: Forms Version: 1.4
Severity: Release blocker Keywords: csrf
Cc: bnomis@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Refer to https://github.com/django/django/blob/master/django/template/defaulttags.py#L49. Why is the hidden input enclosed in a div with style display none? It causes problems on certain low-end handsets. These handsets do not include inputs in hidden containers as part of the post.

Change History (13)

comment:1 follow-up: Changed 3 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Hmm, that is really broken behaviour. http://www.w3.org/TR/html401/interact/forms.html#h-17.13.2

I believe the original reason was to ensure the inserted div had no effect on appearance. You cannot put the input in without a div due to HTML validity constraints. IIRC, having been tortured by IE for several years, I was worried that IE would do funny things with divs that are not completely empty, and give them some pixel space etc. (I've come across very similar bugs with almost empty divs in IE).

That concern is probably passed now, and if this is causing a genuine problem, let's remove the 'style="display:none"'.

comment:2 Changed 3 years ago by Luke Plant <L.Plant.98@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [2ba4278cb38f1346d70cf427bbeac71a4d1dc5ad]:

Fixed #18484 - 'display:none' on CSRF token div is redundant and causes problems with some browsers

Thanks to hedleyroos for the report

comment:3 follow-up: Changed 3 years ago by simonb

  • Resolution fixed deleted
  • Status changed from closed to reopened

By removing style="display:none" the layout of every form that uses csrf_token has been changed. The div becomes significant to the layout/flow of the form and moves everything that follows below it - divs are blocks. This is a backwards incompatible change. Could we, at least, give the surrounding div an id or class so we can address it in CSS to fix the layout of all of our forms?

Or get rid of the div completely?

Thanks

Last edited 3 years ago by simonb (previous) (diff)

comment:4 Changed 3 years ago by hedleyroos@…

I vote for removing the div completely.

comment:5 Changed 3 years ago by aaugustin

  • Severity changed from Normal to Release blocker

Marking as a release blocker since the current code might trigger regressions.

comment:6 in reply to: ↑ 1 Changed 3 years ago by claudep

Replying to lukeplant:

You cannot put the input in without a div due to HTML validity constraints.

Placing an input directly inside a form element is not valid in HTML 4/XHTML Strict DTDs. It is accepted in the respective Transitional versions or in HTML5. Do we guarantee Strict conformance in Django?

comment:7 Changed 3 years ago by simonb

  • Cc bnomis@… added
  • Component changed from Uncategorized to Forms

comment:8 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by lukeplant

Replying to simonb:

By removing style="display:none" the layout of every form that uses csrf_token has been changed. The div becomes significant to the layout/flow of the form and moves everything that follows below it - divs are blocks.

Could you give an example of a layout where this happens? (You could use jsfiddle e.g. http://jsfiddle.net/abXgB/ ) That would help us assess whether this is badly designed or fairly unlikely CSS.

Could we, at least, give the surrounding div an id or class so we can address it in CSS to fix the layout of all of our forms?

That sounds fine to me. It may be the easiest option for us given the other constraints.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by simonb

Replying to lukeplant:

Replying to simonb:

By removing style="display:none" the layout of every form that uses csrf_token has been changed. The div becomes significant to the layout/flow of the form and moves everything that follows below it - divs are blocks.

Could you give an example of a layout where this happens? (You could use jsfiddle e.g. http://jsfiddle.net/abXgB/ ) That would help us assess whether this is badly designed or fairly unlikely CSS.

Inline forms

http://jsfiddle.net/simonb/r9nTK/

comment:10 in reply to: ↑ 9 Changed 3 years ago by lukeplant

Replying to simonb:

Inline forms

Thanks. That seems like a pretty common/simple use case, and it does validate the original choice to make the div 'display:none;'. I don't know what we do with the broken browsers that decide not to submit those inputs.

I'll take this to django-devs.

comment:11 Changed 3 years ago by lukeplant

Composing the email brought me to a decision: we already ship HTML5 templates with Django in 1.4. We promised functional backwards compatibility with major browsers, but not necessarily validity for HTML4/XHTML. Our direction has been made clear.

If people are really worried about HTML4/XHTML validity, which seems less likely these days, they can always implement their own {% csrf_token %} tag.

So, let's remove the <div> altogether.

comment:12 Changed 3 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

In fa2e28ccc45d383ad9b1398565a9d106a80fd1db:

Fixed #18484 -- Removed the div around the csrf token input

comment:13 Changed 3 years ago by Claude Paroz <claude@…>

In e6f45aa623d9a67a2d6389665ca1bea0556dc832:

Added release note about removed div around csrf token

Refs #18484. Thanks Simon Charette for the suggestion.

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