Add "required" & "error" CSS classes to form rows in as_* methods

[Waylan Limberg <waylan@…>]

Currently, there is no way to identify which fields have errors or are required with CSS. The patch below adds HTML classes to the containing block element for that purpose.

Therefore, using as_tr(), a field that is both required and contains an error would begin like so:

<tr class="error required">...

I have submitted three patches for consideration as I am unsure of whether we want to to allow people creating their own as_methods (through subclassing etc) to be able to add their own classes separate from the above.

See this thread for more details: ​http://groups.google.com/group/django-developers/browse_thread/thread/fbd0c8cfbf1b819b/

The three patches do not yet have tests (or at least the existing tests have not been adjusted) and I haven't even looked at docs. Maybe when a decision is made, I'll tackle that. Or if anyone else wants to...

[Waylan Limberg <waylan@…>]

Simple solution, easy to override, but could result in <tr class="">

[Waylan Limberg <waylan@…>]

For html_class1.diff the example in the note should have been: <tr class="">. Sorry about that.

[Waylan Limberg <waylan@…>]

Simple, not over-ridable, but always clean

[Waylan Limberg <waylan@…>]

Adds another argument to _as_html() but is easy to override and clean.

[Waylan Limberg <waylan@…>]

My guess is the docs and tests only need to be updated to reflect the new output, but I haven't got that far yet.

[Simon Litchfield <simon@…>]

Hey Wayne check out ​http://code.djangoproject.com/ticket/3515 your idea is great but I needed these options on the actual fields, and to be included regardless of how I output the form. Also added css='myfield' support to all field types.

[Chris Beaven]

slightly improved, should use same logic for html_class2.diff if that is preferred

[Chris Beaven]

Perhaps this needs some sort of merge with #3515?

[boxed@…]

I strongly prefer this fix to the one suggested in #3515. There is a bug in the patch at line 122 though, it says:
class_list = html_class_list # Reset for each loop.
This does in fact not reset the list for each loop step, but results in "required" being appended over and over again. In my example code I tried I got class="required required required" in one case :P

If this is changed to class_list=[] everything is fine as far as I can see. I would very much like this patch to go into trunk.

[Simon Litchfield <simon@…>]

So why did you prefer 3512 over 3515? Personally, I've found it essential to control CSS class names on a per form element basis, otherwise you can't control sizes/layout etc and also for script handlers etc. The auto generated forms aren't any use to me either except for pre-design functionality testing.

[boxed@…]

Because with 3512 I can set styles on the label or the actual form input at will with "label .required" and "input .required" for example. With 3515 the only way to set a style on the label tag is by traversing the DOM, since only the input control got the CSS class set.

With this patch you CAN set css styles on a per form basis, just not in the model. As long as you on the form tag itself put an ID, you can individually control the styling of any item in the form with the CSS selector, for example: "#id_my_poll_form #id_title label { background: yellow; }" This will only set the color of labels inside items with the id "id_title" that are in turn inside a form "id_my_poll_form". This prevents unreasonable leakage across the MVC layers, unlike 3515 where I have to set CSS classes in the model where they really don't belong.

[Malcolm Tredinnick]

The latest patch in this sequence has a bug that would need to be fixed: using mutable objects (such as []) as default arguments to functions is wrong. They are created once at import time and exist in the module's scope (not the function body). So if you accidently change them in place, the changes just accumulate each time the function is called.

This is why the bug in comment 5 is occurring (because it is assinging, by reference, a module-level object). Pass in a default of None and check it inside the function body. If the arg is None, assign an empty list to it.

I'm still trying to make sense of all the differences between this #3515 at the moment, but I wanted to note the problem while I remember it.

[Simon Litchfield <simon@…>]

