There are various discussions posted here re: themeing CCK input forms. They all require a fair bit of template code to do this.

It would seem that a simple change to the forms.inc to wrap forms in DIVs (none at the moment) and add IDs to existing form element DIVs would be a pretty simple and very smart thing to do.

It is then a much simpler task to simply use CSS to theme the input forms.

I tried a 1 line change to the first line forms.inc in the theme_form_element() function and it did most of what is required.

Peter Lindstrom
LiquidCMS - Content Management Solution Experts

CommentFileSizeAuthor
#32 form-item-wtf.patch548 bytesRobLoach
#18 form-item-id.patch808 bytesRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

liquidcms’s picture

Title: forms and form items should have DIVs » forms and form items should have DIVs/IDs

just changing the title

nevets’s picture

The form already has an ID in Drupal 5 so not need for div really. Also the form inputs have ID's so I am wondering what else needs and ID (or a class)?

liquidcms’s picture

hmmm.. one of us is missing something then... in Drupal 5.2 /admin/build/modules i have:

<form action="/admin/build/modules/list/confirm"  method="post" id="system-modules">

<div><fieldset class=" collapsible"><legend>CCK</legend><table class="package">
 <thead><tr><th>Enabled</th><th>Name</th><th>Version</th><th>Description</th> </tr></thead>
<tbody>
 <tr class="odd"><td align="center">
    <div class="form-item"><label class="option"><input type="checkbox" name="status[cck_address]" id="edit-status-cck-address" value="cck_address"   class="form-checkbox" /> </label>
    </div>

</td><td><strong>Address Field</strong></td><td>5.x-1.x-dev</td><td class="description">Defines address field so all parts are treated as a single unit.<div class="admin-dependencies">Depends on: Content (<span class="admin-enabled">enabled</span>)</div><div class="admin-required">Required by: Address Field Canadian Support (<span class="admin-disabled">disabled</span>)</div></td> </tr>
 <tr class="even"><td align="center"><div class="form-item">
 <label class="option"><input type="checkbox" name="status[cck_address_canada]" id="edit-status-cck-address-canada" value="cck_address_canada"   class="form-checkbox" /> </label>

</div>

</td><td><strong>Address Field Canadian Support</strong></td><td>5.x-1.x-dev</td><td class="description">Adds support for Canadian Provinces.<div class="admin-dependencies">Depends on: Content (<span class="admin-enabled">enabled</span>), Address Field (<span class="admin-disabled">disabled</span>)</div></td> </tr>
 <tr class="odd"><td align="center"><div class="form-item">
 <label class="option"><input type="checkbox" name="status[computed_field]" id="edit-status-computed-field" value="computed_field"   class="form-checkbox" /> </label>
</div>

i see a lot of div's with class=form-item but they should have an ID and i see a DIV around fieldsets with no IDs and no DIV around entire form..

and this is the same with all Drupal forms - and most annoying on forms created by CCK - except it is form.inc issue to fix this.

liquidcms’s picture

maybe not the best example.. i was confusing this post with one of my posts that is somewhat similar about the admin module page - but the issue still applies here.

bradlis7’s picture

Version: 5.x-dev » 7.x-dev

You definitely won't get a new feature for drupal5. The inputs have id's, but it doesn't seem like much trouble to put an id in the div tag.

You should be able to overwrite theme_form_element() in your theme to do this.

liquidcms’s picture

yes, i could mod theme to add this but my point was ALL drupal form elements should have IDs.. seem to only make sense.

JirkaRybka’s picture

IMO Drupal's default HTML is flooded with various DIV/ID things quite enough, I have rather concerns with my site's HTML being twice the necessary size.

The 'class=form-item' point seems valid to me, but I really can't see why we should add another DIV around the form - CSS is not (yet?) required to operate on DIV's only, why can't you refer to 'form#system-modules' (perhaps making it 'display:block' if you need)?

As a sidenote, the IDs need to be ensured unique for XHTML compliance (just to remind by the way), so tend to be really long if generated properly.

liquidcms’s picture

