Invisible node removal: preheaders and responsive design

[sanderkruger]

Invisible node removal is nice but it creates problems for 2 cases: preheaders and responsive design.

Preheaders are hidden paragraphs at the start of an email to force a digest of the email to show a certain text. When an email is nicely designed with some links at the top or the subject repeated as a header, this is not the most important content you want recipients to see in the email list of their email client. Most email clients show the first 80-something readable characters of an email.

Typically, you put a <span class="preheader">blablabla</span> as the first element in the body and attach display:none, font-size=1px and some other styles to it to make sure it's hidden.

It would be good to be able to exclude certain selectors from invisible node removal.

The second issue is with responsive design. You may want to have 2 alternative representations in the document, by default one is shown and the other is hidden. A media query hides the one and shows the other.

invisible node removal should check media queries to see if a hidden node might be shown under certain circumstances.

One interesting way to handle both cases and open a route to more generic control over emogrifier is to introduce emogrifier classes. The idea is that by applying a class to a DOM element, that element gets special treatment from emogrifier. For example:

<span class="emogrifier-show preheader">My preheader</span>

Emogrifier would see the class and keep the node, despite being a candidate for invisible node removal. The emogrified result HTML would remove all classes starting with emogrifier-.

[oliverklee]

Sounds good! 👍

[oliverklee]

@jjriv, @zoliszabo I'd like to completely remove the removal of invisible nodes. Minifying the HTML should not be the job of Emogrifier, but the job of the person/component creating the input HTML in the first place, and removing those nodes is tricky. Thoughts?

[zoliszabo]

Since the feature is already implemented, instead of removing it, wouldn't it be better to only change the default behavior to not remove invisible nodes by default?

Secondly, the current option flag name is not OK: it does exactly the opposite to how it is called.

emogrifier/Classes/Emogrifier.php

Lines 408 to 410 in 73219f1

	if ($this->shouldKeepInvisibleNodes) {
	$this->removeInvisibleNodes($xPath);
	}

(PR #472)

[oliverklee]

I find it very important that we have the courage to remove code or features once it does not provide enough benefit anymore or creates problems which are not easily solvable. Every line of code is a line we need to maintain and test.

If I read @sanderkruger's comment correctly, the removal of invisible nodes creates problems as the nodes might not stay hidden or might be needed for other purposes than being visible.

As the removal of (hopefully) invisible nodes has the only purpose of reducing the HTML size, we can remove this feature without any visible problems, while avoiding the problems this feature brings.

Removing unneeded elements should be the responsible of the client providing the HTML or some other class/tool that comes after Emogrifier has inlined the CSS.

[jjriv]

I agree with deprecating this feature. Emogrifier has one purpose, and that is not to minimize or alter the payload.

[oliverklee]

So we'll deprecate this feature and move it to a separate class.

[JakeQZ]

This should be straightfoward to implement - we can just modify the XPath in HtmlPruner::DISPLAY_NONE_MATCHER to exclude elements with a specific class.

For the class name, I would suggest a vendor prefix is approprate, and 'keep' or perhaps 'do not remove' is more descriptive than 'show', so we'd have class="-emogrifier-keep" or perhaps class="-emogrifier-do-not-remove".

The class would be automatically removed by HtmlPruner::removeRedundantClassesAfterCssInlined() (or HtmlPruner::removeRedundantClasses()) if invoked after removeElementsWithDisplayNone() (which is the logically more optimal order anyway). So I would suggest that users would need to invoke one of these methods in order to have the class removed, rather than us implement duplicate functionality in removeElementsWithDisplayNone() to do this.

@oliverklee, @zoliszabo, Does this seem reasonable?

[oliverklee]

Sounds good. (I'd prefer "keep" over "not remove".)