From 569fdcd48bfd2febb2820afb15ec110974ee4ebc Mon Sep 17 00:00:00 2001
From: Timothy Asp <tim@ifixit.com>
Date: Tue, 4 Nov 2014 12:09:36 -0700
Subject: [PATCH] Add removeDisplayNone option + tests

This adds an options parameter and some tests for a `removeDisplayNone` option
which specifies if the Emogrifier parser will remove elements with the style
"display: none".

Unfortunatly, the nice little optimization has some consiquences.

When writing responsive email templates, "display: none" is used often to hide
certain sections of the template for desktop email clients and show them in
phone clients.

I can see how it'd be nice to clean up elements that will not be displayed in
some cases, so that's why I made it an options field.
---
 Classes/Emogrifier.php        | 68 ++++++++++++++++++++++++++---------
 README.md                     | 21 ++++++-----
 Tests/Unit/EmogrifierTest.php | 60 +++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 26 deletions(-)

diff --git a/Classes/Emogrifier.php b/Classes/Emogrifier.php
index 479dfbde..44887799 100644
--- a/Classes/Emogrifier.php
+++ b/Classes/Emogrifier.php
@@ -121,6 +121,14 @@ class Emogrifier
      */
     private $isStyleBlocksParsingEnabled = true;
 
+    /**
+     * Determines whether elements with the `display: none` property are
+     * removed from the DOM.
+     *
+     * @var bool
+     */
+    private $shouldKeepInvisibleNodes = true;
+
     /**
      * This attribute applies to the case where you want to preserve your original text encoding.
      *
@@ -289,23 +297,8 @@ public function emogrify()
             $this->fillStyleAttributesWithMergedStyles();
         }
 
-        // This removes styles from your email that contain display:none.
-        // We need to look for display:none, but we need to do a case-insensitive search. Since DOMDocument only
-        // supports XPath 1.0, lower-case() isn't available to us. We've thus far only set attributes to lowercase,
-        // not attribute values. Consequently, we need to translate() the letters that would be in 'NONE' ("NOE")
-        // to lowercase.
-        $nodesWithStyleDisplayNone = $xpath->query(
-            '//*[contains(translate(translate(@style," ",""),"NOE","noe"),"display:none")]'
-        );
-        // The checks on parentNode and is_callable below ensure that if we've deleted the parent node,
-        // we don't try to call removeChild on a nonexistent child node
-        if ($nodesWithStyleDisplayNone->length > 0) {
-            /** @var \DOMNode $node */
-            foreach ($nodesWithStyleDisplayNone as $node) {
-                if ($node->parentNode && is_callable(array($node->parentNode,'removeChild'))) {
-                    $node->parentNode->removeChild($node);
-                }
-            }
+        if ($this->shouldKeepInvisibleNodes) {
+            $this->removeInvisibleNodes($xpath);
         }
 
         $this->copyCssWithMediaToStyleNode($cssParts, $xmlDocument);
@@ -337,6 +330,16 @@ public function disableStyleBlocksParsing()
         $this->isStyleBlocksParsingEnabled = false;
     }
 
