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.

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

Menschified juice #220

Merged
merged 14 commits into from
Jun 23, 2016
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
7 changes: 6 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
language: node_js
node_js:
- "0.10"
- "6"
- "5"
- "4.1"
- "4.0"
- "0.12"
- "0.11"
- "iojs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably fine to take out iojs and 0.11 at least

- "node"
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,6 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

- Uses [cheerio](https://github.com/cheeriojs/cheerio) for the underlying DOM
representation.
- Uses [cssom](https://github.com/NV/CSSOM) to parse out CSS selectors and
- Uses [mensch](https://github.com/brettstimmerman/mensch) to parse out CSS and
[Slick](https://github.com/subtleGradient/slick) to tokenize them.
- Icon by [UnheardSounds](http://unheardsounds.deviantart.com/gallery/26536908#/d2ngozi)
27 changes: 16 additions & 11 deletions client.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,21 @@ function inlineDocument($, css, options) {
// go through the properties
function addProps(style, selector) {
for (var i = 0, l = style.length; i < l; i++) {
var name = style[i];
var value = style[name] + (options.preserveImportant && style._importants[name] ? ' !important' : '');
var prop = new utils.Property(name, value, selector, style._importants[name] ? 2 : 0);
var existing = el.styleProps[name];

// if property name is not in the excluded properties array
if (juiceClient.excludedProperties.indexOf(name) < 0) {
if (existing && existing.compare(prop) === prop || !existing) {
el.styleProps[name] = prop;
if (style[i].type == 'property') {
var name = style[i].name;
var value = style[i].value;
var important = style[i].value.match(/!important$/) !== null;
if (important && !options.preserveImportant) value = value.replace(/\s*!important$/, '');
var prop = new utils.Property(name, value, selector, important ? 2 : 0);
var existing = el.styleProps[name];

// if property name is not in the excluded properties array
if (juiceClient.excludedProperties.indexOf(name) < 0) {
if (existing && existing.compare(prop) === prop || !existing) {
// deleting a property let us change the order (move it to the end in the setStyleAttrs loop)
if (existing) delete el.styleProps[name];
el.styleProps[name] = prop;
}
}
}
}
Expand All @@ -175,8 +181,7 @@ function inlineDocument($, css, options) {
// sort properties by their originating selector's specificity so that
// props like "padding" and "padding-bottom" are resolved as expected.
props.sort(function(a, b) {
return a.selector.specificity().join('').localeCompare(
b.selector.specificity().join(''));
return a.compareFunc(b);
});
var string = props
.filter(function(prop) {
Expand Down
18 changes: 11 additions & 7 deletions lib/property.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,23 @@ function Property(prop, value, selector, priority) {
* @api public
*/

Property.prototype.compare = function(property) {
var a = this.selector.specificity();
Property.prototype.compareFunc = function(property) {
var a = [];
a.push.apply(a, this.selector.specificity());
a[0] += this.priority;
var b = property.selector.specificity();
var b = [];
b.push.apply(b, property.selector.specificity());
b[0] += property.priority;
var winner = utils.compare(a, b);
return utils.compareFunc(a, b);
};

if (winner === a && a !== b) {
return this;
}
Property.prototype.compare = function(property) {
var winner = this.compareFunc(property);
if (winner === 1) return this;
return property;
};


/**
* Returns CSS property
*
Expand Down
121 changes: 56 additions & 65 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* Module dependencies.
*/

var cssom = require('cssom');
var mensch = require('mensch');
var cheerio = require('cheerio');
var own = {}.hasOwnProperty;
var os = require('os');
Expand All @@ -18,7 +18,7 @@ exports.Property = Property;
* Returns an array of the selectors.
*
* @license Sizzle CSS Selector Engine - MIT
* @param {String} selectorText from cssom
* @param {String} selectorText from mensch
* @api public
*/

Expand Down Expand Up @@ -61,33 +61,36 @@ exports.extract = function extract(selectorText) {
*/

exports.parseCSS = function(css) {
var rules = cssom.parse(css).cssRules || [];
var parsed = mensch.parse(css);
var rules = typeof parsed.stylesheet != 'undefined' && parsed.stylesheet.rules ? parsed.stylesheet.rules : [];
var ret = [];

for (var i = 0, l = rules.length; i < l; i++) {
if (rules[i].selectorText) { // media queries don't have selectorText
if (rules[i].type == 'rule') {
var rule = rules[i];
var selectors = exports.extract(rule.selectorText);
var selectors = rule.selectors;

for (var ii = 0, ll = selectors.length; ii < ll; ii++) {
ret.push([selectors[ii], rule.style]);
ret.push([selectors[ii], rule.declarations]);
}
}
}

return ret;
};


var getStringifiedStyles = function(rule) {
var styles = [];
for (var style = 0; style < rule.style.length; style++) {
var property = rule.style[style];
var value = rule.style[property];
var important = rule.style._importants[property] ? ' !important' : '';
styles.push(' ' + property + ': ' + value + important + ';');
}
return styles;
var removeStyle = function(style, startPos, endPos, skipRows, startOffset, endOffset, insert) {
var styleRows = style.split("\n");
var start = startOffset;
var end = endOffset;
for (var r = 1 + skipRows; r < startPos.line; r++) start += styleRows[r - 1 - skipRows].length + 1;
start += startPos.col;
if (endPos !== null) {
for (var r2 = 1 + skipRows; r2 < endPos.line; r2++) end += styleRows[r2 - 1 - skipRows].length + 1;
end += endPos.col;
} else end += style.length + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include block style braces { and } for these else and for statements. I think the JSCS rules in the repo require that.

var newStyle = style.substr(0, start - 1) + insert + style.substr(end - 1);
return newStyle;
};

/**
Expand All @@ -99,52 +102,35 @@ var getStringifiedStyles = function(rule) {
*/

exports.getPreservedText = function(css, options) {
var rules = cssom.parse(css).cssRules || [];
var preserved = [];

for (var i = 0, l = rules.length; i < l; i++) {
/* CSS types
STYLE: 1,
IMPORT: 3,
MEDIA: 4,
FONT_FACE: 5,
*/

if (options.fontFaces && rules[i].type === cssom.CSSFontFaceRule.prototype.type) {
var fontFace = [ '' ];
fontFace.push('@font-face {');
fontFace = fontFace.concat(getStringifiedStyles(rules[i]));
fontFace.push('}');

if (fontFace.length) {
preserved.push(fontFace.length ? fontFace.join(os.EOL) + os.EOL : '');
}
}

if (options.mediaQueries && rules[i].type === cssom.CSSMediaRule.prototype.type) {
var query = rules[i];
var queryString = [];

queryString.push(os.EOL + '@media ' + query.media[0] + ' {');

for (var ii = 0, ll = query.cssRules.length; ii < ll; ii++) {
var rule = query.cssRules[ii];

if (rule.type === cssom.CSSStyleRule.prototype.type
|| rule.type === cssom.CSSFontFaceRule.prototype.type) {
queryString.push(' '
+ (rule.type === cssom.CSSStyleRule.prototype.type ? rule.selectorText : '@font-face') + ' {');
queryString = queryString.concat(getStringifiedStyles(rule));
queryString.push(' }');
}
}

queryString.push('}');
preserved.push(queryString.length ? queryString.join(os.EOL) + os.EOL : '');
var parsed = mensch.parse(css, {position: true, comments: true});
var rules = typeof parsed.stylesheet != 'undefined' && parsed.stylesheet.rules ? parsed.stylesheet.rules : [];
var preserved = css;
var preserved2 = [];
var lastStart = null;

for (var i = rules.length - 1; i >= 0; i--) {
if (options.fontFaces && rules[i].type === 'font-face') {
// preserve
preserved2.push(mensch.stringify({ stylesheet: { rules: [ rules[i] ] }}, { comments: false, indentation: ' ' }).replace(/}/,os.EOL+'}'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

should replace be global here? /}/g? I'm finding this line long and hard to read, maybe add some line breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be global.

Consider this is just to "mimic" the output from CSSOM and not something I would keep in a final release (the additional \n is just to make some test pass without alterning the ".out" file)

As you can see I compute the result in 2 different ways (one creates a new CSS from "keep rules", the other removes unwanted styles from the original style, so to keep formatting of the kept stuff "as is").

I don't know if you prefer to make a first "menschified" release that tries to mimic the old behaviour so we collect user feedback on a "minimal change" or if you instead prefer to kepp adding the changes and then make a final release including all of the major changes.

} else if (options.mediaQueries && rules[i].type === 'media') {
// preserve
preserved2.push(mensch.stringify({ stylesheet: { rules: [ rules[i] ] }}, { comments: false, indentation: ' ' }));
} else {
// remove
preserved = removeStyle(preserved, rules[i].position.start, lastStart, 0, 0, 0, '');
// TODO +os.EOL?
}
lastStart = rules[i].position.start;
}
// preserved works by removing unwanted stuff, preserved2 by generating a new style with "stuff to keep"
// We have to decide what is our strategy (the preserved2 produces the same output of the cssom based version <= 2.0.0)
if (false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be if (false) {?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see the comment above (I just don't know what kind of behaviour you prefer, so I implemented both of them and kept an if to enable the alternative way.

Again I don't consider this code "ready to be released" but depending on the way you are used to collaborate this could be merged while we keep working on other aspects. I just wanted to share what I'm doing ASAP, so we stay more aligned.

if (preserved.trim().length === 0) return false;
return preserved;
} else {
if (preserved2.length === 0) return false;
return os.EOL+preserved2.join(os.EOL)+os.EOL;
}

return preserved.join(os.EOL);
};

/**
Expand All @@ -160,7 +146,8 @@ exports.cheerio = function(html, options) {
};

exports.normalizeLineEndings = function(text) {
return text.replace(/\r\n/g, '\n').replace(/\n/g, '\r\n');
// NOTE this consider multiple newlines the same as a single newline
return text.replace(/\r\n/g, '\n').replace(/\n/g, '\r\n').replace(/\r\n\r\n/g, '\r\n');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this change here. What is this solving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are differences in newlines in the way mensch writes CSS and juice wrote CSS (i talk about @media and @font-face rules). Instead of altering all of the ".out" files I decided that a CSS with one more or one less empty line can be considered the same for the tests.

What I tried to do is to do the strict changes to the "expected" out files, so it is easier to see the real differences between the two parsers.

Of course this replace can be changed if we recreate the out files as mensch wants them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, this is the smaller diff, so let's go with this for now

};

exports.encodeEJS = function(html) {
Expand Down Expand Up @@ -193,14 +180,18 @@ exports.decodeEntities = function(html) {
* @api public
*/

exports.compare = function(a, b) {
exports.compareFunc = function(a, b) {
for (var i = 0; i < 4; i++) {
if (a[i] === b[i]) { continue; }
if (a[i] > b[i]) { return a; }
return b;
if (a[i] > b[i]) { return 1; }
return -1;
}

return b;
return 0;
};

exports.compare = function(a, b) {
return exports.compareFunc(a, b) == 1 ? a : b;
};

exports.extend = function(obj, src) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"cheerio": "0.20.0",
"commander": "2.9.0",
"cross-spawn-async": "^2.1.8",
"cssom": "0.3.1",
"mensch": "brettstimmerman/mensch#v0.3.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this supposed to be a github reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works here and in the integrations tests.
I guess mensch 0.3.2 is not available in https://www.npmjs.com/package/mensch
mensch 0.3.2 includes a lot of fixes I did to mensch for mosaico that are needed here, too.

"deep-extend": "^0.4.0",
"slick": "1.12.2",
"web-resource-inliner": "2.0.0"
Expand Down
5 changes: 5 additions & 0 deletions test/cases/important-specificity.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.a {
padding-top: 5px !important;
padding-top: 7px;
padding-bottom: 5px;
}
1 change: 1 addition & 0 deletions test/cases/important-specificity.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<div class="a" style="padding: 10px"></div>
1 change: 1 addition & 0 deletions test/cases/important-specificity.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<div class="a" style="padding-bottom: 5px; padding: 10px; padding-top: 5px;"></div>
10 changes: 10 additions & 0 deletions test/cases/improper-syntax-affect-cascade-issue44.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
body {
padding: 5px;
}
a {
color: ;
}
table {
padding: 5px;
color: #333;
}
13 changes: 13 additions & 0 deletions test/cases/improper-syntax-affect-cascade-issue44.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Testing</title>
</head>
<body>
<table>
<tr>
<td><a href="sdf">a test!</a></td>
</tr>
</table>
</body>
</html>
13 changes: 13 additions & 0 deletions test/cases/improper-syntax-affect-cascade-issue44.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Testing</title>
</head>
<body style="padding: 5px;">
<table style="padding: 5px; color: #333;">
<tr>
<td><a href="sdf">a test!</a></td>
</tr>
</table>
</body>
</html>
6 changes: 6 additions & 0 deletions test/cases/inline-specificity.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.a {
padding-top: 5px !important;
}
.a {
padding-bottom: 5px;
}
1 change: 1 addition & 0 deletions test/cases/inline-specificity.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<div class="a" style="padding: 10px"></div>
1 change: 1 addition & 0 deletions test/cases/inline-specificity.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<div class="a" style="padding-bottom: 5px; padding: 10px; padding-top: 5px;"></div>
Loading