Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_fieldset
template_preprocess_radios
template_preprocess_checkboxes
template_preprocess_input
template_preprocess_textarea
template_preprocess_form_element
template_preprocess_form_element_label

Twig Templates Modified

fieldset.html.twig
radios.html.twig
checkboxes.html.twig
input.html.twig
textarea.html.twig
form-element.html.twig
form-element-label.html.twig

Screenshots

Before:

After

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Issue tags: +FUDK
lanchez’s picture

Assigned: Unassigned » lanchez

Started to look into this.

lanchez’s picture

Status: Active » Needs review
FileSize
15.29 KB

Ok so, I moved all the classes away from form.inc and also one processing from CompositeFormElementTrait.php. There will most probably be problems with inherited classes, but I tried to be careful not to miss anything. Some elements still have hardcoded classes, but they come from element rendering php files, like for example checkboxes form-checkbox class. Also classes might be in different order so some tests might fail because of that. Lets see..

Status: Needs review » Needs work

The last submitted patch, 3: 2329901-3.patch, failed testing.

lanchez’s picture

Status: Needs work » Needs review
FileSize
15.4 KB
1.49 KB

Fixed textarea required attribute and also I had accidentally removed for example aria attributes. Now this should pass tests.

Status: Needs review » Needs work

The last submitted patch, 5: 2329901-fixed-textarea-attributes-5.patch, failed testing.

lanchez’s picture

Status: Needs work » Needs review
FileSize
16.5 KB
1011 bytes

Ok so fixed the (hopefully) last failing test.

There is a general "problem" with adding classes in twig template with addClass if there is no classes in attributes before. The classes goes always to the back of the attributes array and gets printed last. I dont know what standards say about that, but I at least would like them to be after just after possible id attribute.

hass’s picture

That looks very dirty way, but I'm new to twig. The preprocess from past allowed us to add a class without changing the template. How can this done here? I do not like to clone the twig template in my themes if i can add a class in a preprocess function. That looks like a big WTF to me.

star-szr’s picture

@hass - this doesn't prevent anyone (modules or themes) from adding classes in preprocess. It just moves the default class declarations to the template.

hass’s picture

Do we have tests for this? I just ask as D7 has several bugs and do not allow adding additional classes in several preprocess functions. See #1114398: Form element & Form element label theming is broken

star-szr’s picture

No, it's not the responsibility of this meta to add tests or fix preprocess bugs. I don't think this meta would ever get done in time if that were the case. We're just moving the class declarations around.

hass’s picture

I fear that you may fix the issue by the migration automatically... but testing the case would be good so it does not break again in D9.

lauriii’s picture

Status: Needs review » Needs work

Overally good job while working on this! We still need to work some more to get clear documentation to Twig templates.

+++ b/core/modules/system/templates/fieldset.html.twig
@@ -22,10 +23,26 @@
+    <legend{{ title_display == 'invisible' ? legend.attributes.addClass('visually-hidden') : legend.attributes }}>

I also think we should use variable instead when we have logic while adding classes

lanchez’s picture

Status: Needs work » Needs review
FileSize
16.65 KB
3.97 KB

Thanks Lauriii. Here's a patch that moves that one class definition to an twig variable and also modifies some comments.

Too bad that I have deleted by mistake a custom module that generated a nice form to check all this functionality :/

lanchez’s picture

Assigned: lanchez » Unassigned
lauriii’s picture

Status: Needs review » Needs work

