Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed fix for #93 #99

Merged
merged 3 commits into from
Apr 5, 2016
Merged

Proposed fix for #93 #99

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd760a6
Ignore additional at-rules and test
Mar 29, 2016
ddb1f56
Test chunked at-rules
Mar 29, 2016
8e9f363
Updates #93. `@page` returns count = 1
Apr 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/count.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ function count(ast) {
case 'keyframes':
case 'import':
case 'supports':
case 'charset':
case 'namespace':
case 'page':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a link to any documentation from Microsoft that these selectors are not counted against you in IE9? While this may fix the bug in #93 it may mask and obscure another bug that is harder to diagnose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Matt,

Not sure what you meant if this update 'can be counted against you in IE 9' by Microsoft, did you mean if we ignore these at-rules they will break the css?

For the documentation I can only find this one, https://msdn.microsoft.com/en-us/library/aa358815(v=vs.85).aspx and they seem to support those 3 selectors.

...mask and obscure another bug...

Which bug are you referring to? Do you have a reference? Or you meant it may introduce new bugs?

For additional testing, I tested this simple code in IE 6, 7, 8, 9 and they all worked fine:

blessed1.css:

@charset "UTF-8"; /* Test @charset, Test @charset as first line */
@import url(http://fonts.googleapis.com/css?family=Roboto:300,400,700|Roboto+Slab:300,400|Lato:100,300,400,900);

@page :first {
  margin: 1in;
}

@namespace url(http://www.w3.org/1999/xhtml); /* Namespace for XHTML */
@namespace svg url(http://www.w3.org/2000/svg); /* Namespace for SVG embedded in XHTML */

body {
  background-color: red !important;
}

blessed.css

@import url('app-ie-blessed1.css?z=453');

body {
  color: green;
}

expected: Red background with Green text
actual: Red background with Green text

Is this what you needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary reason for this tool is to deal with the Selector Limit in IE9. If there's too many selectors on the page IE just stops using them. IE9 Does not throw an exception, it just leaves you with some of your styles applied. So, all Bless really does is split the selectors into multiple files so you don't surpass the selector limit per file imposed in IE9. See: http://stackoverflow.com/questions/9906794/internet-explorers-css-rules-limits and https://blogs.msdn.microsoft.com/ieinternals/2011/05/14/stylesheet-limits-in-internet-explorer/

So the question is if the @page selector is the 4096th in the css file will it actually be rendered to the page? If it will then this PR is fine, if it won't then that selector should be placed in a separate file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, not sure if you've seen this commit, makque@ddb1f56, so the expectation should be that all at-rules in the test file, test/fixtures/input/over-limit-with-at-rules.css will be chunked as expected into smaller files as specified by the IE9 and below css limits. I unfortunately am not that very familiar (not at all, hehe) with mocha so I tried to make those 2 tests files the best I could, so I may not be doing the test right.

But I made additional tests as well, in case:

  • System testing - used css, test/fixtures/input/over-limit-with-at-rules.css with gulp in a laravel site
    • Expected & Actual: app.css blessed to 2 files
    • Browser Test: Windows XP - IE 6
    • Browser Test: Windows XP - IE 7
    • Browser Test: Windows XP - IE 8
    • Browser Test: Windows 7 - IE 8
    • Browser Test: Windows 7 - IE 9

Browser test used test/fixtures/input/over-limit-with-at-rules.css with body { background-color: green; } and changed @page :first { margin: .2in;}, Expected & Actual: Page color is green, in print preview first page has .2" margin on all sides

Cheers!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of concerned with what would happen with unknown at-rules. There many at-rules that will still break things: vendor prefixed rules (@-moz-keyframes), obscure rules (@document), garbage/misspellings (@improt), or anything that the W3C cooks up next.

In these situations it would probably be best if the at-rule was ignored and a warning was issued. Crashing with a TypeError error is probably not the most user friendly way of letting people know we missed something. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got what you mean (how the switch, return 0 works). So I've tested it out below and the @page should be fine, the other rules (charset, namespace) should be fine since its conditions are like that of import.

reference: CSS Compatibility in Internet Explorer

switch type notes (IE conditions)
comment
font-face
keyframes
import should occur at the start of a style sheet, before any declarations
supports
charset (new) must be at the top of the file, 1st line (same case as import)
namespace (new) must declare an @namespace rule after any @charset or @import rules (same case as import)
page (new) IE 8+, can be declared anywhere in the file

OBJECTIVES

  • test if @page is not affected by the 4095 selector limit

TEST 1

  • single @page declaration
#test,
...,
#test { color: #900; }
@page :first {
  margin: 2in 3in;
}
@page after 4095th Selector
OS / Ver Expected Actual .
XP / IE 6 Fail Fail ⭕
XP / IE 7 Fail Fail ⭕
XP / IE 8 Pass Pass ⭕
W7 / IE 8 Pass Pass ⭕
W7 / IE 9 Pass Pass ⭕
@page as 1st in line
OS / Ver Expected Actual .
XP / IE 6 Fail Fail ⭕
XP / IE 7 Fail Fail ⭕
XP / IE 8 Pass Pass ⭕
W7 / IE 8 Pass Pass ⭕
W7 / IE 9 Pass Pass ⭕

TEST 2

  • multiple @page declaration
#test,
...,
#test { color: #900; }
@page :left {
    margin-left: 3in;
    margin-right: 1in;
}

@page :right {
    margin-left: 1in;
    margin-right: 3in;
}
@page after 4095th Selector
OS / Ver Expected Actual .
XP / IE 6 Fail Fail ⭕
XP / IE 7 Fail Fail ⭕
XP / IE 8 Pass Pass ⭕
W7 / IE 8 Pass Pass ⭕
W7 / IE 9 Pass Pass ⭕
@page as 1st in line
OS / Ver Expected Actual .
XP / IE 6 Fail Fail ⭕
XP / IE 7 Fail Fail ⭕
XP / IE 8 Pass Pass ⭕
W7 / IE 8 Pass Pass ⭕
W7 / IE 9 Pass Pass ⭕

TEST 3 (Test to Fail)

  • 4096 selectors
  • multiple @page declaration (same as Test 2 code)
@page after 4096th Selector
OS / Ver Expected Actual .
XP / IE 8 Fail Fail ⭕
W7 / IE 8 Fail Fail ⭕
W7 / IE 9 Fail Pass ❌
@page after 4100th single declaration/rule
.redClass0001 { background: red }
.redClass#### { background: red }
@page :left {
    margin-left: 3in;
    margin-right: 1in;
}

@page :right {
    margin-left: 1in;
    margin-right: 3in;
}
OS / Ver Expected Actual .
W7 / IE 9 Fail Fail ⭕

Conclusion

@page will still work if declared after the 4095th selector, but will not work if declared after 4096th selector, as expected, except for the case in Test 3 (IE9).

Hope this helps!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if that's the case then the count of selectors for @page should be 1, not 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I pushed an update to reflect that, so return 1, will count @page as 1 selector and should not be added after the 4095th selector, is that right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is the 4096th selector it should get split to another file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, ye that's what the test file (test/fixtures/input/over-limit-with-at-page.css) did.

return 0;
default:
return countRules(ast.rules);
Expand Down
97 changes: 97 additions & 0 deletions test/fixtures/input/at-rules.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
@charset "UTF-8"; /* Test @charset, Test @charset as first line */

/* Test @import */
@import 'global.css';

/* Test @namespace */
@namespace url(http://www.w3.org/1999/xhtml); /* Namespace for XHTML */

@namespace svg url(http://www.w3.org/2000/svg); /* Namespace for SVG embedded in XHTML */

/* Test @media */
@media only screen
and (min-device-width: 320px)
and (max-device-width: 480px)
and (-webkit-min-device-pixel-ratio: 2) {
.module {
width: 100%;
}
}

@media print {

}

/* Test @supports */
@supports (display: flex) {
.module {
display: flex;
}
}

@supports (display: flex) and (-webkit-appearance: checkbox) {
.module {
display: flex;
}
}

/* Test @document */
@document /* Rules for a specific page */
url(http://css-tricks.com/),
url-prefix(http://css-tricks.com/snippets/),
domain(css-tricks.com),
regexp("https:.*") {
body {
font-family: Comic Sans;
}
}

/* Test @page */
@page :first {
margin: 1in;
}

/* Test @font-face */
@font-face {
font-family: 'MyWebFont';
src: url('myfont.woff2') format('woff2'),
url('myfont.woff') format('woff');
}

/* Test @keyframes */
@keyframes pulse {
0% {
background-color: #001f3f;
}

100% {
background-color: #ff4136;
}
}

/* Test @viewport */
@viewport {
min-width: 640px;
max-width: 800px;
}

@viewport {
zoom: 0.75;
min-zoom: 0.5;
max-zoom: 0.9;
}

@viewport {
orientation: landscape;
}

/* Test @counter-style */
@counter-style circled-alpha {
system: fixed;
symbols: Ⓐ Ⓑ Ⓒ Ⓓ Ⓔ Ⓕ Ⓖ Ⓗ Ⓘ Ⓙ Ⓚ Ⓛ Ⓜ Ⓝ Ⓞ Ⓟ Ⓠ Ⓡ Ⓢ Ⓣ Ⓤ Ⓥ Ⓦ Ⓧ Ⓨ Ⓩ;
suffix: " ";
}

.items {
list-style: circled-alpha;
}
Loading