Updated: Comment #71
Problem/Motivation
The way a form element label is rendered is:
return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";
where $attributes['class'] is either option
or element-invisible
. theme_form_element_label() completely ignores any attributes passed in $variables['element']['#attributes']
. This makes adding classes to a form label impossible for example.
To reproduce, add this to your theme:
function mytheme_preprocess_form_element_label(&$vars) {
$vars['element']['#attributes']['class'][] = 'mycustomclass';
}
and note that the label element will not have that class.
Proposed resolution
Rewrite theme_form_element_label() in such a way that it does not ignore the attributes passed in $variables['element']['#attributes']
Remaining tasks
The patch has been re-rolled, applied and tested. It passes all system tests. There is a concern about two spots where line breaks have been inserted (to wrap within 80 columns) -> evidently this is not necessary to conform to coding standards and apparently causes it to not conform to coding standards. There are two larger concerns about the approach of the patch itself: one part (change to theme_radios) is supposed to be broken out into a separate issue; adding title_classes_array is not desired.
User interface changes
None.
API changes
Yes.
Original report by scor
The way a form element label is rendered is:
return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";
where $attributes['class'] is either option
or element-invisible
. theme_form_element_label() completely ignores any attributes passed in $variables['element']['#attributes']
. This makes adding classes to a form label impossible for example.
To reproduce, add this to your theme:
function mytheme_preprocess_form_element_label(&$vars) {
$vars['element']['#attributes']['class'][] = 'mycustomclass';
}
and note that the label element will not have that class.
Other form elements such as theme_textfield() do respect the passed in #attributes values.
2013-06-04: The patch has been re-rolled, applied and tested. It passes all system tests. There is a concern about two spots where line breaks have been inserted (to wrap within 80 columns) -> evidently this is not necessary to conform to coding standards and apparently causes it to not conform to coding standards. There are two larger concerns about the approach of the patch itself: one part (change to theme_radios) is supposed to be broken out into a separate issue; adding title_classes_array is not desired.
Comment | File | Size | Author |
---|---|---|---|
#83 | allow_class_add_from_preprocess-1114398-82.patch | 9.65 KB | prajaankit |
#81 | allow_class_add_from_preprocess-1114398-81.patch | 9.73 KB | prajaankit |
#80 | allow_class_add_from_preprocess-1114398-80.patch | 9.68 KB | Vj |
#78 | 1114398.png | 112.38 KB | mikeker |
#74 | 1114398-74-d8.png | 193.14 KB | idebr |
Comments
Comment #1
mikeker CreditAttribution: mikeker commentedSeems to be a problem with theme_form_element() as well...
Comment #2
mikeker CreditAttribution: mikeker commentedAttached is a patch that makes
theme_form_element
andtheme_form_element_label
respect the class(es) set in the element's #attribute array.It also changes the way theme_radios() handles attributes to be more consistent with the other theme_* functions, using arrays rather than a string.
One unexpected side effect is that this CSS directive in /modules/user/user.css started working after the patch:
which hides the confirm password textbox on the user edit page. I'm curious why that is there in the first place...
Assuming that line was incorrect, this patch removes them, but I'm happy to reroll if there's a reason I'm not seeing that those lines should be there.
Comment #3
mikeker CreditAttribution: mikeker commented#2: 1114398-2.patch queued for re-testing.
Comment #4
mikeker CreditAttribution: mikeker commentedJust checking that this patch still passes six months later...
Comment #6
mikeker CreditAttribution: mikeker commentedGuess not... user.css has changed. Updated patch coming soon.
Comment #7
tgf CreditAttribution: tgf commentedI ran into this problem trying to find a way to format #tree structures like tables using CSS. This was needed to have a CSS class representing the "column" of fields as opposed to the specific enumerated field. For example:
form-item-parent-0-field (current)
form-item-field (needed)
I haven't checked if #attributes works on a fieldset, but it might be needed there also.
Comment #8
mikeker CreditAttribution: mikeker commentedYikes! How slow am I?
Updated patch attached. And this could probably use some tests. I'll look into that the next time I get some free time. Not promising anything like "coming soon" this time! :)
Comment #9
marcvangendNice work, but I don't think it's perfect yet. I just did a quick test, applying the patch to Drupal 7.10 and then building the following form element:
It produces this:
I think it doesn't make sense that the 'search-bar' class is now applied to the
div
,label
andinput
elements. I would love to have a way to add a class to just thediv
, nothing more.Comment #10
hass CreditAttribution: hass commentedInheriting the label class is no good idea. We need to change more of the array. e.g.
$element['label']['#attributes']['class'][]
and only use this for the label or we betterunset($element['#attributes']['class'])
before it get's passed totheme('form_element_label', $variables)
. Current logic is very bad as it cannot themed.I came here mostly because of theme_form_element() theming bug, but need to get this fixed, too.
Comment #11
hass CreditAttribution: hass commentedtheme_preprocess_form_required_marker() is also broken and does not allow adding other classes.
Comment #12
hass CreditAttribution: hass commentedComment #13
hass CreditAttribution: hass commentedFound on more bug
template_preprocess_block()
does not allow me to add more content classes$variables['content_attributes_array']['class'][]
.EDIT: template_preprocess_block() bug is a duplicate of #1286532: Fix $content_attributes_array does not work for block default template and got a won't fix for D7. :-(
Comment #14
Willysino CreditAttribution: Willysino commented#8: 1114398-8.patch queued for re-testing.
Sorry I want download patch how say the video "http://a3.video4.blip.tv/0460000476680/Add1sun-ApplyingPatchesToCoreDrup..." for install it, and I wrong send this to re-testing.
Sorry
Comment #15
hass CreditAttribution: hass commentedNew patch attached.
Fixes included:
'form-label'
has been added to make sure the class attribute is not added without any classes.Comment #16
hass CreditAttribution: hass commentedSame patch with changed paths applies to D8.
Comment #18
hass CreditAttribution: hass commentedFound a critical side effect with patch #8, see #1524782: Password confirmation field is missing in create user form. Need to investigate further.
Comment #19
hass CreditAttribution: hass commentedComment #20
hass CreditAttribution: hass commentedSo I guess I nailed it out now... let's see what the bot thinks.
form_process_password_confirm()
and this fails without the patch, too. Now with the patch, this works correctly and caused the confusion to the css selectors used in user.js. It is required to change the CSS classdiv.password-confirm
todiv.password-confirm-title
. This may theoretically break themes, but it makes CSS naming consistent with other class names used in core. We need to note the CSS class name change in release notes.visibility: hidden;
that have been removed by the patches above was an invalid change and has been rolled back. The correct change is the CSS class name change todiv.password-confirm-title
.#title_classes_array
is a new array to allow module maintainers to add custom classes to a form element label. It was required to implement this with the patch as we can otherwise only unset the css classes send toform_element_label
. Therefore this is a feature we get for free and a reasonable solution.Comment #21
hass CreditAttribution: hass commentedFixed one mistake.
Comment #22
hass CreditAttribution: hass commentedMissed to add the required bartik theme changes to the patch.
Comment #24
hass CreditAttribution: hass commentedD7 patch attached. This fixes the buggy form label tests.
Comment #25
hass CreditAttribution: hass commentedD8 patch attached. I cannot release my YAML for Drupal themes as final without this patch.
Can I get a review and commit, please?
Comment #26
hass CreditAttribution: hass commentedComment #27
hass CreditAttribution: hass commentedFound one side effect of the bugfix #1549626: Custom node Summary collapsed by default
Comment #28
hass CreditAttribution: hass commentedFix for #1549626: Custom node Summary collapsed by default
Comment #29
hass CreditAttribution: hass commentedComment #30
hass CreditAttribution: hass commentedRe-role for latest D7 changes.
Comment #31
hass CreditAttribution: hass commentedRe-role for latest D8 changes.
Comment #32
hass CreditAttribution: hass commentedComment #33
tim.plunkettThis tag is usually reserved for major or critical bugs, to keep D7 inline with D8, or to fix regressions.
Comment #34
hass CreditAttribution: hass commentedThis is a major bug with no possible workaround, too.
Comment #35
tim.plunkett#31: 1114398_form_element_theming_bugs_2012052001-D8.patch queued for re-testing.
Comment #37
tim.plunkettSee attached for a failing test. Interdiff included.
Comment #38
hass CreditAttribution: hass commentedNew followup bug found in #1704590: Field item ordering changed randomly after drag & dropped form is saved/previewed. D7 Core really suxxx. Reason for the break is currently unknown. Investigating the issue.
Comment #39
hass CreditAttribution: hass commentedComment #40
hass CreditAttribution: hass commentedBugs is inside
theme_file_widget_multiple()
or somewhere in the rendering of these weight form elements that adds the form #id as class to the surrounding DIV and the selectbox. As the tabledrag tries to find the class it fails as the class is not unique.Comment #41
hass CreditAttribution: hass commentedUpdated patch for 7.15 and with the tabledrag bug fixed.
@tim: Could you give me a hint if the tests you have written are back-portable, please? At least two files have failing hunks in D7 and I'm currently not sure in what files this tests need to be added, but I have not spend a reasonable time as I just need to fix the D7 installation asap.
Comment #42
hass CreditAttribution: hass commentedD8 re-role of #37 with tabledrag bugfix.
Interdiff:
Comment #43
hass CreditAttribution: hass commentedComment #44
hass CreditAttribution: hass commentedComment #45
hass CreditAttribution: hass commentedComment #46
star-szr#42: 1114398_form_element_theming_bugs_2012080101-D8.patch queued for re-testing.
Comment #48
star-szrTagging for reroll.
Comment #49
jastraat CreditAttribution: jastraat commentedRerolled.
Comment #50
star-szrThanks @jastraat! Some tabs got introduced in the patch, please remove per http://drupal.org/coding-standards#indenting.
(tabs spotted with Dreditor)
Comment #51
jastraat CreditAttribution: jastraat commentedWow. I can't believe I've been doing without Dreditor. This changes everything. Thank you so much for mentioning it!
Comment #52
star-szrRE: Dreditor - awesome :)
Just reviewed the reroll (diffed the two patches), looks good. This is the only hunk that may be off, before the reroll it was prepend() and only changed the class, so I think the reroll should use append() and just change the class.
Comment #53
hass CreditAttribution: hass commentedComment #54
jastraat CreditAttribution: jastraat commentedAlrighty - changed the patch to use append with just the new "password-confirm-title" class.
Comment #55
benjifisher#54: form_element_theming_D8-1114398-7253944.patch queued for re-testing.
Comment #56
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #58
vegantriathleteI believe this is awaiting a reroll. I will work on it.
Comment #59
vegantriathleteI manually rerolled the patch and it's failing when I run the testing framework locally.
I need to take off for a flight right now.
So, where things stand is that someone needs to download this broken patch and see if it applies. If it does, test it locally and see if you can figure out where I have something messed up. Alternatively, you might compare my patch file to the original patch I was rerolling.
Comment #60
vegantriathleteI'm taking myself off the assigned for right now. If noone has grabbed it I will see if I can come back later.
Comment #61
hass CreditAttribution: hass commentedComment #62
vegantriathleteI am back and am taking another look at this right now.
Comment #63
vegantriathleteOoops! I forgot to assign it to myself.
Comment #64
vegantriathleteI found the errors in my patch and corrected the. I successfully ran the Form API testing. The Node testing just seemed to get stuck after finishing the first (of 38) tests; it reported that it ran successfully without any exceptions, but the second test never finished.
I am attaching the patch here and I will reset my code so that I can verify the issue, check that the patch applies cleanly and check that the patch fixes the issue. In the meantime, let's see if the test completes successfully through the test bot.
Comment #65
vegantriathleteYay! The patch applies cleanly. I have verified that it fixes the issue by hacking bartik with the above preprocess function.
Let's see if it passes all the tests and then get somebody else to apply the patch and test it.
Comment #66
hass CreditAttribution: hass commentedAlso see form_process_password_confirm() in core. Without patch broken, with patch fixed. Not sure if I added this to the previous tests. Noted it in #20. there are a lot more examples in core if we search for them :-)
Comment #67
star-szrReroll looks pretty good, thanks @vegantriathlete! Some code lines were wrapped that should not have been:
Comment #68
chx CreditAttribution: chx commented@vegantriathlete, thanks for working on this patch and getting it to pass.
There are a few problems with the approach. One, the change in theme_radios() -- which I think is correct -- needs to be moved to its own issue because it is a separate issue. Any tests as applicable needs to be moved too and if there are no applicable tests then we need to write tests which show why the change is necessary.
Two, we have removed classes_array from Drupal 8 and so adding a title_classes_array again is not something we would like to do. We need to find a better solution.
Comment #69
vegantriathlete@Cottser: I wrapped the lines so that they would conform to the coding standard of fitting within 80 columns. Since these were new lines added by the original patch I thought it was okay to correct them.
@chx: I copied the original patch as it was written (with exception of correcting for coding standards as mentioned above). I haven't actually analyzed any of the logic. If we need to change how the patch was written and / or write new tests, then we'll need to pull somebody else onto the task to invest the effort to rethink it.
Comment #70
tim.plunkettOur 80 character limit is only for comments. Code lines should only be wrapped for legibility, which in these cases is not the case.
Comment #70.0
tim.plunkettupdated issue summary
Comment #70.1
vegantriathleteupdate issue summary
Comment #71
lz1irq CreditAttribution: lz1irq commentedComment #72
YesCT CreditAttribution: YesCT commentedNeeds issue summary update tag removed since @lz1irq did it recently. Thanks.
Comment #73
jhedstromIf this is still an issue, patch needs a reroll.
Comment #74
idebr CreditAttribution: idebr commentedThis appears to have been fixed in HEAD in the conversion to Twig in issue #2152213: Convert theme_form_element() to Twig
The code
results in the following markup:
Still needs work for D7 though.
Comment #75
idebr CreditAttribution: idebr commentedUpdated the target version to 7.x
Comment #76
hass CreditAttribution: hass commented@Idebr: that is incorrect i think. Try add a class to the sourounding element div only and a different one to label. Your screenshot may only show what is broken. I need to verify all again. See my comments #20 and later, please.
Comment #77
idebr CreditAttribution: idebr commented@hass My bad, sorry :). Would you mind having a look at the issue summary to show the actual problem, so other people don't make the same mistake?
Comment #78
mikeker CreditAttribution: mikeker commented@hass: I believe this has been fixed in 8.x -- likely in the Twig conversion. Please set back to 8.x if I wasn't understanding what you meant in #76.
To test this, I added the following to bartik.theme:
And the result is:
HTML of the form element, because I prefer to read code than images... :)
Comment #79
MustangGB CreditAttribution: MustangGB commentedCan confirm it's still an issue in D7.
Comment #80
Vj CreditAttribution: Vj as a volunteer commentedWorked on patch from previous comments. Tested patch via manual testing. Lable & form wrapper class working properly.
Comment #81
prajaankit CreditAttribution: prajaankit commentedChange some elementary errors in this patch
Comment #83
prajaankit CreditAttribution: prajaankit commentedComment #84
prajaankit CreditAttribution: prajaankit commentedComment #86
prajaankit CreditAttribution: prajaankit commentedComment #87
prajaankit CreditAttribution: prajaankit commentedComment #88
prajaankit CreditAttribution: prajaankit commentedComment #89
prajaankit CreditAttribution: prajaankit at ]init[ AG commentedI correct the patch #80, please review it
Comment #90
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedLatest patch applies cleanly...
Comment #91
joelpittetNeeds work for the review @Cottser gave in #52. There can't be a CSS class change as that will definitely break themes.
Comment #92
torotil CreditAttribution: torotil at more onion commentedI have just released a new module Theming that overrides these two core theme functions with something more sensible. Whoever else runs into problems with the limited extensibility of the core functios can try using it. ;-)