katiefenn / parker

Stylesheet analysis tool.
Other
2.47k stars 73 forks source link

Empty comment (/**/) can cause early exit in some cases #27

Open ejschladweiler opened 10 years ago

ejschladweiler commented 10 years ago

We use an older version of jQuery UI which contains the following bit of CSS to hack some kind of fix for datepickers with IE6 and iframes.

.ui-datepicker-cover {
    display: none;
    display/**/: block;
    position: absolute;
    z-index: -1;
    top: -4px;
    left: -4px;
    width: 200px;
    height: 200px;
}

The /**/ when found inside a selector causes Parker to end without fully parsing the file. I tested both before and after properties and declarations, all causing the early end. Outside of a selector it handles the empty comment fine. Putting a space between the two asterisks also fixes the issue.

Easy enough to fix by just removing the comment, but will affect anyone using an unedited older version of jQuery UI (v. 1.8.13 in my case).

katiefenn commented 10 years ago

Hi there, thank you for raising this issue.

I checked out a copy of jQuery UI 1.8.13 (complete with the empty comment CSS hack). When running Parker against jquery.ui.datepicker.css, I was unable to recreate the problem. Here's Parker's output when run against the stylesheet:

PARKER-JS
Total Stylesheets: 1
Total Stylesheet Size: 4047
Total Rules: 42
Total Selectors: 47
Total Identifiers: 111
Total Declarations: 91
Selectors Per Rule: 1.119047619047619
Identifiers Per Selector: 2.3617021276595747
Specificity Per Selector: 20.170212765957448
Top Selector Specificity: 31
Top Selector Specificity Selector: .ui-datepicker .ui-datepicker-buttonpane button.ui-datepicker-current
Total Unique Colors: 0
Unique Colors:
Total Important Keywords: 0
Total Media Queries: 0
Media Queries:

Is the same output reported for you?

I'd like to continue looking into it if I can; could you confirm which version of Node.JS and Parker you have installed?

ejschladweiler commented 10 years ago

I experimented with this some more. Using ONLY the jquery.ui.datepicker.css file I get the same results as you posted above. If I remove the /**/ from display/**/: block; and rerun parker then my "Total Declarations" line goes up to 92.

In my CSS the datepicker stuff is splat in the middle of a huge file, so it was very obvious something wasn't right. So, more testing! I moved a chunk of CSS from the datepicker.css file to a spot BELOW the /**/ line and it looks like it ignores anything after another comment is encountered. Try the following two samples:

/* this comment is above the hacky junk and won't be a problem */
.test-selector {
    display/**/: block;
}

.test-selector2 {
    font-weight: bold;
}
.test-selector {
    display/**/: block;
}

/* this comment should cause a problem because it follows that hacky junk */
.test-selector2 {
    font-weight: bold;
}

This is on Win8.1, node 0.10.31, and parker 0.0.8.