Missing class Civictheme unset($variables['attributes']['class'])

What are the steps to reproduce?

Tagging for summary update.

We consciously chose to remove all classes and attributes to ensure the Drupal theme is in line with UI Kit, using only CivicTheme-specific classes.

While we recognize this may be a divisive move, it's aimed at providing stable rendering and reducing CivicTheme maintenance costs.

If there's strong community demand to revert this, it's something we can consider.

There's also an "opt-out" feature in CivicTheme allowing for the deactivation of specific features. We could extend this to include class removal, but note that doing so would void CivicTheme support for any visual issues caused by non-UI Kit classes.

I would argue that the default behaviour should be "opt-in" instead of "opt-out". CivicTheme can remove the class attribute from all the elements in the preprocess hooks it implements but it should not touch the elements it has no support.

For example, CivicTheme is breaking the Facets module. When using with CivicTheme, Facets module cannot transform its facet links into checkboxes because the required class attribute is removed by CivicTheme. Facets' theme hook facets_item_list and its template facets-item-list.html.twig are not supported by CivicTheme. CivicTheme is not aware of the hook/template and does not implement the _preprocess_facets_item_list hook either. However, it still removes the class attribute from the element of the theme hook and breaks the default functionality.

As a workaround, a developer has to implement a _preprocess_facets_item_list hook with $variables['modifier_class'] = FALSE; to opt-out although Facets does not interfere with CivicTheme and CivicTheme does not support Facets.

@sonnykt
So what component would CivicTheme use for those Facets exactly? CivicTheme is a design system and only works with supported components. Any added classes by 3-rd party modules will be bringing styles that are not 100% compatible with CivicTheme and a developer would need to style them anyway.

@alex.skrypnyk It doesn't matter how the unsupported elements are rendered in default CivicTheme. It's up to the site owner to decide whether to repurpose an existing CivicTheme component, extend a component, develop a new component, or even ignore the UIKit rules and go with a custom styling. Using an "unsupported" contrib module with CivicTheme is a conscious choice and no reasonable person will blame CivicTheme for not displaying the unsupported elements correctly.

It's totally fine for the elements to not display correctly with default CivicTheme. The minimum expectation is to have an unstyled but functional element. Developers will need to style the said elements for sure but they need the functionality to work before it reaches the styling stage. However, it's unacceptable for a backend functionality to be broken because a theme strips the necessary information from the output. Backend code doesn't set the class attribute for nothing, especially when it sets js-* classes. It's even worse when the theme has no support of the said elements but it still change the element anyway. Developers don't know why the backend functionality stops working until they spend time on debugging the contrib module and reckon that the culprit is not a module bug but from a theme. The idea of developers now have to look into theme code to investigate a backend issue doesn't sound good at all.

CivicTheme doesn't just strip the class attribute. It also strips other "unsupported" attributes. This behaviour is causing issues with both core and contrib modules. Facets module can't transform its facet links into checkboxes because of a missing js- class. Views exposed text filter loses its placeholder attribute because StringFilter uses ['#attributes']['placeholder'] instead of #placeholder. Better Exposed Filter module's checkboxes filter loses their checked state with CivicTheme. I'd assume that any contrib module that relies on the attributes will be likely impacted (if not broken) with default CivicTheme.

The worst impact is standard Form API losing all meaningful and valid attributes that have nothing to do with styling:

$form['log'] = [
  '#type' => 'textfield',
  '#title' => $this->t('Log message'),
  '#size' => 30,
  '#attributes' => [
    'my-custom-attributes' => 'abc',
    'data-abc' => 1,
    'pattern' => '[A-Za-z]{3}',
    'minlength' => 10,
    'spellcheck' => 'true',
    'placeholder' => 'Enter a log message',
  ]
];

The above element is expected to have the following output with core and regular contrib themes:

<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' -->
<input my-custom-attributes="abc" data-abc="1" pattern="[A-Za-z]{3}" minlength="10" spellcheck="true" placeholder="Enter a log message" data-drupal-selector="edit-log" type="text" id="edit-log" name="log" value="" size="30" maxlength="128" class="form-text form-element form-element--type-text form-element--api-textfield" tabindex="-1">

With CivicTheme, the output is minimal and meaningless:

<!-- BEGIN OUTPUT from 'themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig' -->
<input type="text" class="ct-textfield ct-theme-light  ct-field__control form-text" name="log" id="edit-log" size="30" maxlength="128" data-abc="1" data-drupal-selector="edit-log" tabindex="-1">

@sonnykt
thank you for a detailed response. I really appreciate it.

Just a note before the reply: $variables['modifier_class'] = FALSE; is already available to preserve the stripping-out of classes, so this functionality can be disabled by a developer. If this does not work technically - this is a defect.

Conceptually

It's up to the site owner to decide whether to repurpose an existing CivicTheme component, extend a component, develop a new component, or even ignore the UIKit rules and go with a custom styling.

I agree in principle. However, "the site owner" is not always a developer - sometimes it is a script within an automation that just launches the site, sometimes it is a human evaluator without any technical skills. So what we are really talking about here is a opt-in vs opt-out. CivicTheme leans towards having a stable presentation layer out-of-the-box and only supports the components that are available in the UIKit.
The reason why this decision was made is quite simple: more than once contribs has broken the front-end by doing something that should not be doing at all! This has resulted in affecting many-many sites.

So, instead of opt-in to use contribs' classes by default and fix it per-site, we are opting-out by default (to only use CivicTheme's components) and fixing issues per site.

