apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 363 forks source link

Broken CSS if contains classes starting with dash #993

Open f-liva opened 9 years ago

f-liva commented 9 years ago

Target: <div class="block__element --modifier"></div>

Original CSS:

.block__element.\--modifier {
    outline: 1px solid red,
}

Standard CSS minified output:

.block__element.\--modifier{outline:1px solid red}

PageSpeed CSS minified output:

.block__element.--modifier{outline:1px solid red}

PageSpeed it removes the backslash before the dash and break all stylesheet.

jeffkaufman commented 9 years ago

Looking at the css spec I don't think --modifier is legal here. A class is an ident, which is -?{nmstart}{nmchar}*. That means "an optional -, followed by one of [_a-z], {nonascii}, or {escape}, followed by any combination of zero or more of [_a-z0-9-], {nonascii}, or, {escape}". In other words, classes are specifically banned from starting with -- because {nmstart} doesn't include -. And note that {escape} is defined as "either {unicode} or \\[^\r\n\f0-9a-f]", which also does not allow \-.

Are you sure this works in all browsers?

(If there's something people are doing in html/css/js that isn't to spec but is commonly used and widely supported by browsers then we should fix our minifier to work with it to avoid breaking sites.)

f-liva commented 9 years ago

Thank you for the exhaustive answer. Yes, these class names work in every browser, even the old and dated IE 7 and Safari 5. Could you consider to implement the correct handling of these class names?

jeffkaufman commented 9 years ago

Ok, sounds like this is browser behavior we have to work with.

jmarantz commented 9 years ago

If our CSS parser considers CSS to be invalid, it should not minify it and lose content. It can also black-box the lexical region with the error but minify everything else around it. Further, even in syntactically invalid CSS we have a fallback parser than can do whatever is needed to embedded URLs (e.g. absolutify them if the CSS itself is being moved).

What actually happens to the CSS in this case? If we are actually breaking it in some way then that's a bug.

-Josh

On Thu, Jul 16, 2015 at 9:21 AM, Jeff Kaufman notifications@github.com wrote:

Ok, sounds like this is browser behavior we have to work with.

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/ngx_pagespeed/issues/993#issuecomment-121953932 .

f-liva commented 9 years ago

The PageSpeed outputted CSS contains the first --class occurrence correctly escaped with .\--class and the next occurrences not escaped, and these brake the stylesheet.