Html::cleanCssIdentifier Strips Valid Escaped Characters

@PCate Please provide a patch with ideally test coverage as well. I just had a look and your comment makes totally sense to be in line with the w3c standard: https://www.w3.org/TR/CSS21/syndata.html#characters

I can confirm, this is also an issue for the UIKit framework / theme. Responsive classes are also addressed with @ (U+0040).

So I added a patch, which covers the "@ sign", use by many CSS frameworks in responsive breakpoint selectors, and the "slash", used by some CSS frameworks as grid element width selector.

While this is still opinionated, as most characters will work as escaped character in a Stylesheet file, while there is no need to escape those in a tag attribute.

The issue isn't with the slash and the '@' sign, specifically. It's with escaped characters more generally. There's a sister issue at #3050007: CSS identifiers created via `Html::cleanCssIdentifier` shouldn't strip colons where they're solving for colons (but not for slash or '@' sign).

Is this being worked on by anyone? Drupal8 is currently not compatible with Tailwindcss classes due to this problem -- specifically because it uses "/" in class identifiers. Therefore, these classes can't be used in Views configs and similar. I'd like to fix this somehow.

I'm very confused about how the submitted patches actually worked, since there's a line above the patch-modded lines which uses the default filter (which specifies a "/" should be changed to "-") to modify the identifier.

Perhaps the testers only cared about "@" et al, and not "/"?

I agree we should wrap any of the related issues into a single issue and handle them together.

I hadn't thought about the BC aspect, but that will be the challenging part. Here's the risk: we change how ::cleanCssIdentifier() processes strings and one or more class names cleaned with this method are changed. The result would be CSS regressions on sites.

Here's an alternative to adding a flag: Deprecate ::cleanCssIdentifier() and add a new method. The problem with that approach is that ::cleanCssIdentifier() is used all over core. This solution helps with contrib but not with core.

Adding a flag in config might be the way to go.

This merges in the fine work from #3050007: CSS identifiers created via `Html::cleanCssIdentifier` shouldn't strip colons.

Adding U+002E for the period character supports margin in Tailwind.

[^\x{002D}\x{002E}\x{002F}\x{003A}\x{0030}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-\x{FFFF}]

https://www.phpliveregex.com/p/yxz

Working on a patch and test fixes.

Here is a patch and I fixed the DisplayBlockTest::testBlockEmptyRendering rendering test. But it seems like supporting : and . in class names will cause backwards compatibility problems in class names.

Any chance someone might roll a patch for D8? It's a problem there, too, and the latest patch won't apply.

Re-rolled patch. Please review.

Re-roll. Conflicts where in the test class.

I cannot use underscore in views CSS classes; it gets replaced by a hyphen.