Facets module can't transform its facet links into checkboxes because of a missing js- class.

This is a good example. Let's look deeper.

If there was no stripping-out of classes - the functionality will not work (there is no component in CivicTheme, for example), a developer would dive into code and realise that some work needs to be done to add a support for this (new component etc.), but the site could be affected because a site builder could have accidentally misconfigured the contrib leading to "bleeding" styles.

If there is a stripping-out of classes in place (what we have now) - the functionality will not work, a developer would dive into code and realise that some work needs to be done to add a support for this (new component etc.), but the site will not be affected in any way, so there are no surprises on pages that may or may not be affected by this contrib. If the classes are really harmless for the front-end, a developer can use $variables['modifier_class'] = FALSE; to "opt-out" from stripping-out the classes.

So, it comes down to developer being surprised that a contrib does not work out-of-the-box (and needed to debug why) vs having a really solid front-end that would be affected only when a developer consciously enables the original classes.

Developer's surprise can be avoided if they read documentation (which may or may not exist at the moment of writing this, but it is clearly a fixed-scope item to write such documentation).

The worst impact is standard Form API losing all meaningful and valid attributes that have nothing to do with styling:

This looks like a defect. 1.8 has brought Styleguide and testing of form elements, including testing of passing of attributes.
The attributes passing itself was refactored to handle more cases. Maybe this is not an issue anymore.

<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' -->
<input my-custom-attributes="abc" data-abc="1" pattern="[A-Za-z]{3}" minlength="10" spellcheck="true" placeholder="Enter a log message" data-drupal-selector="edit-log" type="text" id="edit-log" name="log" value="" size="30" maxlength="128" class="form-text form-element form-element--type-text form-element--api-textfield" tabindex="-1">

The custom attributes and other attributes from for API should be rendered by CivicTheme - if they are not - it is a defect (but see above - we have fixes the attributes in 1.8).

The CSS classes though - they should not be used for anything else but presentation. And CivicTheme provides that presentation using own classes. If a contrib uses CSS to bind JS events or any other custom functionality - they are doing it wrong by occupying the space used purely for presentation. And they should fix it to use attributes rather than classes. The `js-*` is an old-school approach from times when custom attributes were not supported by browsers. It should not be used in the modern development practices.

-----

Next steps:

There is no way that CivicTheme is going to remove the stripping-out of classes altogether - as mentioned above - this may potentially break front-end leading to upset client's and their site visitors.

But we can create a more robust system of opt-in/opt-out and, potentially, a curated list of supported modules whose classes would not need to be stripped-out.

We are open to other suggestions on how to organise this better.

This is why contextual links are broken right? Not sure if you just want to fix contextual links over here 🐛 Contextual links are broken Needs work . Or stop removing attributes here. But something should be done.

@sime afaik CivicTheme has limited support for contextual links on Layout Builder only.

This looks like a defect. 1.8 has brought Styleguide and testing of form elements, including testing of passing of attributes.
The attributes passing itself was refactored to handle more cases. Maybe this is not an issue anymore.

The snippets were produced from CivicTheme 1.8.

I agree in principle. However, "the site owner" is not always a developer - sometimes it is a script within an automation that just launches the site, sometimes it is a human evaluator without any technical skills. ...

In that case, the site owner is also the person decided to use CivicTheme in the first place. When facing a technical issue e.g. whether to follow CivicTheme rules for an unsupported contrib module, a developer is always involved.

If there was no stripping-out of classes - the functionality will not work (there is no component in CivicTheme, for example), a developer would dive into code and realise that some work needs to be done to add a support for this (new component etc.), but the site could be affected because a site builder could have accidentally misconfigured the contrib leading to "bleeding" styles.

That is not true. If CivicTheme didn't strip out classes, unsupported contrib modules would always work. Their output would be just unstyled. It's the expected behaviour of all Drupal themes. Developers only need to look into the theme hook of the said module to figure out how to style the theming elements.

In reality, if a module defines its own theme element in hook_theme, it is likely broken by CivicTheme due to the stripping out of classes behaviour. However, developers won't know why the expected functionality doesn't work and they have to dig into the module code. Only debugging could reveal the ultimate culprit which is civictheme_preprocess_last().

CivicTheme implements about 50 template_preprocess_ hooks. Instead of stripping out the classes/attributes from the 50 supported theme elements (aka opt-in fashion), CivicTheme goes with the ultimate but lazy approach to remove everything with template_preprocess_last(). It doesn't care whether a theme element is supported by CivicTheme. Just remove them all.

There are thousands of contrib module out there defining their own theme elements with hook_theme(). By design, CivicTheme will likely break them all if their users (read: developers) don't write a template_preprocess_ hook to opt-out for every theme element. When a Drupal theme by design tends to break "unsupported" contrib modules, I would seriously lower the confidence factor when assessing an unsupported contrib module for a site with CivicTheme.

The CSS classes though - they should not be used for anything else but presentation. And CivicTheme provides that presentation using own classes. If a contrib uses CSS to bind JS events or any other custom functionality - they are doing it wrong by occupying the space used purely for presentation. And they should fix it to use attributes rather than classes. The `js-*` is an old-school approach from times when custom attributes were not supported by browsers. It should not be used in the modern development practices.

It doesn't matter if it's an "old-school" approach. There are thousands of contrib modules out there still relying on the "js-*" classes. Drupal core is still using "js-*" classes. It's not the job of a design system to tell core and contrib modules how to implement their functionalities. Why can other Drupal themes of other design systems work with most contrib modules but not CivicTheme?