Thanks for the tip mtredinnick. I was wondering how scoping & instancing works in Python (I'm new).

Also I agree with boxed too. I've since realised I don't like the MVC leakage my CSS patch encourages. Gets a bit long winded in the CSS but that's just unavoidable MVC overhead. One suggestion though, redo 3512 at the lower level so it works for all forms not just the as_* ones, which I rarely find myself using.

[Waylan Limberg <waylan@…>]

IMO anyone who says having the class names directly on the form elements gives them more control needs to brush up on their css skills. Given the following html:

<li class="required"><label><input...></label></li>

I can do any of the following:

# style the wrapping block with
.required {...}

# style the label
.required label {...}

# style the form element
.required input {...}

They're called "Cascading Style Sheets" for a reason. Remember there is only one form element per wrapping block. I could see a problem if that wasn't the case, but it is.

With #3515's approach, if you want to style the wrapping block element (li, tr, p) or the label you can't. I understand some people may not be using the as_methods and thats why they like 3515, but if they're building their own html, then IMO they should be building their own html.

[Chris Beaven]

And actually, the bug is that class_list should be creating a copy of html_class_list.

[Simon Litchfield <simon@…>]

We're getting wires crossed - I don't think anyone here lacks CSS understanding.

1. The reason 3515's CSS control is bad is because of MVC leakage; although it's convenient, model is perhaps not the cleanest place for CSS definition
2. #id_ CSS selectors can be used to control individual field sizes, styles etc
3. Another alternative is to make a css class name filter for templates, I'll think some more and post thoughts about that later
4. 3512 is the more MVC-compliant approach but needs implementing for all output methods not just the as_ functions

Nice work people now lets finish it off and hopefully get it into trunk.

[Simon Litchfield <simon@…>]

We're getting wires crossed - I don't think anyone here lacks CSS understanding.

1. The reason 3515's CSS control is bad is because of MVC leakage; although it's convenient, model is perhaps not the cleanest place for CSS definition
2. #id_ CSS selectors can be used to control individual field sizes, styles etc
3. Another alternative is to make a css class name filter for templates, I'll think some more and post thoughts about that later
4. 3512 is the more MVC-compliant approach but needs implementing for all output methods not just the as_ functions

Nice work people now lets finish it off and hopefully get it into trunk.

[Chris Beaven]

fixing bug in previous patch, adding tests and fixing docs

[Chris Beaven]

Simon, I disagree that this specific ticket needs to implement this new behaviour for all output methods. IMO, it's ready to go.

If you're after fine-grained then you can do this yourself (by testing form[fieldname].errors and form[fieldname].field.required) or with a new filter like you suggested.

[Waylan Limberg <waylan@…>]

For those who aren't using the as_methods(), you can still check if a field is required in the template, much like you can check that a field has errors:

{% for boundfield in form %}
    {% if boundfield.errors %}
    do stuff...
    {% endif %}

    {% if boundfield.field.required %}
    do stuff...
    {% endif %}
{% endfor %}

We could always add a 'required' property to the BoundField class, but why? Here it is anyway (Allowing one to do {% if boundfield.required %} ):

def _required(self):
    "Returns True if this BoundField is required."
    return self.field.required
required = property(_required)

Whether that little patch is applied or not, those of you building your own forms can generate whatever html you want without any additional patches and that is why this patch only affects the as_methods(). Yes, if you want a class attribute on a field element itself, you have to manually build the entire field element (with boundfield.name, boundfield.data, boundfield.label etc) but isn't that the way it should be for those who want such fine grained control as to assign custom classes to individual elements in the templates?

Note: I'm posting this partially as a note to myself. I figured this all out a month ago when I wrote the patches, but promptly forgot. Following the above discussion, I dug in again only to realize I was repeating myself. D'oh.

[Waylan Limberg <waylan@…>]

... And I see SmileyChris made the same point in less words while I was typing that. Sorry for the repitition.

Nice work SmileyChris. I'd say your latest patch is ready to go as well.

Oh and Simon,

"We're getting wires crossed - I don't think anyone here lacks CSS understanding."

Your previous comment would appear otherwise:

"Personally, I've found it essential to control CSS class names on a per form element basis, otherwise you can't control sizes/layout etc"

If I misunderstood you, sorry about that, but between that comment and some in the original thread on the mailling list, I figured I'd clear things up once and for all.

[Tai Lee]

i would like to see something similar to the old css classes that were applied to actual form elements. with oldforms we had .vPasswordField, .vEmailField, .vIntegerField, etc. that was great, as it allowed you to set smaller fields for numerical data, or larger fields for email data.

in a pinch i'd accept simplified classes based on the form field type, not the model field type. e.g. .text, .button, .checkbox, .radio, and any other types of form elements specified with the input tag. this is helpful due to some browsers lacking the more specific selectors and therefore being unable to distinguish between a checkbox or a text input.

i'd also like to see an "optional" class applied to fields that are blank=True and/or null=True. if required/error/optional must go on the containing p, tr, or td tags, i'd like to see them renamed to something a little more obscure or specific. there may be many people out there who already have generic required/error classes which are used in p and table tags but are not actually related to forms. if the classes are assigned to label or actual form elements, you know it's attached to a form. otherwise there may be some collission.

[mrmachine <real dot human at mrmachine dot net>]

oh, and i agree that at least some kind of css class needs to be attached to every actual html form field tag - input, select, textarea - as these may be output individually without generating the entire p or li or tr around a label and an input, select, or textarea.

[Chris Beaven]

mrmachine, you're getting rather sidetracked from this ticket. If you want to suggest other changes (even if they are similar), please open a new ticket ;)

[Tai Lee]

done. #3898. apologies ;)

[Gary Wilson <gary.wilson@…>]

See also #3686, which aims to add the classes back to the label and input tags, which is how Django currently does things.

[Chris Beaven]

updated to latest svn

[Chris Beaven]

add updated tests for modeltests.model_forms too

[James Bennett]

Closing in favor of #3515.

[Chris Beaven]

After talking with ubernostrum, we came to the conclusion that this is not a direct duplicate

Because of their different tacts (independent approaches), I think it is best to leave them both open for a design decision even though they are attempting to solve the same problem.

#3515 is about attaching the class information directly to the HTML widget

#3512 is about attaching the class information to the HTML row (which you could then use to style the label or widget)

Tests need updating now that the newform tests have been split up. But I wouldn't bother doing that until I actually got a confirmation on this ticket or it'll just rot again.

[Simon Litchfield]

Problem with this one, is it applies only to the as_ul etc which aren't useful in real world applications.

Newforms needs built-in, object level row *and* fieldset support. Then, we can talk about CSS classing rows and fieldsets.

[Chris Beaven]

Replying to Simon Litchfield:

Problem with this one, is it applies only to the as_ul etc which aren't useful in real world applications.

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Just test form[fieldname].errors and form[fieldname].field.required

[Simon Litchfield]

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Yes, but what a mess of a template we end up with. The PHP and .NET guys would be laughing at us.

[Chris Beaven]

Replying to Simon Litchfield:

It only needs to apply to the helper methods because if you're using low level methods, then you can already do this.

Yes, but what a mess of a template we end up with. The PHP and .NET guys would be laughing at us.

If you're silly enough not to abstract it off to a helper method of your own, then they deserve to laugh ;) My point was that it's already possible. And aside from that, the current patch in #3515 shouldn't be attaching presentation information to the Field class - that's what the Widget class is for.

Anyway, I brought this up on django-dev again, let's discuss it there.

[Simon Litchfield]

Think that's my point really, users shouldn't have to abstract newforms for such basic functionality. How many hoops should a user have to go through, for such simple functionality. Without row and fieldset support, newforms is half baked.

[Marc Fargas]

I'm almost sure that the spirit of the as_* methods is, precisely to avoid to use a template to print all fields. Having to do such template to have the error & required attributes seems a bit... non-DRY at all.

Moving to accepted. But the patch still needs to be updated.

[Chris Beaven]

Ok, here's my new take. Fully backwards compatible now.

Forms have two new attributes: error_html_class and required_html_class (both set to None by default)
When you set them, your error / required rows get given the html class. Simple. Effective. Bam.

[liangent]

i need this

[Gergely Kontra]

Any info when it will be merged to django?

thanks

[Jacob]

(In [11830]) Fixed #3512: it's now possible to add CSS hooks to required/erroneous form rows. Thanks, SmileyChris.