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

Process CSS selectors with states and add as style block #280

Closed
peterfox opened this issue Nov 6, 2015 · 7 comments
Closed

Process CSS selectors with states and add as style block #280

peterfox opened this issue Nov 6, 2015 · 7 comments
Assignees
Milestone

Comments

@peterfox
Copy link

peterfox commented Nov 6, 2015

Currently if you process CSS with state CSS selectors e.g. :hover these selectors get lost in the inline process. Instead it would be ideal if they're added to the HTML as a style block instead in the same way the media is.

@roman-1983
Copy link

+1

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2018

I'll be proposing a PR for this next week, so if anyone has had any more thoughts, or tried any code changes, let us know here.

@oliverklee
Copy link
Contributor

Sounds good to me. I say: Go for it! @jjriv, @zoliszabo: More thoughts?

@oliverklee oliverklee modified the milestones: 4.0.0, 3.0.0 Feb 10, 2018
@zoliszabo
Copy link
Contributor

Sounds good to me. I say: Go for it! @jjriv, @zoliszabo: More thoughts?

Sounds good to me too!
By the way: Nice PRs @JakeQZ!

@jjriv
Copy link
Contributor

jjriv commented Feb 12, 2018

I'm with @zoliszabo and @oliverklee

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 13, 2018

Proposed solution...

