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.

CommentFileSizeAuthor
#83 allow_class_add_from_preprocess-1114398-82.patch9.65 KBprajaankit
#81 allow_class_add_from_preprocess-1114398-81.patch9.73 KBprajaankit
#80 allow_class_add_from_preprocess-1114398-80.patch9.68 KBVj
#78 1114398.png112.38 KBmikeker
#74 1114398-74-d8.png193.14 KBidebr
#65 form-element-label-theme-patched.png124.2 KBvegantriathlete
#64 form_element_theming_D8-reroll-1114398-64.patch13.4 KBvegantriathlete
#59 form-element-theming-reroll-1114398-59-do-not-test.patch13.4 KBvegantriathlete
#54 form_element_theming_D8-1114398-7253944.patch13.42 KBjastraat
#51 form_element_theming_D8-1114398-7236664.patch13.42 KBjastraat
#49 form_element_theming_D8-1114398-7236664.patch13.42 KBjastraat
#41 1114398_form_element_theming_bugs_2012080101-D7.patch11.41 KBhass
#42 1114398_form_element_theming_bugs_2012080101-D8.patch12.95 KBhass
#37 drupal-1114398-37-fail.patch1.77 KBtim.plunkett
#37 drupal-1114398-37.patch12.85 KBtim.plunkett
#37 interdiff.txt2.35 KBtim.plunkett
#31 1114398_form_element_theming_bugs_2012052001-D8.patch11.11 KBhass
#30 1114398_form_element_theming_bugs_2012050401-D7.patch11.24 KBhass
#29 1114398_form_element_theming_bugs_2012042801-D7.patch11.24 KBhass
#28 1114398_form_element_theming_bugs_2012042801-D8.patch11.15 KBhass
#24 1114398_form_element_theming_bugs_2012041501.patch10.38 KBhass
#25 1114398_form_element_theming_bugs_2012041501-D8.patch10.26 KBhass
#15 1114398_form_element_theming_bugs_20120301901.patch2.7 KBhass
#22 1114398_form_element_theming_bugs_2012041404.patch5.75 KBhass
#21 1114398_form_element_theming_bugs_2012041403.patch5.28 KBhass
#20 1114398_form_element_theming_bugs_2012041402.patch5.3 KBhass
#16 1114398_form_element_theming_bugs_20120301901_D8-do-not-test.patch2.74 KBhass
#8 1114398-8.patch2.42 KBmikeker
#2 1114398-2.patch2.43 KBmikeker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeker’s picture

Title: theme_form_element_label() overrides any custom #attributes » theme_form_element() and theme_form_element_label() overrides any custom #attributes

Seems to be a problem with theme_form_element() as well...

mikeker’s picture

Status: Active » Needs review
FileSize
2.43 KB

Attached is a patch that makes theme_form_element and theme_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:

div.password-confirm {
  visibility: hidden;
}

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.

mikeker’s picture

#2: 1114398-2.patch queued for re-testing.

mikeker’s picture

Just checking that this patch still passes six months later...

Status: Needs review » Needs work

The last submitted patch, 1114398-2.patch, failed testing.

mikeker’s picture

Guess not... user.css has changed. Updated patch coming soon.

tgf’s picture

I 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.

mikeker’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.42 KB

Yikes! 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! :)

marcvangend’s picture

Nice 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:

  $form['search'] = array(
    'keywords' => array(
      '#attributes' => array('class' => array('search-bar')),
      '#type' => 'textfield',
      '#title' => t('Search'),
    ),
  );

It produces this:

<div class="form-item form-type-textfield form-item-keywords search-bar">
  <label for="edit-keywords" class="search-bar">Search </label>
  <input type="text" maxlength="128" value="" name="keywords" id="edit-keywords" class="search-bar form-text">
</div>

I think it doesn't make sense that the 'search-bar' class is now applied to the div, label and input elements. I would love to have a way to add a class to just the div, nothing more.

hass’s picture

Inheriting 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 better unset($element['#attributes']['class']) before it get's passed to theme('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.

hass’s picture

Status: Needs review » Needs work

theme_preprocess_form_required_marker() is also broken and does not allow adding other classes.

hass’s picture

hass’s picture

Status: Needs work » Needs review