It goes through Html::cleanCssIdentifier(...). The underscore is removed by the str_replace which uses the $filter character list (which contains the underscore by default), even before the preg_replace processes the identifier.

  public static function cleanCssIdentifier($identifier, array $filter = [
    ' ' => '-',
    '_' => '-',
    '/' => '-',
    '[' => '-',
    ']' => '',
  ]) {
    // We could also use strtr() here but its much slower than str_replace(). In
    // order to keep '__' to stay '__' we first replace it with a different
    // placeholder after checking that it is not defined as a filter.
    $double_underscore_replacements = 0;
    if (!isset($filter['__'])) {
      $identifier = str_replace('__', '##', $identifier, $double_underscore_replacements);
    }
    $identifier = str_replace(array_keys($filter), array_values($filter), $identifier);
    // Replace temporary placeholder '##' with '__' only if the original
    // $identifier contained '__'.
    if ($double_underscore_replacements > 0) {
      $identifier = str_replace('##', '__', $identifier);
    }

The line with

$identifier = str_replace(array_keys($filter), array_values($filter), $identifier);

converts valid underscore into hyphens.

If I remove the default underscore value in the argument, it works.

  public static function cleanCssIdentifier($identifier, array $filter = [
    ' ' => '-',
//    '_' => '-',
    '/' => '-',
    '[' => '-',
    ']' => '',
  ]) {

I understand this change is prone to create unexpected results, especially when existing theme CSS classes were based on the previous behaviour. Alas, I don't have any idea on how views can customize the filter variable.

And with the previous test, I have the class in both formats. Not sure why.

After further investigation, now I know why there are both formats. There is a backwards compatibility functionality in the template_preprocess_views_view which calls the cleanCssIdentifier:

// @todo https://www.drupal.org/project/drupal/issues/2977950 Decide what to
// do with the backwards compatibility layer.

cleanCssIdentifier is called in a callback passed to an array_map in views.theme.inc, which may be reworked for an additional option alongside the css_class one. So this is a views module specific processing which is not really related to this very issue.

Regarding our issue here, I was in a rush, so I decided to patch the cleanCssIdentifier method by doing a simple check on the identifier:

  public static function cleanCssIdentifier($identifier, array $filter = [
    ' ' => '-',
    '_' => '-',
    '/' => '-',
    '[' => '-',
    ']' => '',
  ]) {
    if (strpos($identifier, 'keep_underscores___') === 0) {
      unset($filter['_']);
      $identifier = substr($identifier, strlen('keep_underscores___'));
    }
...

... so that anywhere I want to keep the underscores, I only need to prefix my class with the 'keep_underscores___' string. Not the prettiest way to handle this, but it works and does not impact existing classes.

Re-roll.

This shouldn't make test failures any worse. The css selectors in the block test were already re-worked and aren't in need of updates any longer.

Incredibly important patch for anyone using the USWDS framework and many others. Thanks @heddn and others! Not sure what else is needed to get this to land, other than passing tests, but hope to see this one in a release soon! :)

Just an additional note that this has a side effect of changing how clean_class twig filter transforms variables into class names (since it uses cleanCssIdentifier). For example, we had a block which had:

{% set classes = [
  'block',
  'block-' ~ configuration.provider|clean_class,
  'block-' ~ plugin_id|clean_class,
] %}

Previously, the plugin_id had its : stripped out and the css was written and applied to that class. After applying the patch in #40, the class name changed because clean_class no longer stripped that colon out of the plugin_id. Not a huge deal, but something I hadn't anticipated and had to do a (very minor) css update to address. I would assume there could be other unexpected side effects for other places/modules that use the cleanCssIdentifier method of this class.

I am uploading a patch that can be applied to the latest stable Drupal 10 version: 10.2.4

This patch seems to work with 10.3

The patch in #46 removes the comment that mentions period:
// - the period (U+002E)

It is still present in regex, just not in comment.

IMHO, I agree that we should allow more characters in CSS classes like Tailwind CSS classes with colon, brackets, etc. But I'm concerned when we makes changes for the function cleanCssIdentifier, since this function is used to generate a string/class for use as a CSS identifier.

For example, I place a field block in layout builder, that field block has a class like block-field-block:paragraph:grid:field-column if we allow colons in cleanCssIdentifier, originally, this class is block-field-blockparagraphgridfield-column.

I wonder if generated classes should have colons, and it might impact an existing site which has many elements being styled based on the generated classes. We should think twice before applying changes for cleanCssIdentifier.

@sea2709 - That's exactly the issue. We can't change Html::cleanCssIdentifier without causing CSS bugs.

#13

I'm wondering if the best way to solve this issue is to use a configuration flag/setting similar to the $conf['allow_css_double_underscores'] setting in D7 that allowed BEM double underscore CSS selectors.

I think the behavior should be configurable. Maybe a $setting to define additional characters?

> I think the behavior should be configurable. Maybe a $setting to define additional characters?

That is a great idea!

Just like in the block_class module where it is also configurable to allow special characters in classes.

Rerolled the 10.3 patch from #46 to include the missing comment.

I believe the order of unicode characters is wrong and will result in unwanted characters being valid. The correct order should be:

$identifier = preg_replace('/[^\x{002D}\x{002E}\x{002F}\x{0030}-\x{0039}\x{003A}\x{0040}\x{0041}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-\x{FFFF}]/u', '', $identifier);

Alternately, the syntax could be shortened to the following, but then the alphanumeric ranges would not be specifically listed.
$identifier = preg_replace('/[^\x{002D}-\x{003A}\x{0040}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-\x{FFFF}]/u', '', $identifier);

Rerolled the 10.3 patch from #51 to include the correct unicode regex.

11.2 patch

Even ":" (colon) is not allowed, but it's used by TailwindCSS. For example I can't enter grid gap-4 md:grid-cols-2 xl:grid-cols-3 in the View CSS class.