+    /**
+     * Disables the removal of elements with `display: none` properties.
+     *
+     * @return void
+     */
+    public function disableInvisibleNodeRemoval()
+    {
+        $this->shouldKeepInvisibleNodes = false;
+    }
+
     /**
      * Clears all caches.
      *
@@ -418,6 +421,37 @@ public function removeUnprocessableHtmlTag($tagName)
         }
     }
 
+     /**
+      * This removes styles from your email that contain display:none.
+      * We need to look for display:none, but we need to do a case-insensitive search. Since DOMDocument only
+      * supports XPath 1.0, lower-case() isn't available to us. We've thus far only set attributes to lowercase,
+      * not attribute values. Consequently, we need to translate() the letters that would be in 'NONE' ("NOE")
+      * to lowercase.
+      *
+      * @param \DOMXPath $xpath
+      *
+      * @return void
+      */
+    private function removeInvisibleNodes(\DOMXPath $xpath)
+    {
+        $nodesWithStyleDisplayNone = $xpath->query(
+            '//*[contains(translate(translate(@style," ",""),"NOE","noe"),"display:none")]'
+        );
+
+        if ($nodesWithStyleDisplayNone->length === 0) {
+            return;
+        }
+
+        // The checks on parentNode and is_callable below ensure that if we've deleted the parent node,
+        // we don't try to call removeChild on a nonexistent child node
+        /** @var \DOMNode $node */
+        foreach ($nodesWithStyleDisplayNone as $node) {
+            if ($node->parentNode && is_callable(array($node->parentNode, 'removeChild'))) {
+                $node->parentNode->removeChild($node);
+            }
+        }
+    }
+
     /**
      * Normalizes the value of the "style" attribute and saves it.
      *
diff --git a/README.md b/README.md
index 0562bff4..fbc9005c 100644
--- a/README.md
+++ b/README.md
@@ -65,12 +65,12 @@ After you have set the HTML and CSS, you can call the `emogrify` method to merge
 
 ## Options
 
-There are several options that you can set on the Emogrifier object before 
+There are several options that you can set on the Emogrifier object before
 calling the `emogrify` method:
 
 * `$emogrifier->disableStyleBlocksParsing()` - By default, Emogrifier will grab
   all `<style>` blocks in the HTML and will apply the CSS styles as inline
-  "style" attributes to the HTML. The `<style>` blocks will then be removed 
+  "style" attributes to the HTML. The `<style>` blocks will then be removed
   from the HTML. If you want to disable this functionality so that Emogrifier
   leaves these `<style>` blocks in the HTML and does not parse them, you should
   use this option.
@@ -78,6 +78,9 @@ calling the `emogrify` method:
   preserves all of the "style" attributes on tags in the HTML you pass to it.
   However if you want to discard all existing inline styles in the HTML before
   the CSS is applied, you should use this option.
+* `$emogrifier->disableInvisibleNodeRemoval()` - By default, Emogrifier removes
+  elements from the DOM that have the style attribute `display: none;`.  If
+  you would like to keep invisible elements in the DOM, use this option.
 
 
 ## Installing with Composer
@@ -163,18 +166,18 @@ Those that wish to contribute bug fixes, new features, refactorings and clean-up
 When you contribute, please take the following things into account:
 
 * Please cover all changes with unit tests and make sure that your code does not break any existing tests.
-* To run the existing PHPUnit tests, run the following 
+* To run the existing PHPUnit tests, run the following
   command:
-  
+
       composer install
-  
+
   and then run this command:
-  
+
       vendor/bin/phpunit Tests/
-  
-  If you want to uninstall the development packages that were installed when 
+
+  If you want to uninstall the development packages that were installed when
   you ran `composer install`, you can run this command:
-  
+
       composer install --no-dev
 * Please use the same coding style (PSR-2) as the rest of the code. Indentation is four spaces.
 * Please make your code clean, well-readable and easy to understand.
diff --git a/Tests/Unit/EmogrifierTest.php b/Tests/Unit/EmogrifierTest.php
index 9016a2f4..f6bb51d6 100644
--- a/Tests/Unit/EmogrifierTest.php
+++ b/Tests/Unit/EmogrifierTest.php
@@ -1090,4 +1090,64 @@ public function emogrifyMergesCssWithMixedUnits()
             $this->subject->emogrify()
         );
     }
+
+    /**
+     * @test
+     */
+    public function emogrifyByDefaultRemovesElementsWithDisplayNoneFromExternalCss()
+    {
+        $css = 'div.foo { display: none; }';
+        $html = $this->html5DocumentType . self::LF .
+            '<html><body><div class="bar"></div><div class="foo"></div></body></html>';
+
+        $expected = '<div class="bar"></div>';
+
+        $this->subject->setHtml($html);
+        $this->subject->setCss($css);
+
+        $this->assertContains(
+            $expected,
+            $this->subject->emogrify()
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function emogrifyByDefaultRemovesElementsWithDisplayNoneInStyleAttribute()
+    {
+        $html = $this->html5DocumentType . self::LF .
+            '<html><body><div class="bar"></div><div class="foobar" style="display: none;"></div>'
+                . '<div class="foo"></div></body></html>';
+
+        $expected = '<div class="bar"></div><div class="foo"></div>';
+
+        $this->subject->setHtml($html);
+
+        $this->assertContains(
+            $expected,
+            $this->subject->emogrify()
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function emogrifyAfterDisableInvisibleNodeRemovalPreservesInvisibleElements()
+    {
+        $css = 'div.foo { display: none; }';
+        $html = $this->html5DocumentType . self::LF .
+            '<html><body><div class="bar"></div><div class="foo"></div></body></html>';
+
+        $expected = '<div class="foo" style="display: none;">';
+
+        $this->subject->setHtml($html);
+        $this->subject->setCss($css);
+        $this->subject->disableInvisibleNodeRemoval();
+
+        $this->assertContains(
+            $expected,
+            $this->subject->emogrify()
+        );
+    }
 }