Individual style properties overridden by shorthand properties could be eliminated

[JakeQZ]

Example in #508. When the resultant inline style is something like margin-top: 0; margin: 1em;, the individual property margin-top: 0 is completely overridden by the shorthand margin: 1em and could be eliminated.

[JakeQZ]

First question: is this something that would be considered 'core' functionality, or something that should go into a HtmlProcessor class (presumably HtmlMinifier) as outlined in #479?

Data for CSS shorthand properties available at https://github.com/gilmoreorless/css-shorthand-properties/blob/master/index.js could easily be converted to PHP.

[oliverklee]

Let's put it in a separate class. I propose the class name CssMinifier and the method name collapseShorthandProperties.

[JakeQZ]

Ok.

I think some common functionality would need to be shared between Emogrifier and CssMinifier, notably parseCssDeclarationsBlock, attributeValueIsImportant and generateStyleStringFromSingleDeclarationsArray. Whilst these are fairly stand-alone, they do use two caches which would need to be object properties. Could the code reuse be achieved via a PHP trait (perhaps named CssDeclarationsBlockParser)?

If so, for compatibility with non-autoloaders, would it be sufficient to add if (!trait_exists('qualified\trait\name')) { require 'path/to/trait.php'; } until such time as autoloading is a requirement?

Also, it seems inefficient to serialize the Emogrified DOMDocument into HTML then immediately reparse it back into a DOMDocument. Could there be an emogrifyDomDocument method which returns the DOMDocument after Emogrification (like emogrifyBodyContent has an alternative return value), with the AbstractHtmlProcessor constructor allowing either an HTML string or a DOMDocument as its parameter?

In terms of the actual implementation, I think something like this should work:

• Iterate through all elements with a style attribute;
• For each, use parseCssDeclarationsBlock to obtain the properties as keys and values, and iterate these;
• If any are a shorthand property, iterate through the corresponding individual properties and test if any exist;
• For any individual properties, determine whether they can be unset according to attributeValueIsImportant and their position in the declarations list (after or before the shorthand property);
• Update the style attribute using generateStyleStringFromSingleDeclarationsArray.

I would imagine separate PRs for the various preliminary refactoring before the actual implementation.

[oliverklee]

Sorry, I was not clear enough about the Composer dependency. I'll do my best to clear that up now. :-)

The Emogrifier class itself should not use any Composer autoloading before Emogrifier 4.0.0 (so that the Emogrifier class could be dropped in any non-Composer project up to version 3.x.y). The other classes can use Composer autoloading. So we could (temporarily) duplicate code into the other classes and then de-duplicate it for version 4.0.0.

Yes, allowing to provide and get back the DOMDocument makes sense. We then should allow to create a new HtmlProcessor from either HTML or a DOMDocument, e.g. like this (with the names just as examples):

$processor = CssMinifier::fromDomDocument($document);
$newHtml = $cssMinifier->collapseAttributes()->render();
$anotherProcessor = CssMinifier::fromHtml($document);
$processedDocument = $anotherProcessor->collapseAttributes()->getDomDocument();

@JakeQZ, @zoliszabo What do you think?

[JakeQZ]

Presumably then AbstractHtmlProcessor would have a private no-argument constructor and private methods initializeFromHtml and initializeFromDomDocument which would be called by the public fromHtml and fromDomDocument.

In terms of retro-fitting the pattern of methods render and getDomDocument to Emogrifier, there would need to be a new method which would currently call createAndProcessXmlDocument and store the result in an xmlDocument property for access by render or getDomDocument. The emogrify method name is already taken (which does that and render) - emogrifyOnly? emogrifyDomDocument...? Or emogrify has an optional parameter added for whether to render and return the HTML?

[oliverklee]

Presumably then AbstractHtmlProcessor would have a private no-argument constructor and private methods initializeFromHtml and initializeFromDomDocument which would be called by the public fromHtml and fromDomDocument.

Exactly.

[oliverklee]

#479 (comment)

We're planning to change the Emogrifier class to a CssInliner class. If we're changing the class anyway, we can also change the interface (and also inherit from AbstractHtmlProcessor).

As a first step, we could add a new CssInliner class that uses the new interface and redirects most of its work to the old Emogrifier class (and has basically the same tests). We then could port the code bit-by-bit. What do you think?

[oliverklee]

As a first step, we could add a new CssInliner class that uses the new interface and redirects most of its work to the old Emogrifier class (and has basically the same tests). We then could port the code bit-by-bit. What do you think?

After thinking about it a bit more, I think we should

1. copy the code to the new class
2. change the interface to be the same as the AbstractHtmlProcessor
3. inherit from AbstractHtmlProcessor
4. for Emogrifier 3.0.0, deprecate the old Emogrifier class
5. for Emogrifier 4.0.0, remove the old Emogrifier class

[JakeQZ]

Yes, that would make sense. It would also mean reusable code can be refactored in the new CssInliner class for sharing with HtmlMinifier etc. without waiting until 4.0.0. I guess that until 2.1.0, enhancements/fixes would be applied to both CssInliner and Emogrifier, but after that there would be no need to maintain the old class.

[oliverklee]

I guess that until 2.1.0, enhancements/fixes would be applied to both CssInliner and Emogrifier, but after that there would be no need to maintain the old class.

Up to including version 3.0.0 (i.e., the last version before 4.0.0).

We can keep version 3.0.0 small (mostly to have a release for the breaking changes).

[oliverklee]

Now that I've thought about it a bit more, we could mark the Emogrifier class as deprecated in version 3.0.0, and recommend using the new class. Then we could leave the Emogrifier class unchanged after 2.1.0 (except for removing the deprecated methods).

[zoliszabo]

I am sorry I am bit late to the party.

The Emogrifier class itself should not use any Composer autoloading before Emogrifier 4.0.0 (so that the Emogrifier class could be dropped in any non-Composer project up to version 3.x.y). The other classes can use Composer autoloading. So we could (temporarily) duplicate code into the other classes and then de-duplicate it for version 4.0.0.

What about making the old Emogrifier class a simple adapter for CssInliner and we bundle everything into a single big source file via some automated tool? Then each time we make changes to our new classes, we just regenerate the legacy Emogrifier class (bundle file)?
This would help us not to duplicate code manually and we could (easily) support legacy class Emogrifier up until version 4.

I will try to prepare a PR with the above this weekend.

What do you guys think?

[oliverklee]

Please let's move this discussion to #516.