Currently we have in process():

    $cssParts = $this->splitCssAndMediaQuery($allCss);
    //...
    $cssRules = $this->parseCssRules($cssParts['css']);
    foreach ($cssRules as $cssRule) {
        //...

Here $cssRules does not currently contain rules from media queries or with selectors involving pseudo -classes or -elements whose rules cannot be inlined.

I suggest changing this so that $cssRules contains all rules, with each entry having additional keys:

  • "media" - Media query string if rule is from within a media query block;
  • "hasPseudo" - Boolean indicating whether the selector involves a pseudo -class or -element such that the rules cannot be inlined;
  • "uninlineable" - Convenience Boolean indicating either "media" is nonempty or "hasPseudo" is true (or both).

Alternatively, $cssRules could contain two arrays:

  • Inlineable rules (as before), sorted by selector precedence;
  • Uninlineable rules, i.e. those from media queries and/or with selectors involving psuedo-elements or behavioural pseudo-classes, with additional keys "media" and "hasPseudo" described above, and which do not need sorting by selector precedence.

The rationale is that original 'line number' information from media queries will also be required so that in the added style element they can be correctly ordered with respect to other rules for selectors involving pseudo-elements or behavioural psuedo-classes (and their rule blocks are currently parsed later anyway).

The loop which follows would ignore uninlineable rules.

For copying uninlineable rules to a style element, I suggest renaming copyCssWithMediaToStyleNode to a more general name like copyUninlineableCssToStyleNode. This would

  • iterate through the uninlineable CSS rules (sorted back into line number order if needed, depending on the above);
  • use existsMatchForCssSelector to test if each rule is relevant for the document;
  • for selectors involving pseudo-elements or behavioural psuedo-classes, strip these components from the selector passed to existsMatchForCssSelector (e.g. so "a:hover" becomes "a" for the test);
  • append each relevant rule to a CSS string, re-combining rules from the same original line number, and those from the same original media query block.

A simple CssConcatenator class may help with the final item, making the code clearer (though I note that reliance on autoloading is not until 4.0.0, so would any additional classes need to be defined in the same source file for now, or require'd if !class_exists(...)?).

Returning to $cssRules and its creation, and firstly splitCssAndMediaQuery. This would need to retain the ordering of the CSS media query blocks and any CSS rules between them. I suggest it returns an ordered array of such content in which each element is an array with the following keys:

  • "css" - The CSS content, either from between supported media query blocks (cleaned, with @import, @charset and unsupported media query blocks removed), or the content of a supported media query block (i.e. that within {...});
  • "media" - Media query string if the CSS is from within a media query block

Finally, parseCssRules would be changed to take the returned data from splitCssAndMediaQuery as its parameter, and return the value for $cssRules described above. It would include selectors for pseudo-elements and unsupported pseudo-classes, setting the "hasPseudo" flag, and copy the value for "media" from the input data. Alternatively it could call splitCssAndMediaQuery itself and be passed the full original CSS as its parameter.

The changes can be separated into two PRs, one for the preliminary refactoring, and one for actually adding the feature.

Thoughts? Comments? Questions?

@oliverklee
Copy link
Contributor

@JakeQZ Sounds good! I'd say: Go for it!

As for the two approaches: I'm for taking the one that is simpler or provides better performance.

And please let's have small, incremental PRs.

JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 16, 2018
Return from `splitCssAndMediaQuery` an array of split CSS parts for different
media queries, along with the parts of intervening CSS not within a media query,
in order.

The ordering will need to be retained for implementing MyIntervals#280.

Currently in `process` the return value is converted back to `$cssParts` as it
was before, as a temporary measure, pending further refactoring, of
`parseCssRules` and `copyCssWithMediaToStyleNode`.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 17, 2018
In tests using `assertNotContains`, assert only that a minimal string sufficient
to indicate a test failure is not present.

Changes for MyIntervals#280 will inevitably mean that during processing, due to the way
that some components of the CSS are `trim`med, some unnecessary whitespace is
removed from the CSS for media queries added to the style element.  Thus
asserting that a complete media query rule is not present in the result may
allow a test to pass that should fail.  Asserting that just, e.g., "@media" or
"@media tv" is not present in the result gives a more stringent test.

For completeness, needles used with `assertNotContains` in other tests have also
been similarly shortened.  Some of these tests would have allowed invalid
passes, e.g. with a semicolon at the end of a style attribute that was asserted
not to be present.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 17, 2018
Ensured there's at least one property declaration in CSS rule blocks used with
@media rule tests, as changes for MyIntervals#280 will result in elimination of empty CSS
rule blocks during processing.
oliverklee pushed a commit that referenced this issue Feb 18, 2018
Ensured there's at least one property declaration in CSS rule blocks used with
@media rule tests, as changes for #280 will result in elimination of empty CSS
rule blocks during processing.
oliverklee pushed a commit that referenced this issue Feb 18, 2018
In tests using `assertNotContains`, assert only that a minimal string sufficient
to indicate a test failure is not present.

Changes for #280 will inevitably mean that during processing, due to the way
that some components of the CSS are `trim`med, some unnecessary whitespace is
removed from the CSS for media queries added to the style element.  Thus
asserting that a complete media query rule is not present in the result may
allow a test to pass that should fail.  Asserting that just, e.g., "@media" or
"@media tv" is not present in the result gives a more stringent test.

For completeness, needles used with `assertNotContains` in other tests have also
been similarly shortened.  Some of these tests would have allowed invalid
passes, e.g. with a semicolon at the end of a style attribute that was asserted
not to be present.
oliverklee pushed a commit that referenced this issue Feb 20, 2018
Return from `splitCssAndMediaQuery` an array of split CSS parts for different
media queries, along with the parts of intervening CSS not within a media query,
in order.

The ordering will need to be retained for implementing #280.

Currently in `process` the return value is converted back to `$cssParts` as it
was before, as a temporary measure, pending further refactoring, of
`parseCssRules` and `copyCssWithMediaToStyleNode`.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 20, 2018
Added test assertion methods similar to `assertContains` and `assertNotContains`
but which also allow for removal of some unnecessary whitespace from the CSS,
without having to make the test data less humanly readable or change the test
inputs.

Changes for MyIntervals#280 will inevitably mean that during processing, due to the way
that some components of the CSS are `trim`med, some unnecessary whitespace is
removed from the CSS for media queries added to the style element.  Relevant
tests have been changed to used the new assertion methods.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 20, 2018
Added test assertion methods similar to `assertContains` and `assertNotContains`
but which also allow for removal of some unnecessary whitespace from the CSS,
without having to make the test data less humanly readable or change the test
inputs.

Changes for MyIntervals#280 will inevitably mean that during processing, due to the way
that some components of the CSS are `trim`med, some unnecessary whitespace is
removed from the CSS for media queries added to the style element.  Relevant
tests have been changed to used the new assertion methods.
oliverklee pushed a commit that referenced this issue Feb 20, 2018
Added test assertion methods similar to `assertContains` and `assertNotContains`
but which also allow for removal of some unnecessary whitespace from the CSS,
without having to make the test data less humanly readable or change the test
inputs.

Changes for #280 will inevitably mean that during processing, due to the way
that some components of the CSS are `trim`med, some unnecessary whitespace is
removed from the CSS for media queries added to the style element.  Relevant
tests have been changed to used the new assertion methods.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 21, 2018
In `splitCssAndMediaQuery`:
- Corrected regular expression for splitting out `@media` rules from the CSS:
  - not to consume a subsequent `@media` rule after an empty one;
  - not to allow disallowed media types with an extra space before the type;
- Corrected regular expression cleaning the remaining CSS to remove at-rules
  even if they are not at the beginning of a line;
- Use a common regular expression component for matching a `@media` rule body
- (using possessive quantifiers where possible for improved performance).

Added related PHPUnit tests of which those that would fail illustrate the issues
(and would also fail prior to MyIntervals#515).

(Found while working on MyIntervals#280, as the regular expression would match
"@media screen {} @media tv { h1 { color: red; } }" as a single `@media` rule,
causing an issue parsing its presumed rule body with `parseCssRules`.)
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 21, 2018
In `splitCssAndMediaQuery`:
- Corrected regular expression for splitting out `@media` rules from the CSS:
  - not to consume a subsequent `@media` rule after an empty one;
  - not to allow disallowed media types with an extra space before the type;
- Corrected regular expression cleaning the remaining CSS to remove at-rules
  even if they are not at the beginning of a line;
- Use a common regular expression component for matching a `@media` rule body
- (using possessive quantifiers where possible for improved performance).

Added related PHPUnit tests of which those that would fail illustrate the issues
(and would also fail prior to MyIntervals#515).

(Found while working on MyIntervals#280, as the regular expression would match
`"@media screen {} @media tv { h1 { color: red; } }"` as a single `@media` rule,
causing an issue parsing its presumed rule body with `parseCssRules`.)
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 22, 2018
Refactored `parseCssRules` so that it can now parse an entire CSS string,
including @media rules.  It now returns two arrays, in an array with the
following keys:
- "inlineable" => The inlineable rules that would have been previously returned;
- "uninlineable" => The uninlineable rules, currently those from within @media
  rules.

The original line number information from the CSS is available in both arrays,
so the original ordering of all the rules (that will be required for MyIntervals#280) can
be determined.

A temporary method `combineCssMediaRules` has been added to recombine the @media
rules back into a string of CSS as expected by `copyCssWithMediaToStyleNode`,
pending refactoring of that method to instead accept a parsed rule array as its
parameter.  This does mean that, for now, the rules within each @media rule are
parsed again.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 23, 2018
This abstracts the (re-)combining of CSS rules for various media queries and
with various selectors and declaration blocks, merging adjacent rule blocks
where possible (i.e. for the same media query, with the same selectors, or with
the same declarations block).

Although not yet utilized, it will be required for MyIntervals#280 to simplify the code.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Feb 28, 2018
Corrected regular expression for splitting out `@media` rules from the CSS not
to consume a subsequent `@media` rule after an empty one.

Similarly corrected the regular expression cleaning the unneeded @media rules,
using a common subexpression for matching a `@media` rule body.

(Found while working on MyIntervals#280, as the regular expression would match "@media
screen {} @media tv { h1 { color: red; } }" as a single @media rule, causing an
issue parsing its presumed rule body later.)
oliverklee pushed a commit that referenced this issue Mar 11, 2018
Corrected regular expression for splitting out `@media` rules from the CSS not
to consume a subsequent `@media` rule after an empty one.

Similarly corrected the regular expression cleaning the unneeded @media rules,
using a common subexpression for matching a `@media` rule body.

(Found while working on #280, as the regular expression would match "@media
screen {} @media tv { h1 { color: red; } }" as a single @media rule, causing an
issue parsing its presumed rule body later.)
oliverklee pushed a commit that referenced this issue Mar 29, 2018
This abstracts the (re-)combining of CSS rules for various media queries and
with various selectors and declaration blocks, merging adjacent rule blocks
where possible (i.e. for the same media query, with the same selectors, or with
the same declarations block).

Although not yet utilized, it will be required for #280 to simplify the code.
oliverklee pushed a commit that referenced this issue Mar 31, 2018
Refactored `parseCssRules` so that it can now parse an entire CSS string,
including @media rules.  It now returns two arrays, in an array with the
following keys:
- "inlineable" => The inlineable rules that would have been previously returned;
- "uninlineable" => The uninlineable rules, currently those from within @media
  rules.

The original line number information from the CSS is available in both arrays,
so the original ordering of all the rules (that will be required for #280) can
be determined.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Apr 2, 2018
Renamed `copyCssWithMediaToStyleNode` to `copyUninlineableCssToStyleNode` and
changed the final parameter from a string of CSS to the array of "uninlineable"
parsed CSS rule data returned by `parseCssRules`.

The method can now potentially also include rules other than @media rules in the
style element added to the document, and will do when the final changes for MyIntervals#280
are implemented.
oliverklee pushed a commit that referenced this issue Apr 2, 2018
Renamed `copyCssWithMediaToStyleNode` to `copyUninlineableCssToStyleNode` and
changed the final parameter from a string of CSS to the array of "uninlineable"
parsed CSS rule data returned by `parseCssRules`.

The method can now potentially also include rules other than @media rules in the
style element added to the document, and will do when the final changes for #280
are implemented.
JakeQZ added a commit to JakeQZ/emogrifier that referenced this issue Apr 3, 2018
Allow varying whitespace around commas (in selector list separators) in
`AssertContainsCss`, etc.

This will be required for testing that CSS rules with state-based pseudo-classes
are copied to the style element for MyIntervals#280.
oliverklee pushed a commit that referenced this issue Apr 3, 2018
Allow varying whitespace around commas (in selector list separators) in
`AssertContainsCss`, etc.

This will be required for testing that CSS rules with state-based pseudo-classes
are copied to the style element for #280.
JakeQZ added a commit that referenced this issue Apr 5, 2018
Copy CSS for selectors involving pseudo-elements and dynamic (state-based)
pseudo-classes to the style element added to the document (along with the media
queries).

`parseCssRules`:
- Include rules for such selectors in the "uninlineable" rules array (instead
  of discarding them), flagging them with "hasUnmatchablePseudo";
- Don't consider selectors with both an unsupported and a supported
  pseudo-class as matchable for inlining styles.

`copyUninlineableCssToStyleNode`:
- Strip the unmatchable pseudo-components from selectors before calling
  `existsMatchForCssSelector`.

Various unit tests added.

This addresses #280 for the `Emogrifier` class.
oliverklee pushed a commit that referenced this issue Apr 5, 2018
Copy CSS for selectors involving pseudo-elements and dynamic (state-based)
pseudo-classes to the style element added to the document (along with the media
queries).

`parseCssRules`:
- Include rules for such selectors in the "uninlineable" rules array (instead
  of discarding them), flagging them with "hasUnmatchablePseudo";
- Don't consider selectors with both an unsupported and a supported
  pseudo-class as matchable for inlining styles.

`copyUninlineableCssToStyleNode`:
- Strip the unmatchable pseudo-components from selectors before calling
  `existsMatchForCssSelector`.

Various unit tests added.

This addresses #280 for the `Emogrifier` class.
JakeQZ added a commit that referenced this issue Apr 6, 2018
@oliverklee oliverklee modified the milestones: 3.0.0, 2.1.0 Apr 6, 2018
oliverklee pushed a commit that referenced this issue Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants