Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Avoid exception with :last-of-type etc. #875

Merged
merged 2 commits into from
Jun 11, 2020

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Jun 11, 2020

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 behaviour 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. Their level of support in email clients is likely to be drasitcally lower per se for 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.

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 behaviour 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 drasitcally 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.
@JakeQZ JakeQZ added the bug label Jun 11, 2020
@JakeQZ JakeQZ added this to the 4.0.0 milestone Jun 11, 2020
@JakeQZ JakeQZ requested review from oliverklee and zoliszabo June 11, 2020 03:44
@JakeQZ JakeQZ self-assigned this Jun 11, 2020
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jun 11, 2020

PS. In the README I am trying to clarify with (unsupported selector combinations being treated equivalently to) :not(*) that the selector will not match anything but will not be discarded completely if in a comma-separated list. E.g. those in-the-know will know that

input::-webkit-input-placeholder, input:-ms-input-placeholder, 
  input:-moz-placeholder, input::-moz-placeholder 
{
  font-size: 1.25rem;
}

will not work. Each specifically-targeted browser will throw away the entire rule because it doesn't recognize the component targeting a rival browser, and the above must be written as

input::-webkit-input-placeholder {
  font-size: 1.25rem;
}
input:-ms-input-placeholder {
  font-size: 1.25rem;
}
input:-moz-placeholder {
  font-size: 1.25rem;
}
input::-moz-placeholder {
  font-size: 1.25rem;
}

Or, for a non-browser-specific example lifted from a popular WordPress theme which does not work as well as it could:

.home.blog .site-header,
.home.page:not(.page-template-template-homepage) .site-header,
.home.post-type-archive-product .site-header {
    margin-bottom: 4.235801032em
}

IE8 (or any other parser which does not support :not) would, perfectly justifiably according to the spec, discard the entire rule, so it should be broken up as follows:

.home.blog .site-header,
.home.post-type-archive-product .site-header {
    margin-bottom: 4.235801032em
}
.home.page:not(.page-template-template-homepage) .site-header {
    margin-bottom: 4.235801032em
}

I want to be clear that this splitting up of rules is not required, even if it is slightly at odds with the spec.

Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful.

@oliverklee oliverklee merged commit 5bf2e14 into master Jun 11, 2020
@oliverklee oliverklee deleted the bugfix/symfony-exception branch June 11, 2020 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExpressionErrorException: "*:last-of-type" is not implemented.
2 participants