Skip to content

Commit

Permalink
[BUGFIX] Avoid exception with :last-of-type etc. (#875)
Browse files Browse the repository at this point in the history
This simply broadens the range of exceptions thrown by `symfony/css-selector`
(for unsupported selector expressions) which will be caught and discarded in
non-debug mode.

It does not make anything magically work which didn't previously; however, it
does allow the rest of the CSS to achieve full functionality, with only the
affected CSS rule and selector suffering.

The tests and changes to the README clarify and correct-as-documented the
current behavior for these edge cases, which could be improved upon in future.

These are currently rarely-used CSS pseudo-classes; their level of support in
mainstream browsers has only
[recently reached 95% of audiences](https://caniuse.com/#search=last-of-type).
Their level of support in email clients is likely to be drastically lower per
se of some time to come, but may be something Emogrifier can help with moving
forwards, as their use becomes more widespread...

Closes #872.
New issue #876 for follow-up.
  • Loading branch information
JakeQZ authored Jun 11, 2020
1 parent a23db03 commit 5bf2e14
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 16 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
([#773](https://github.com/MyIntervals/emogrifier/pull/773))

### Fixed
- Allow `:last-of-type` etc. without type, without causing exception
([#875](https://github.com/MyIntervals/emogrifier/pull/875))
- Make sure to use the Composer-installed development tools
([#862](https://github.com/MyIntervals/emogrifier/pull/862),
[#865](https://github.com/MyIntervals/emogrifier/pull/865))
Expand Down
19 changes: 11 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -308,33 +308,36 @@ Emogrifier currently supports the following
* [first-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-child)
* [first-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-of-type)
(with a type, e.g. `p:first-of-type` but not `*:first-of-type` which will
behave as `*:first-child`)
currently be treated as `*:not(*)`)
* [last-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-child)
* [last-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-of-type)
(with a type)
(with a type – without a type, it will be treated as `:not(*)`)
* [not()](https://developer.mozilla.org/en-US/docs/Web/CSS/:not)
* [nth-child()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-child)
* [nth-last-child()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-child)
* [nth-last-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-of-type)
(with a type)
(with a type – without a type, it will be treated as `:not(*)`)
* [nth-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-of-type)
(with a type)
(with a type – without a type, it will be applied as if `:nth-child`)
* [only-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-child)
* [only-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-of-type)
(with a type)
(with a type – without a type, it will be applied as if `:only-child`
or `:not(*)`, depending on version constraints for `symfony/css-selector`)

The following selectors are not implemented yet:

* [case-insensitive attribute value](https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors#case-insensitive)
* static [pseudo-classes](https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes):
* [first-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-of-type)
without a type (will behave as `:first-child`)
without a type (declarations discarded)
* [last-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-of-type)
without a type (will behave as `:last-child`)
without a type (declarations discarded)
* [nth-last-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-of-type)
without a type (will behave as `:nth-last-child()`)
without a type (declarations discarded)
* [nth-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-of-type)
without a type (will behave as `:nth-child()`)
* [only-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-of-type)
without a type (will behave as `:only-child()` or `:not(*)`)
* any pseudo-classes not listed above as supported – rules involving them
will nonetheless be preserved and copied to a `<style>` element in the
HTML – including (but not necessarily limited to) the following:
Expand Down
16 changes: 8 additions & 8 deletions src/CssInliner.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Pelago\Emogrifier\HtmlProcessor\AbstractHtmlProcessor;
use Pelago\Emogrifier\Utilities\CssConcatenator;
use Symfony\Component\CssSelector\CssSelectorConverter;
use Symfony\Component\CssSelector\Exception\SyntaxErrorException;
use Symfony\Component\CssSelector\Exception\ParseException;

/**
* This class provides functions for converting CSS styles into inline style attributes in your HTML code.
Expand Down Expand Up @@ -153,7 +153,7 @@ class CssInliner extends AbstractHtmlProcessor
*
* @return self fluent interface
*
* @throws SyntaxErrorException
* @throws ParseException
*/
public function inlineCss(string $css = ''): self
{
Expand Down Expand Up @@ -183,7 +183,7 @@ public function inlineCss(string $css = ''): self
foreach ($cssRules['inlinable'] as $cssRule) {
try {
$nodesMatchingCssSelectors = $this->xPath->query($cssSelectorConverter->toXPath($cssRule['selector']));
} catch (SyntaxErrorException $e) {
} catch (ParseException $e) {
if ($this->debug) {
throw $e;
}
Expand Down Expand Up @@ -569,15 +569,15 @@ private function extractFontFaceRules(string $css): array
*
* @return \DOMElement[]
*
* @throws SyntaxErrorException
* @throws ParseException
*/
private function getNodesToExclude(): array
{
$excludedNodes = [];
foreach (\array_keys($this->excludedSelectors) as $selectorToExclude) {
try {
$matchingNodes = $this->xPath->query($this->getCssSelectorConverter()->toXPath($selectorToExclude));
} catch (SyntaxErrorException $e) {
} catch (ParseException $e) {
if ($this->debug) {
throw $e;
}
Expand Down Expand Up @@ -1018,7 +1018,7 @@ private function determineMatchingUninlinableCssRules(array $cssRules)
*
* @return bool
*
* @throws SyntaxErrorException
* @throws ParseException
*/
private function existsMatchForSelectorInCssRule(array $cssRule): bool
{
Expand All @@ -1038,13 +1038,13 @@ private function existsMatchForSelectorInCssRule(array $cssRule): bool
*
* @return bool
*
* @throws SyntaxErrorException
* @throws ParseException
*/
private function existsMatchForCssSelector(string $cssSelector): bool
{
try {
$nodesMatchingSelector = $this->xPath->query($this->getCssSelectorConverter()->toXPath($cssSelector));
} catch (SyntaxErrorException $e) {
} catch (ParseException $e) {
if ($this->debug) {
throw $e;
}
Expand Down
115 changes: 115 additions & 0 deletions tests/Unit/CssInlinerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3083,6 +3083,121 @@ public function inlineCssNotInDebugModeKeepsInvalidOrUnrecognizedSelectorsInMedi
self::assertContainsCss($css, $subject->render());
}

/**
* @return string[][]
*/
public function provideCssRulesWithUnsupportedSelectorCombination(): array
{
return [
':first-of-type without type' => [':first-of-type { color: red; }'],
':last-of-type without type' => [':last-of-type { color: red; }'],
':nth-last-of-type without type' => [':nth-last-of-type(2n) { color: red; }'],
];
}

/**
* This test enstablishes the current expected/observed behaviour with currently supported versions of
* `symfony/css-selector` for static pseudo-classes which are only partially supported.
*
* The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
* <style> element so that they can at least be applied correctly by fully-supporting email clients. It is also
* possible that (before then) future changes to Symfony may break this test, in which case the documentation would
* need updating and the tests adjusting.
*
* @test
*
* @param string $css
*
* @dataProvider provideCssRulesWithUnsupportedSelectorCombination
*/
public function inlineCssNotInDebugModeDiscardsRulesWithUnsupportedSelectorCombination(string $css)
{
$subject = CssInliner::fromHtml('<html><p>Hello</p><p>World</p></html>');
$subject->setDebug(false);

$subject->inlineCss($css);

$result = $subject->render();
self::assertNotContainsCss($css, $result);
self::assertNotContains('color: red', $result);
}

/**
* @return string[][]
*/
public function provideCssRulesWithPossiblyIncorrectlyImplementedSelectorCombination(): array
{
return [
':only-of-type without type' => [':only-of-type { color: red; }'],
];
}

/**
* This test enstablishes the current expected/observed behaviour with currently supported versions of
* `symfony/css-selector` for static pseudo-classes which are only partially supported.
*
* The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
* <style> element so that they can at least be applied correctly by fully-supporting email clients. It is also
* possible that (before then) future changes to Symfony may break this test, in which case the documentation would
* need updating and the tests adjusting.
*
* @test
*
* @param string $css
*
* @dataProvider provideCssRulesWithPossiblyIncorrectlyImplementedSelectorCombination
*/
public function inlineCssNotInDebugModeMayDiscardRulesWithPossiblyIncorrectlyImplementedSelectorCombination(
string $css
) {
$subject = CssInliner::fromHtml('<html><p>Hello</p><p>World</p></html>');
$subject->setDebug(false);

$subject->inlineCss($css);

$result = $subject->render();
self::assertNotContainsCss($css, $result);
// The declaration may or may not be haphazardly applied depending on `symofony/css-selector` version.
// Nothing more can be asserted that would always be true for the full range of versions supported.
}

/**
* @return string[][]
*/
public function provideCssRulesWithIncorrectlyImplementedSelectorCombination(): array
{
return [
':nth-of-type without type' => [':nth-of-type(2n) { color: red; }'],
];
}

/**
* This test enstablishes the current expected/observed behaviour with currently supported versions of
* `symfony/css-selector` for static pseudo-classes which are only partially supported.
*
* The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
* <style> element so that they can at least be applied correctly by fully-supporting email clients. It is also
* possible that (before then) future changes to Symfony may break this test, in which case the documentation would
* need updating and the tests adjusting.
*
* @test
*
* @param string $css
*
* @dataProvider provideCssRulesWithIncorrectlyImplementedSelectorCombination
*/
public function inlineCssAppliesRulesWithIncorrectlyImplementedSelectorCombination(string $css)
{
$subject = $this->buildDebugSubject('<html><p>Hello</p><p>World</p></html>');

$subject->inlineCss($css);

$result = $subject->render();
self::assertNotContainsCss($css, $result);
// The declaration will currently be haphazardly applied: in the case of `...of-type`, as if `...child`.
self::assertContains('color: red', $result);
}

/**
* @return string[][]
*/
Expand Down

0 comments on commit 5bf2e14

Please sign in to comment.