I overlooked the patch and there's still few minor documentation issues left:

  1. +++ b/core/includes/form.inc
    @@ -303,35 +303,32 @@ function form_get_options($element, $key) {
    +  // Pass element's #type to template.
    
    @@ -570,27 +551,23 @@ function template_preprocess_form_element(&$variables) {
    +  // Pass element's disabled status to template.
    

    These shouldn't have apostrophe ( http://dictionary.reference.com/browse/its )

  2. +++ b/core/modules/system/templates/form-element.html.twig
    @@ -35,13 +37,30 @@
    + * - title_display: Title display setting.
    

    This could include example. Look how its done on label_display

I also created a sandbox module for testing different types of forms. The modules isn't 100% yet so It has to be pushed further later.

bfr’s picture

Lauriii, Are you aware of the Examples project? That would most likely be suitable home for your form examples.

Edit: And here's the meta issue: #1880976: [meta] Port examples (including submodules) to D9.4+

lanchez’s picture

I'll make a new patch on Saturday.

tim.plunkett’s picture

This is such a massive DX regression. Can you please leave FAPI alone?

star-szr’s picture

Needs a bit more discussion but it was brought up on IRC that fieldset.html.twig probably shouldn't have knowledge of radios and checkboxes coded into it.

lanchez’s picture

Mmkay, so do I continue pathing or should we plan something else?

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.94 KB
4.59 KB

Went a bit back with the changes. I removed the type from fieldset.html.twig and left the functionality now to CompositeFormElementTrait::preRenderCompositeFormElement.

Status: Needs review » Needs work

The last submitted patch, 22: 2329901-22.patch, failed testing.

David Hernández queued 22: 2329901-22.patch for re-testing.

David Hernández’s picture

I've reviewed it a bit and the only thing I could find is a comment that I think it needs to be updated too:

On core/includes/form.inc template_preprocess_form_element:

/*
 * Each form element is wrapped in a DIV container having the following CSS
 * classes:
 * - form-item: Generic for all form elements.
 * - form-type-#type: The internal element #type.
 * - form-item-#name: The internal form element #name (usually derived from the
 *   $form structure and set via form_builder()).
 * - form-disabled: Only set if the form element is #disabled.
 */

We can't forget the comment, shouldn't this be moved into the twig template too?

The rest looks fine to me. Anyways, leaving it as needs review, as I'm not really experienced on the frontend / banana stuff.

The last submitted patch, 22: 2329901-22.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
15.71 KB

I removed the comment now since I think there's no need for documenting that anymore because themers can easily see the classes in top of template

David Hernández’s picture

Status: Needs review » Reviewed & tested by the community

Updating it to RTBC.

Well done!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/templates/fieldset.html.twig
@@ -22,11 +22,17 @@
-    <legend{{ legend.attributes }}><span class="{{ legend_span.attributes.class }}">{{ legend.title }}{{ required }}</span></legend>
...
+    <span{{ legend_span.attributes.addClass(legend_span_classes) }}>{{ legend.title }}</span>

It looks like we are losing the required marker here? Did this variable not exist and it now does and is a bool. If so the docs in fieldset.html.twig are now wrong.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
16.24 KB

Added required class and fixed docs

lauriii’s picture

Issue tags: +sprint
lauriii’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Interdiff looks great to me, screenshots proof the functionality.

I _assume_ that the required boolean works correctly.

Back to RTBC! Thanks!

lauriii’s picture

Patch & screenshots proofs that .form-required is being added so the required boolean is working.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#29

+++ b/core/modules/system/templates/fieldset.html.twig
@@ -22,11 +21,17 @@
-<fieldset{{ attributes }}>
-  {% if legend.title is not empty or required -%}
-    {#  Always wrap fieldset legends in a SPAN for CSS positioning. #}
-    <legend{{ legend.attributes }}><span class="{{ legend_span.attributes.class }}">{{ legend.title }}{{ required }}</span></legend>
-  {%- endif %}
...
+  <legend{{ legend.attributes }}>
+    <span{{ legend_span.attributes.addClass(legend_span_classes) }}>{{ legend.title }}</span>
+  </legend>

required here is not a boolean but text that is appearing on the page. To be sure we are not breaking this it would be nice to see a required fieldset both and after this change.

Fabianx’s picture

#35: Isn't that what the screenshots provide?

alexpott’s picture

#36 but we seem to be printing required and now we are not.

lauriii’s picture

Status: Needs work » Needs review

I dont know what for the required variable was there but its empty even if the fieldset has been set required

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I think required was before referencing an empty variable, which was a bug.

Twig silently fails on undefined variables per agreement on twig team, so it was just outputting nothing ...

@alexpott: Thanks for your attention to detail!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1256191 and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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