Opened 5 years ago

Closed 4 years ago

#14710 closed (wontfix)

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

Reported by: trebor74hr Owned by: trebor74hr
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: UI/UX:

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 trebor74hr 5 years ago.
Patch to forms css class rendered for BoundField

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by trebor74hr

Patch to forms css class rendered for BoundField

comment:1 Changed 5 years ago by trebor74hr

  • Component changed from Uncategorized to Forms
  • Has patch set
  • milestone set to 1.3
  • Needs documentation set
  • Needs tests unset
  • Owner changed from nobody to trebor74hr
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 5 years ago by trebor74hr

Related tickets:

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

comment:3 Changed 5 years ago by akaariai

  • Triage Stage changed from Unreviewed to Design 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 Changed 5 years ago by trebor74hr

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 Changed 4 years ago by julien

  • milestone 1.3 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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 Changed 4 years ago by trebor74hr

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 Changed 4 years ago by vizualbod

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I am not happy with the decision to keep the initial design omission on the grounds of backwards incompatibility.

The error styles need to be propagated to widgets when they're rendered on their own.

@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 to remove redundancy?

Version 0, edited 4 years ago by vizualbod (next)

comment:8 Changed 4 years ago by julien

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

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