FAPI: Add name/type as CSS class for form elements

Old topic with no feedback.

Moving to fixed.

Automatically closed -- issue fixed for two weeks with no activity.

+1

This is what we're actually doing in a per-se default override of theme_form_element() in all of our Drupal sites. Theming forms based on these classes is *a lot* easier, because you simply can't differ between div.form-item and div.form-item in CSS.

Attached patch adds the element's #name (if set) to the .form-item container (resulting in <div class="form-item elementname">), and the element's #type to the label of radio buttons and checkboxes (resulting in <label class="radio">), which is usually needed to take the special rendering/layout of single radio buttons/checkboxes into account.

Patch still applied.

The last submitted patch failed testing.

Re-test.

+1

This also allows for less tricky/more readable JS code.

From:
$('div.form-item[select]')
To:
$('.select')

The last submitted patch failed testing.

will this also work on D5 or D6? This is a great idea.

For those of us who just want to theme a form we created in using cck like a node profile for example, I would love to easily get to the element to theme instead of having to do form manipulation heavy lifting with hook form alter.

Is there a D5 or D6 equivalent?

If something like this is already available, you can contact me and let me know. I have been looking and have not found or understood as much as I should.

Thanks,
Chris

This patch additionally adds the classes from the element to the surrounding div. In combination with #342316: FAPI: Turn options and optgroups into proper #types this allows you to style the complete element (including label), e.g. a checkbox with class="default".

Imo its no necessary to set the class to the label because you can style it with div.form-item.form-radio label {} for example.

Argh, its not always good to use drupals functions (e.g. form_clean_id).
Here's a retry.

Hm, seems to be to early this morning. Here's the working patch.

This gets even more complicate. Updated some style rules and js-magic to work with the new classes.

I advanced on this in one of my latest themes. Will post a new patch shortly.

And, btw, the reason why we need to add those classes also to checkboxes/radios is:

However, my new patch probably won't need it anymore.

So here is the new patch.

- Adds the element's #type as class.

- Adds the element's #name as class.

- Changes the CSS class name for fieldset descriptions to "fieldset-description".

Reasoning:

a) When implementing a custom form layout in your theme (e.g. a two-column form layout, with floating form item labels to the left), you currently have no way to target a certain kind (#type) of form item. This patch adds the class "form-item-<#type>" to all form items. That makes it easy to specifically target all elements of type "checkboxes", or all elements of type "textarea", aso. Absolutely required for advanced theming of forms.

b) In addition, the element's #name is added as "<#name>-wrapper" class to the form item. Yes, we already introduced "edit-<#name>-wrapper" CSS ids, but CSS ids must be unique and thus can only exist once on a page. In addition, those existing CSS ids are not added to the wrapping form items for checkboxes and radios.

c) When styling forms, you have all kind of ".description" classes flowing around. However, the description of fieldsets is not really comparable to regular form item descriptions: they usually contain more lengthy guidelines and explanation for all form items contained in the fieldset. Yes, it is a description, but logically and semantically, it is intended and displayed differently than those form item descriptions (usually displayed relatively small below the form element, or rendered completely differently using JS tooltips).

.

Someone up for testing?

Updated the patch to latest HEAD, hopefully the bot doesn't get angry, out of practice.

This is a very useful patch for theming.

Oops, comment cut off somehow.

I meant to add that I did the following:
-- applied to HEAD
-- visually checked form html to see the extra classes
-- ran the Form API tests
-- validated a random form (content type creation) through w3org.

This line catches my eye:

That's pretty gruesome. Why the need for hyper-compact, unreadable code? Since this is a theme function that designers/themers (who may not be PHP-savvy) will want to override, this code should be as friendly as possible.

What if, before the return statement, we do all the conditional stuff and store things in $legend, $description, and $children, and then concatenate all that in the return statement? That would make this a lot more readable and expressive. What do people think of this approach?

@akahn: Not invented here. If we want to make those theme functions for form elements cleaner for themers, then we should tackle them all at once in a separate issue.

Point taken. Anyone think it would be worthwhile to get rid of this kind of dense, hard-to-read code from Drupal core?

@akahn, my opinion is you'd prefer not to override this theme function if you could avoid it. But I take your point.

I don't do a lot of javascript, so this patch seems overkill for my needs as a themer; however, merlinofchaos and quicksketch both suggested in IRC the idea could be useful. Hopefully they'll get a chance to look at the patch directly and add their comments. HINT, HINT, HINT. :)

I really like the change to include "form-item-[type]" to the wrapper, but I'm not so sure about the "form-item-[name]" change. For everything except checkboxes/radios, this is going to be nearly identical to the ID. Seems like we should either A) remove the ID entirely or B) add an ID to checkboxes/radios. New classes are great, redundant ones not so much.

To compromise, could we add a #wrapper_attributes property and then run drupal_attributes() on it and tack that onto the wrapper div? Then anyone interested could add back in the "form-item-[name]" class through a hook_elements() and an extra #process function. Of course its usefulness would be different than the original patch... maybe that change should be elsewhere, but still I'd like to see a different alternative that having an id and class attribute that are nearly identical.

Given that form_clean_id() will rename any ID that appears more than once on a page, no developer should rely on those IDs, but classes instead. That issue bit me just recently when I mistakenly tried to target the username field on the user login block form. However, on the path user/login, the regular user login form is rendered already, so my login block suddenly used a different ID. So neither my JS nor my CSS did apply anymore.

I've had that exact same situation happen with the login form when using LoginToboggin's fancy login form. Per your description, "no developer should rely on those IDs, but classes instead," it sounds like that's the exact reason why we shouldn't have the automatic ID at all. I'm not opposed to adding more classes, I'm opposed to having an ID and Class that are identical 90% of the time. If we add this wrapper class based on the element name, we should get rid of the ID, since it serves no functional purpose that the class cannot provide.

Yeah, I agree. Remaining question is: Tackle that duplicate ID/class in this issue or in a separate? I'm not entirely sure about how much CSS/JS we need to change accordingly for that, so I'd rather prefer to move that into a separate issue.

I'm in support of just adding the "form-item-[type]" to make life easier 90% of the time.

As long as I can avoid overriding theme_form_element().

For example, if I could over-ride individual element types, I could then simply implement phptemplate_form_element_item() or phptemplate_form_element_radios() to just sort out the weirdness that happens from design to design. Is this out of scope?

Does this still need more discussion?

The changes from removing the #edit-[name]-wrapper ID doesn't look drastic, but would clearly be dependent upon this patch. I made a separate issue in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers that contains a dependent patch for removing the ID attribute entirely from all CSS and JS, though it's not removed from the markup (yet) until this patch is committed. So let's do one step at a time, get this in and finish it in the other issue.

sime, I think restructuring the form element function calls is probably not part of this issue either. It'd be a more signficant code change, but at least after this patch the need to override it should be reduced quite a bit.

Although it's probably un-related I ran into this a little while ago. It's unsetting the ID of any conditional form element........

The last submitted patch failed testing.

Setting to rtbc - testbot was broken.

So #33 is the patch that should get committed?

#19 it is. I've sent that patch for re-testing now.

quicksketch was so kind to already create #482636: Remove "edit-[name]-wrapper" ID from form element wrappers, so #19 is ready to go.

I'm not sure what the patch of Rob Loach in #33 is for, but it sounds like a different issue.

Alright. Thanks! Committed to CVS HEAD. See you in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers. ;-)

Automatically closed -- issue fixed for 2 weeks with no activity.