Not sure how display block can help you here? My point was if you have an identifiable DIV around form elements - then you can style each element (i.e. including moving it around).

Hard to imagine how this could ever be a bad idea??? Except of course for your suggestion about oversized HTML code - but again, hard to imagine the performance case where you are more worried about the extra 1-2k of html code over the 100 or so sql queries that Drupal takes to create a page. But, perhaps an admin setting to "thin" the html for those who don't care about themeing their forms.

bradlis7’s picture

I'd support wrapping fields with adding an id to the div.form-item that already exists.

<div class="form-item" id="edit-status-cck-address-div"><label class="option"><input type="checkbox" name="status[cck_address]" id="edit-status-cck-address" value="cck_address"   class="form-checkbox" /> </label></div>

I made the id "edit-status-cck-address-div" to avoid having multiple IDs, which is illegal in html/xhtml. It doesn't seem that hard to do, I can come up with a patch, but I'd like to see someone's opinion who has authority w/ drupal first.

bradlis7’s picture

Title: forms and form items should have DIVs/IDs » Wrap Form Fields with ID's

(Better title maybe)

liquidcms’s picture

A patch would be great. I had already created a patch to try out my idea but would have to dig up what i had done - i don't think it was as simple as adding ID to form-item (although not sure why it wouldn't have been). Perhaps i was trying to wrap a new DIV around entire form (but that wouldn't have been enough and i did get my form layout done with CSS.

anyway it "mostly" worked - but one issue i saw was flipping Tiny in and out - it didn't replace correctly and i ended up having the Tiny window and the plain text window both showing.

.. but a patch would be good.. :)

eaton’s picture

yes, i could mod theme to add this but my point was ALL drupal form elements should have IDs.. seem to only make sense.

This is actually a very, very bad thing.

The reason is simple: the same form can appear twice on the same page. There is no inherent protection against this in FormAPI, and in fact it's an essential feature in a number of instances. Given that the same form can appear twice, the individual elements inside each form must either have sequential IDs (#form-field-title, #form-field-title-1, etc.) or use classes and surrounding selectors instead.

The first solution (sequential IDs) is unhelpful, because it would require duplicate CSS and JS code for each possible variation of an ID -- defeating the entire purpose of the ID as a unique identifier.

JirkaRybka’s picture

Related issue also discussed at http://drupal.org/node/111719 - existing ID's in forms (submit buttons for instance) being duplicate, failing XHTML validation.

mikeschinkel’s picture

I found your post while googling for the same problem ptalindstrom experienced.

I agree 1000% on this. There is currently no way to target a specific field <label> or to target a <label><input> combination with selectors. Had the authors of form.inc put the #id #id on the <div> instead of the <input> there would be no problem, but moving it from the <input> to the <div> would cause backward compatibility problems so it ideally needs to be added to the <div> as ptalindstrom suggests. Ironically it is a tiny patch to one line of code.

Here's a relatively common example that illustrates the problem, first & last name fields:

<div class="form-item">
 <label for="edit-first-name">Name, First: </label>
 <input type="text" maxlength="128" name="first_name" id="edit-first-name"  size="60" value="" class="form-text" />
</div>
<div class="form-item">
 <label for="edit-last-name">Last: </label>
 <input type="text" maxlength="128" name="last_name" id="edit-last-name"  size="60" value="" class="form-text" />
</div>

In the above example there is no way (that I am aware of) to use CSS to place the last name fields on the same line as the first name field. This seems such an obvious oversight for a common need I can't imagine that this was missed for so long. Had the theme code been generated to look like either of the following, there would be no problem (yet the first example would potentially cause backward compatibility problems for some people):

#1

<div class="form-item" id="edit-first-name">
 <label for="edit-first-name">Name, First: </label>
 <input type="text" maxlength="128" name="first_name" size="60" value="" class="form-text" />
</div>
<div class="form-item" id="edit-last-name">
 <label for="edit-last-name">Last: </label>
 <input type="text" maxlength="128" name="last_name" size="60" value="" class="form-text" />
</div>

#2

