Repository: Automattic/juice (js)

Issue:
core bug with "getPreservedText" function
The function `getPreservedText` in `lib/utils.js` accepts two arguments, `css` and `options`.  The `options` object consists of `fontFaces`, `mediaQueries`, `keyFrames`, and `pseudos` which are all Boolean values.  ~The core bug is that regardless if any of them are false, if at least one is true, then the entire `css` String passed will be preserved.~

The way it should work is that the `css` String passed should have filtered out values based off the `options` Booleans passed.  For example, if you have `pseudos` set to an Array (which by default already exists, as `juice.ignoredPseudos`, then any pseudoselector in the `css` String such as `:hover`, `:active`, etc should be removed from resulting `css` output, and then if `css` is empty, it would return false as `preserved.length === 0`.

The reason why this is such a MAJOR bug is that ~if you don't manually pass `false` for everything, then~ Gmail and other mail clients will render a message "Message too long, clipped..." if you have a lot of HTML/CSS in your email layout.

Pull request:
fix: fixed core bug with getPreservedText (closes #328)
I also fixed the tests, not sure why `src=""` was being changed to `src`.

Patch:
diff --git a/lib/inline.js b/lib/inline.js
index c86940c..2899c04 100644
--- a/lib/inline.js
+++ b/lib/inline.js
@@ -352,8 +352,8 @@ function getStylesData($, options) {
         mediaQueries: options.preserveMediaQueries,
         fontFaces: options.preserveFontFaces,
         keyFrames: options.preserveKeyFrames,
-        pseudos: juiceClient.ignoredPseudos
-      });
+        pseudos: options.preservePseudos
+      }, juiceClient.ignoredPseudos);
       if (preservedText) {
         styleElement.childNodes[0].nodeValue = preservedText;
       } else {
diff --git a/lib/utils.js b/lib/utils.js
index 632e10d..051e1eb 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -85,7 +85,7 @@ exports.parseCSS = function(css) {
  * @api public
  */
 
-exports.getPreservedText = function(css, options) {
+exports.getPreservedText = function(css, options, ignoredPseudos) {
   var parsed = mensch.parse(css, {position: true, comments: true});
   var rules = typeof parsed.stylesheet != 'undefined' && parsed.stylesheet.rules ? parsed.stylesheet.rules : [];
   var preserved = [];
@@ -95,7 +95,7 @@ exports.getPreservedText = function(css, options) {
     if ((options.fontFaces && rules[i].type === 'font-face') ||
         (options.mediaQueries && rules[i].type === 'media') ||
         (options.keyFrames && rules[i].type === 'keyframes') ||
-        (options.pseudos && rules[i].selectors && this.matchesPseudo(rules[i].selectors[0], options.pseudos))) {
+        (options.pseudos && rules[i].selectors && this.matchesPseudo(rules[i].selectors[0], ignoredPseudos))) {
       preserved.unshift(
         mensch.stringify(
           { stylesheet: { rules: [ rules[i] ] }},


Test patch:
diff --git a/test/juice.test.js b/test/juice.test.js
index 78c9fa4..a8ebc9f 100644
--- a/test/juice.test.js
+++ b/test/juice.test.js
@@ -189,6 +189,6 @@ it('test style attributes and important priority', function() {
 
 it('test that preserved text order is stable', function() {
   assert.deepEqual(
-      utils.getPreservedText('div { color: red; } @media (min-width: 320px) { div { color: green; } } @media (max-width: 640px) { div { color: blue; } }', { mediaQueries: true }).replace(/\s+/g, ' '),
+      utils.getPreservedText('div { color: red; } @media (min-width: 320px) { div { color: green; } } @media (max-width: 640px) { div { color: blue; } }', { mediaQueries: true }, juice.ignoredPseudos).replace(/\s+/g, ' '),
       ' @media (min-width: 320px) { div { color: green; } } @media (max-width: 640px) { div { color: blue; } } ');
-});
\ No newline at end of file
+});