Found 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. :-(

Willysino’s picture

#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

hass’s picture

Assigned: » Unassigned
Status: Needs work » Needs review
FileSize
2.7 KB

New patch attached.

Fixes included:

  • Form item classes are no longer inherited to the item label classes.
  • theme_preprocess_form_element_label() is now able to add classes to the label.
  • Class 'form-label' has been added to make sure the class attribute is not added without any classes.
hass’s picture

Same patch with changed paths applies to D8.

Status: Needs review » Needs work

The last submitted patch, 1114398_form_element_theming_bugs_20120301901.patch, failed testing.

hass’s picture

Found a critical side effect with patch #8, see #1524782: Password confirmation field is missing in create user form. Need to investigate further.

hass’s picture

Assigned: Unassigned »
hass’s picture

Status: Needs work » Needs review
FileSize
5.3 KB

So I guess I nailed it out now... let's see what the bot thinks.

  • Core also tries to add a classes to the password fields in 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 class div.password-confirm to div.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.
  • The 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 to div.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 to form_element_label. Therefore this is a feature we get for free and a reasonable solution.
hass’s picture

Fixed one mistake.

hass’s picture

Missed to add the required bartik theme changes to the patch.

Status: Needs review » Needs work

The last submitted patch, 1114398_form_element_theming_bugs_2012041404.patch, failed testing.

hass’s picture

Version: 8.x-dev » 7.x-dev
FileSize
10.38 KB

D7 patch attached. This fixes the buggy form label tests.

hass’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned »
FileSize
10.26 KB

D8 patch attached. I cannot release my YAML for Drupal themes as final without this patch.

Can I get a review and commit, please?

hass’s picture

Title: theme_form_element() and theme_form_element_label() overrides any custom #attributes » Form element & Form element label theming is broken
Version: 7.x-dev » 8.x-dev
hass’s picture

Found one side effect of the bugfix #1549626: Custom node Summary collapsed by default

hass’s picture

hass’s picture

Version: 8.x-dev » 7.x-dev
FileSize
11.24 KB
hass’s picture

Re-role for latest D7 changes.

hass’s picture

Version: 7.x-dev » 8.x-dev
FileSize
11.11 KB

Re-role for latest D8 changes.

hass’s picture

Issue tags: +7.15 release blocker
tim.plunkett’s picture

Issue tags: -7.15 release blocker

This tag is usually reserved for major or critical bugs, to keep D7 inline with D8, or to fix regressions.

hass’s picture

This is a major bug with no possible workaround, too.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, 1114398_form_element_theming_bugs_2012052001-D8.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.35 KB
12.85 KB
1.77 KB

See attached for a failing test. Interdiff included.

hass’s picture

Status: Needs work » Needs review

New 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.

hass’s picture

Status: Needs review » Needs work
hass’s picture

Status: Needs review » Needs work

Bugs 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.

hass’s picture

Version: 8.x-dev » 7.x-dev
FileSize
11.41 KB

Updated 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.

hass’s picture

Status: Needs work » Needs review
FileSize
12.95 KB

D8 re-role of #37 with tabledrag bugfix.

Interdiff:

-  var targetClass = '.' + rowSettings.target;
+  var targetClass = '.form-item .' + rowSettings.target;
   var targetElement = $changedRow.find(targetClass).get(0);
hass’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
hass’s picture

Priority: Normal » Major
hass’s picture

Assigned: » Unassigned
star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1114398_form_element_theming_bugs_2012080101-D8.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

jastraat’s picture

Status: Needs work » Needs review
FileSize
13.42 KB

Rerolled.

star-szr’s picture

Issue tags: -Needs reroll

Thanks @jastraat! Some tabs got introduced in the patch, please remove per http://drupal.org/coding-standards#indenting.

(tabs spotted with Dreditor)

jastraat’s picture

Wow. I can't believe I've been doing without Dreditor. This changes everything. Thank you so much for mentioning it!

star-szr’s picture

RE: 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.

+++ b/core/modules/user/user.jsundefined
@@ -18,9 +18,9 @@ Drupal.behaviors.password = {
-      outerWrapper.find('input.password-confirm').parent().append('<div class="password-confirm">' + translate.confirmTitle + ' <span></span></div>').addClass('confirm-parent');
+      outerWrapper.find('input.password-confirm').parent().prepend('<div class="password-confirm-title">' + translate.confirmTitle + ' <span></span></div>').addClass('confirm-parent');
hass’s picture

Status: Needs review » Needs work
jastraat’s picture

Status: Needs work » Needs review
FileSize
13.42 KB

Alrighty - changed the patch to use append with just the new "password-confirm-title" class.

benjifisher’s picture

dcam’s picture

http://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.

Status: Needs review » Needs work

The last submitted patch, form_element_theming_D8-1114398-7253944.patch, failed testing.

vegantriathlete’s picture

Assigned: Unassigned » vegantriathlete

I believe this is awaiting a reroll. I will work on it.

vegantriathlete’s picture

I 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.

vegantriathlete’s picture

Assigned: vegantriathlete » Unassigned

I'm taking myself off the assigned for right now. If noone has grabbed it I will see if I can come back later.

hass’s picture

Status: Needs work » Needs review
vegantriathlete’s picture

I am back and am taking another look at this right now.

vegantriathlete’s picture

Assigned: Unassigned » vegantriathlete

Ooops! I forgot to assign it to myself.

vegantriathlete’s picture

I 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.

vegantriathlete’s picture

Assigned: vegantriathlete » Unassigned
FileSize
124.2 KB

Yay! 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.

hass’s picture

Also 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 :-)

star-szr’s picture

Status: Needs review » Needs work

Reroll looks pretty good, thanks @vegantriathlete! Some code lines were wrapped that should not have been:

+++ b/core/includes/form.incundefined
@@ -4707,8 +4708,19 @@ function theme_form_element($variables) {
+    $attributes['class'] = array_merge($attributes['class'],
+      $element['#attributes']['class']
+    );
...
+  $variables_title['element']['#attributes']['class'] =
+    $element['#title_classes_array'];

@@ -4805,14 +4817,20 @@ function theme_form_element_label($variables) {
+    $attributes['class'] = array_merge($attributes['class'],
+      $element['#attributes']['class']
+    );
chx’s picture

@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.

vegantriathlete’s picture

@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.

tim.plunkett’s picture

Our 80 character limit is only for comments. Code lines should only be wrapped for legibility, which in these cases is not the case.

tim.plunkett’s picture

Issue summary: View changes

updated issue summary

vegantriathlete’s picture

Issue summary: View changes

update issue summary

lz1irq’s picture

Issue summary: View changes
YesCT’s picture

Needs issue summary update tag removed since @lz1irq did it recently. Thanks.

idebr’s picture

Issue tags: -Needs backport to D7
FileSize
193.14 KB

This appears to have been fixed in HEAD in the conversion to Twig in issue #2152213: Convert theme_form_element() to Twig

The code

function seven_preprocess_form_element_label(&$variables) {
  $variables['attributes']['class'][] = 'mycustomclass';
}

results in the following markup:

Still needs work for D7 though.

idebr’s picture

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

Updated the target version to 7.x

hass’s picture

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

@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.

idebr’s picture

Issue tags: +Needs backport to 7.x

@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?

mikeker’s picture

Version: 8.0.x-dev » 7.x-dev
Issue tags: -Needs backport to 7.x
FileSize
112.38 KB

@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:

function bartik_preprocess_form_element(&$variables) {
  $attr = new Drupal\Core\Template\Attribute($variables['attributes']);
  $attr->addClass('test-form-element');
  $variables['attributes'] = $attr;
}

function bartik_preprocess_form_element_label(&$variables) {
  $attr = new Drupal\Core\Template\Attribute($variables['attributes']);
  $attr->addClass('test-form-element-label');
  $variables['attributes'] = $attr;
}

function bartik_preprocess_input(&$variables) {
  $attr = new Drupal\Core\Template\Attribute($variables['attributes']);
  $attr->addClass('test-input-element-inside-form-element');
  $variables['attributes'] = $attr;
}

And the result is:

HTML of the form element, because I prefer to read code than images... :)

<form action="/search/node" method="get" id="search-block-form" accept-charset="UTF-8" class="form-updated-processed" data-drupal-form-fields="edit-keys,edit-submit">
  

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'form_element' -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->
<div class="test-form-element form-item form-type-search form-item-keys form-no-label">
      

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'form_element_label' -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->
<label for="edit-keys" class="test-form-element-label visually-hidden">Search</label>
<!-- END OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->


        

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'input__search' -->
<!-- FILE NAME SUGGESTIONS:
   * input--search.html.twig
   x input.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' -->
<input title="Enter the terms you wish to search for." type="search" id="edit-keys" name="keys" value="" size="15" maxlength="128" class="form-search test-input-element-inside-form-element">

<!-- END OUTPUT from 'core/modules/system/templates/input.html.twig' -->


      </div>

<!-- END OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->



<!-- THEME DEBUG -->
<!-- THEME HOOK: 'container' -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/container.html.twig' -->
<div class="form-actions form-wrapper" id="edit-actions">

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'input__submit' -->
<!-- FILE NAME SUGGESTIONS:
   * input--submit.html.twig
   x input.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' -->
<input type="submit" id="edit-submit" name="" value="Search" class="button form-submit test-input-element-inside-form-element">

<!-- END OUTPUT from 'core/modules/system/templates/input.html.twig' -->

</div>

<!-- END OUTPUT from 'core/modules/system/templates/container.html.twig' -->


</form>
MustangGB’s picture

Can confirm it's still an issue in D7.

Vj’s picture

Status: Needs work » Needs review
FileSize
9.68 KB

Worked on patch from previous comments. Tested patch via manual testing. Lable & form wrapper class working properly.

prajaankit’s picture

Status: Needs review » Needs work

The last submitted patch, 81: allow_class_add_from_preprocess-1114398-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

prajaankit’s picture

prajaankit’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 83: allow_class_add_from_preprocess-1114398-82.patch, failed testing. View results

prajaankit’s picture

Status: Needs work » Needs review
prajaankit’s picture

Status: Needs review » Needs work
prajaankit’s picture

Status: Needs work » Needs review
prajaankit’s picture

I correct the patch #80, please review it

Manuel Garcia’s picture

Issue tags: -Needs reroll

Latest patch applies cleanly...

joelpittet’s picture

Status: Needs review » Needs work

Needs work for the review @Cottser gave in #52. There can't be a CSS class change as that will definitely break themes.

torotil’s picture

I 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. ;-)