I noticed this today looking at DamZ's staging site for Drupal.org on D7, which has Bartik enabled with aggregation and compression on.

This code from Bartik's style.css:

blockquote :first-child {
  display: inline;
}

Turned into this:

blockquote:first-child {
  display: inline;
}

The two selectors are completely different, so it royally messes things up. The screenshot shows what happens to this blockquote with a p tag inside. The :first-child is meant for the p, not the blockquote itself. :/

We're starting to be able use these more than ever, so this bug will have a huge impact on how we can write our CSS and it's just not an option to turn aggregation and compression off.

CommentFileSizeAuthor
#8 933006_w_test.patch1.74 KBericduran
#3 933006.patch890 bytesericduran
bad-css.png17.36 KBjacine

Comments

ericduran’s picture

ericduran commented

So the problem is in the common.inc (line 3357)

    $contents = preg_replace(
      '<\s*([@{}:;,]|\)\s|\s\()\s*>S',             // Remove whitespace around separators,
      '\1', $contents);                            // but keep space around parentheses.
    // End the file with a new line.
    $contents .= "\n";
  }

The regex patterns remove the white spaces around the ":" hmm. me != regex expert.

ericduran’s picture

ericduran commented

lol, I have no idea what I was looking at. I just checked out D7-Head and I'm so off. You can ignore the above comment.

ericduran’s picture

ericduran commented
StatusFileSize
new 933006.patch890 bytes

Ok, I'm clearly going crazy. Is time for sleep.

Here's a patch that fixes the issue. I might be cheating because I fixed it by removing the ":" in the regex pattern. I did some test and the file size didn't vary with or without the whitespace surrounding the ":". I'm also not sure if removing the white space around ":" is "safe". But I'll leave that for someone else to decided.

ericduran’s picture

ericduran commented
Status: Active » Needs review
jacine’s picture

jacine
she/her
Location New York
commented

@ericduran Awesome, thank you!

I tested the patch and it definitely fixes the problem, but I'm not qualified to properly review this. :/

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +Needs tests

If we even sneeze near these regexes, we need to expand test coverage.

Status: Needs review » Needs work

The last submitted patch, 933006.patch, failed testing.

ericduran’s picture

ericduran commented
StatusFileSize
new 933006_w_test.patch1.74 KB

Hmm, So I just spend sometime tracking this down and it seems like there's been a lot of discussion around this.

#472820: drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS
#769226: Optimize JS/CSS aggregation for front-end performance and DX

There was just to much for me to read.

The solutions are:
- we don't strip white space around the ":"
- we don't use ":" selectors alone without a "*" in front of them (which I think is a horrible idea)
- we change the regex to not replace the white space before ":".

In this patch I'm taking the 1st approach in that we don't strip white spaces around the ":".

Can someone chip in more details on which route we should take? Thanks.

This patch tries to fix the failed test.

ericduran’s picture

ericduran commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 933006_w_test.patch, failed testing.

gapple’s picture

gapple
he/they
Primary language English
commented
Status: Needs work » Closed (duplicate)

In looking around the issue queue I found a prior report: #460448: Some CSS rules are broken once CSS aggregation is enabled
Looks like this exists all the way back to D5