<div class="form-item" id="edit-first-name">
 <label for="edit-first-name">Name, First: </label>
 <input type="text" maxlength="128" name="first_name" id="edit-first-name"  size="60" value="" class="form-text" />
</div>
<div class="form-item" id="edit-last-name">
 <label for="edit-last-name">Last: </label>
 <input type="text" maxlength="128" name="last_name" id="edit-last-name"  size="60" value="" class="form-text" />
</div>

Is there really no chance to consider solving this problem for v5.x?

mikeschinkel’s picture

Version: 7.x-dev » 5.x-dev

The first solution (sequential IDs) is unhelpful, because it would require duplicate CSS and JS code for each possible variation of an ID -- defeating the entire purpose of the ID as a unique identifier.

I'm curious why you don't mention class as an option?

This is actually a very, very bad thing.

Actually, your comment is comparing apples and oranges.

ptalindstrom's request for #id on the <div>s is no more of a bad idea than the #ids on the existing <input>s; adding #ids to <div>s in v5.x would create no new problems.

Ultimately what I am asking for (and I think ptalindstrom is asking for) are targets for CSS selectors. The use of class instead of #id would work just fine to meet my needs and I assume the needs of ptalindstrom, although THAT (removing the #ids on the existing <input>s) would be a breaking change. Here is a breaking change that would work for my needs:

<div class="form-item edit-first-name">
 <label for="edit-first-name1">Name, First: </label>
 <input type="text" maxlength="128" name="first_name" id="edit-first-name1"  size="60" value="" class="form-text" />
</div>
<div class="form-item edit-last-name">
 <label for="edit-last-name">Last: </label>
 <input type="text" maxlength="128" name="last_name" id="edit-last-name1"  size="60" value="" class="form-text" />
</div>

However, I'd advocate the example I showed in #14 for v5.x instead because it doesn't break existing code.

So adding #ids to <div>s is no worse an idea (using your phrase) than leaving the #ids on the <input>s; either of which break validation. IOW, no new harm done, but it would fix the CSS targeting problem in v5.x.

If you want to fix the validation breaking, do it in v7.x. Validation of that sort is an abstract problem for v5.x but inability to target elements via CSS is a real tangible problem in v5.x.

modulist’s picture

better yet, add *classes* to theme form elements

In support of Peter, a really simple tweak to the theme_form_element() function in form.inc gives a themer the necessary handles to theme any CCK forms.

Where line 1540 of form.inc used to be

  $output  = '<div class="form-item">'."\n";

could be modified to be

  $output  = '<div class="form-item '.$element['#id'].'">'."\n";

This gives you a much-needed second class in your form elements that you can use for theming. I can't imagine doing any CSS-based theming without it!

xtfer’s picture

Is there any reason why #16 COULDN'T be added to D5?

RobLoach’s picture

Title: Wrap Form Fields with ID's » Wrap Form Fields with ID's (useless form-item class syndrome)
Category: feature » bug
Status: Active » Needs review
FileSize
808 bytes

This is patch is a backport of what was put in to theme_form_element for Drupal 6.... It fixes the "useless form-item class syndrome" that we all run into with Drupal 5.

cridenour’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied successfully. Works as expected.

ksenzee’s picture

The generic <div class="form-item"> is a real problem for themers, and this patch takes care of it. Thanks Rob.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

owahab’s picture

Status: Closed (fixed) » Active

Hey guys,
This change broke one of our modules and I think it might do the same to others.

As far as I am concerned, backporting this issue fixes the form-item class syndrome which has existed in the past 10 months or more. Thus, many developers have already written their code with this issue in mind. I am strongly against such a change. It looks minor but it will break many UI stuff.

Please rollback.

owahab’s picture

Version: 5.x-dev » 5.11

This patch first appeared in Drupal 5.11. Pumping version number.

RobLoach’s picture

Adding an ID to a div broke one of your modules? How does that happen? What's your module doing? Everything is working perfectly here....

It wouldn't break any modules because it's not a module-level change. This is a theme level change (theme_form_element). Even if your theme implements mytheme_form_element, it wouldn't change anything. If you're really that desperate, you can stick this in your theme to reverse the change:

