Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#32 | form-item-wtf.patch | 548 bytes | RobLoach |
#18 | form-item-id.patch | 808 bytes | RobLoach |
Comments
Comment #1
liquidcms CreditAttribution: liquidcms commentedjust changing the title
Comment #2
nevets CreditAttribution: nevets commentedThe 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)?
Comment #3
liquidcms CreditAttribution: liquidcms commentedhmmm.. one of us is missing something then... in Drupal 5.2 /admin/build/modules i have:
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.
Comment #4
liquidcms CreditAttribution: liquidcms commentedmaybe 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.
Comment #5
bradlis7 CreditAttribution: bradlis7 commentedYou 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.
Comment #6
liquidcms CreditAttribution: liquidcms commentedyes, i could mod theme to add this but my point was ALL drupal form elements should have IDs.. seem to only make sense.
Comment #7
JirkaRybka CreditAttribution: JirkaRybka commentedIMO 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.
Comment #8
liquidcms CreditAttribution: liquidcms commentedNot 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.
Comment #9
bradlis7 CreditAttribution: bradlis7 commentedI'd support wrapping fields with adding an id to the div.form-item that already exists.
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.
Comment #10
bradlis7 CreditAttribution: bradlis7 commented(Better title maybe)
Comment #11
liquidcms CreditAttribution: liquidcms commentedA 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.. :)
Comment #12
eaton CreditAttribution: eaton commentedThis 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.
Comment #13
JirkaRybka CreditAttribution: JirkaRybka commentedRelated issue also discussed at http://drupal.org/node/111719 - existing ID's in forms (submit buttons for instance) being duplicate, failing XHTML validation.
Comment #14
mikeschinkel CreditAttribution: mikeschinkel commentedI 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:
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
#2
Is there really no chance to consider solving this problem for v5.x?
Comment #15
mikeschinkel CreditAttribution: mikeschinkel commentedI'm curious why you don't mention
class
as an option?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#id
s on the existing <input>s; adding#id
s 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#id
s on the existing <input>s) would be a breaking change. Here is a breaking change that would work for my needs:However, I'd advocate the example I showed in #14 for v5.x instead because it doesn't break existing code.
So adding
#id
s to <div>s is no worse an idea (using your phrase) than leaving the#id
s 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.
Comment #16
modulist CreditAttribution: modulist commentedbetter 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
could be modified to be
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!
Comment #17
xtfer CreditAttribution: xtfer commentedIs there any reason why #16 COULDN'T be added to D5?
Comment #18
RobLoachThis 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.
Comment #19
cridenour CreditAttribution: cridenour commentedPatch applied successfully. Works as expected.
Comment #20
ksenzeeThe generic <div class="form-item"> is a real problem for themers, and this patch takes care of it. Thanks Rob.
Comment #21
drummCommitted to 5.x.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #23
owahab CreditAttribution: owahab commentedHey 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.
Comment #24
owahab CreditAttribution: owahab commentedThis patch first appeared in Drupal 5.11. Pumping version number.
Comment #25
RobLoachAdding 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:
Comment #26
cridenour CreditAttribution: cridenour commentedI agree with Rob... How does this break...anything module related? Ever?
Comment #27
owahab CreditAttribution: owahab commentedHow 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.
Comment #28
RobLoachThis is the same code that was put into Drupal 6 and it works perfectly.
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.
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.
Throughout the whole issue, they were pretty much talking about chx's solution that he brought into Drupal 6.
If you're specific about what problem you're hitting with your module, we might be able to help you fix it....
Comment #29
smitty CreditAttribution: smitty commentedI think there are really problems with wrapping the form fields. I found two issues, which I described in: http://drupal.org/node/344317.
Comment #30
RobLoachThis is a closed issue. Open up a new issue for any follow ups, like #344317: HTML-Errors with wrapped form fields.
Comment #31
RobLoachThis problem still persists in Drupal 7. I ran into it with #439798: Vertical Tabs for the Node Type Form.
Comment #32
RobLoachIt seems that for some reason, we were getting rid of the ID of the wrapper..... Hmmmmmm.
Comment #33
webchickCVS 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.
Comment #34
sunThis is already discussed over here: #43493: FAPI: Add name/type as CSS class for form elements
Reverting status.
Comment #35
newswatch CreditAttribution: newswatch commentedthis fix is already there in D 5.20 which i am using. i still get this error.