Opened 13 years ago

Closed 13 years ago

#14710 closed (wontfix)

Form css classes for bound fields are not rendered {{ form.field_name }}

Reported by: Robert Lujo Owned by: Robert Lujo
Component: Forms Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Version 1.2 added support for form css styling with two additional form class attributes:

    error_css_class = 'error'
    required_css_class = 'required'

http://docs.djangoproject.com/en/dev/ref/forms/api/#styling-required-or-erroneous-form-rows

This is rendered in container field value e.g. when {{ form.as_ul }} is rendered then we get:

...
<li class="required error">
    <ul class="errorlist"><li>This field is required.</li></ul>
    <label for="id_name">Name:</label>
    <input class="required error" type="text" name="name" id="id_name" />
</li>

So class attributes are placed in container DOM element, in <li> in this case.

But if you want to render fields manually like explained in
http://docs.djangoproject.com/en/dev/topics/forms/#looping-over-the-form-s-fields

   {{ form.name  }}

This is rendered without class attributes:

<input type="text" name="name" id="id_name" />

I think this should be changed:

<input class="required error" type="text" name="name" id="id_name" />

In attachment you can find patch that changes this with unit-testing adapted.
Documentation is not updated.

Attachments (1)

14710_FormsCssClassRenderedInBoundField.diff (5.5 KB ) - added by Robert Lujo 13 years ago.
Patch to forms css class rendered for BoundField

Download all attachments as: .zip

Change History (9)

by Robert Lujo, 13 years ago

Patch to forms css class rendered for BoundField

comment:1 by Robert Lujo, 13 years ago

Component: UncategorizedForms
Has patch: set
milestone: 1.3
Needs documentation: set
Owner: changed from nobody to Robert Lujo
Status: newassigned

comment:2 by Robert Lujo, 13 years ago

Related tickets:

  • #13564 Provide class attributes for form fields
  • #3512 Add "required" & "error" CSS classes to form rows in as_* methods

comment:3 by Anssi Kääriäinen, 13 years ago

Triage Stage: UnreviewedDesign decision needed

If I am not mistaken, before the patch the classes were not assigned to the input tags, that is the output was like this:

<li class="required error">
    <ul class="errorlist"><li>This field is required.</li></ul>
    <label for="id_name">Name:</label>
    <input type="text" name="name" id="id_name" />
</li>

In the original report it is claimed that the input tag has the class attributes, but at least according to the test changes this is not the case.

The big question in this patch is if this is something that will be backwards incompatible. People might have css styling applying to just .error or .required, and this styling would affect the <input> tag after Django update. I think this is not acceptable, but leaving as DDN.

comment:4 by Robert Lujo, 13 years ago

Yes, originally class attribute was filled for form.field container tag (<li>),
and the patch leave this logic the same, and adds adding the class attributes
to the form.field input tag (<input>).

Yes, it is backward incompatible, but the real question is

should rendering of {{ form.field }} should have class attribute

in rendered html code (<input>) or not?

I think it should. This could be especially interesting if you consider #13564.

comment:5 by Julien Phalip, 13 years ago

milestone: 1.3
Resolution: wontfix
Status: assignedclosed

I am not convinced this is a good idea. As you've noticed, when rendering a form with as_p or as_ul, the CSS classes are never applied directly to the <input> elements but on the <p>'s or <li>'s. So to me it seems perfectly consistent and expected that the classes still aren't applied to the <input>'s when they're rendered on their own. What's more is that changing this behaviour would likely break a lot of existing websites.

Your proposal and efforts writing this patch are really appreciated but I think it is best to leave things the way they are here. #13564, however, is something really worth fixing and getting right.

comment:6 by Robert Lujo, 13 years ago

I reviewed the issue and found out that my starting point was wrong, i.e. example:

class ContactForm(forms.Form):
    error_css_class = 'error'
    required_css_class = 'required'
    name = forms.CharField(max_length=50)

print ContactForm(data={}).as_ul()

returns:

   <li class="required error">
       <ul class="errorlist"><li>This field is required.</li></ul>
       <label for="id_name">Name:</label>
       <input id="id_name" type="text" name="name" maxlength="50" />
   </li>

I stated before that it returns:

    ...
    <input class="required error" type="text" name="name" id="id_name" />
    ...

So, I was wrong and I agree with you completely.

comment:7 by vizualbod, 13 years ago

Resolution: wontfix
Status: closedreopened

The error styles need to be propagated to widgets when they're rendered on their own. This initial design omission seems fixable.

@trebor74hr: Your patch is almost there. Could we perhaps improve it to cancel out those classes from widgets when rendered within as_table or as_ul? This way we could remove redundancy and minimize backwards incompatibility issues.

Last edited 13 years ago by vizualbod (previous) (diff)

comment:8 by Julien Phalip, 13 years ago

Resolution: wontfix
Status: reopenedclosed

Replying to vizualbod:

The error styles need to be propagated to widgets when they're rendered on their own. This initial design omission seems fixable.

I don't see any reason why it needs to be so. I invite you to check out #15667 which is taking a holistic approach to customizing template rendering.

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