Code

Opened 22 months ago

Closed 18 months ago

Last modified 18 months 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.

Attachments (0)

Change History (13)

comment:1 follow-up: Changed 22 months 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 22 months 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 19 months 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 19 months ago by simonb (previous) (diff)

comment:4 Changed 19 months ago by hedleyroos@…

I vote for removing the div completely.

comment:5 Changed 19 months 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 19 months 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 19 months ago by simonb

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

comment:8 in reply to: ↑ 3 ; follow-up: Changed 18 months 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 18 months 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 18 months 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 18 months 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 18 months 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 18 months ago by Claude Paroz <claude@…>

In e6f45aa623d9a67a2d6389665ca1bea0556dc832:

Added release note about removed div around csrf token

Refs #18484. Thanks Simon Charette for the suggestion.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.