function phptemplate_form_element($element, $value) {
  $output  = '<div class="form-item">'; // Don't include the div ID
  $required = !empty($element['#required']) ? '<span class="form-required" title="'. t('This field is required.') .'">*</span>' : '';

  if (!empty($element['#title'])) {
    $title = $element['#title'];
    if (!empty($element['#id'])) {
      $output .= ' <label for="'. $element['#id'] .'">'. t('!title: !required', array('!title' => filter_xss_admin($title), '!required' => $required)) ."</label>\n";
    }
    else {
      $output .= ' <label>'. t('!title: !required', array('!title' => filter_xss_admin($title), '!required' => $required)) ."</label>\n";
    }
  }

  $output .= " $value\n";

  if (!empty($element['#description'])) {
    $output .= ' <div class="description">'. $element['#description'] ."</div>\n";
  }

  $output .= "</div>\n";

  return $output;
}
cridenour’s picture

I agree with Rob... How does this break...anything module related? Ever?

owahab’s picture

How about a module that repeats a form element in the page (which is something people used to do with input type="file" for long time) and this was broken to the introduction of the form element ID? I could give you few more examples of "How does that happen? What's your module doing?". I'd suggest you re-read this issue before assuming everything works fine everywhere not just "here".

I do not really see this as a mere theme change. I haven't tested your code but I am pretty sure that there was no bug with the old form element and this issue doesn't count as a bug fix thus there is no "real" need for it to get in D5 branch.

Please rollback.

RobLoach’s picture

This is the same code that was put into Drupal 6 and it works perfectly.

How about a module that repeats a form element in the page

You're doing something wrong if you're outputting the same form item twice because then you have conflicting names. This does not mean that your modules will break though, nor does it mean the form will not function the way it's suppose to, it just means that you're HTML will not be HTML 4 Valid Strict. But that's beside the point that you're doing something wrong if you're outputting the same form twice.

......which is something people used to do with input type="file" for long time....

If you're talking about what the Upload module does with file attachments, you're sadly mistaken. The upload module correct indexes the form element names, so that when this ID gets outputted, you're left with "edit-files-upload-0-description-wrapper", "edit-files-upload-1-description-wrapper", etc. This is how you're suppose to approach the repeating form elements problem.

I'd suggest you re-read this issue before assuming everything works fine everywhere not just "here".

Throughout the whole issue, they were pretty much talking about chx's solution that he brought into Drupal 6.

I haven't tested your code...

  1. It's not my code, chx wrote it
  2. It would help to test code before claiming it doesn't work
  3. It was tested by four people before it went in
  4. If you want to test the code, take a look at the HTML output of the comment form on your site, the same code is there.

If you're specific about what problem you're hitting with your module, we might be able to help you fix it....

smitty’s picture

I think there are really problems with wrapping the form fields. I found two issues, which I described in: http://drupal.org/node/344317.

RobLoach’s picture

Status: Active » Closed (fixed)

This is a closed issue. Open up a new issue for any follow ups, like #344317: HTML-Errors with wrapped form fields.

RobLoach’s picture

Version: 5.11 » 7.x-dev
Status: Closed (fixed) » Active

This problem still persists in Drupal 7. I ran into it with #439798: Vertical Tabs for the Node Type Form.

RobLoach’s picture

Status: Active » Needs review
FileSize
548 bytes

It seems that for some reason, we were getting rid of the ID of the wrapper..... Hmmmmmm.

webchick’s picture

CVS annotate shows this was changed in the final version of the patch at #56508-17: Remove notices from form.inc. Unfortunately there are absolutely no comments along with the patch to determine why that was added. Let's get chx to look at this patch.

sun’s picture

Version: 7.x-dev » 5.x-dev
Status: Needs review » Closed (fixed)

This is already discussed over here: #43493: FAPI: Add name/type as CSS class for form elements

Reverting status.

newswatch’s picture

this fix is already there in D 5.20 which i am using. i still get this error.