Opened 13 years ago

Closed 12 years ago

Last modified 12 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 by Luke Plant, 13 years ago

Triage Stage: UnreviewedAccepted

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 by Luke Plant <L.Plant.98@…>, 12 years ago

Resolution: fixed
Status: newclosed

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 by Simon Blanchard, 12 years ago

Resolution: fixed
Status: closedreopened

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?

Thanks

Version 0, edited 12 years ago by Simon Blanchard (next)

comment:4 by hedleyroos@…, 12 years ago

I vote for removing the div completely.

comment:5 by Aymeric Augustin, 12 years ago

Severity: NormalRelease blocker

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

in reply to:  1 comment:6 by Claude Paroz, 12 years ago

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 by Simon Blanchard, 12 years ago

Cc: bnomis@… added
Component: UncategorizedForms

in reply to:  3 ; comment:8 by Luke Plant, 12 years ago

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.

in reply to:  8 ; comment:9 by Simon Blanchard, 12 years ago

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/

in reply to:  9 comment:10 by Luke Plant, 12 years ago

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 by Luke Plant, 12 years ago

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 by Claude Paroz <claude@…>, 12 years ago

Resolution: fixed
Status: reopenedclosed

In fa2e28ccc45d383ad9b1398565a9d106a80fd1db:

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

comment:13 by Claude Paroz <claude@…>, 12 years ago

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