[FEATURE] Add the ability to excluded CSS selectors from being inlined #1202

nlemoine · 2023-01-05T14:13:52Z

Exclude CSS selectors from the stylesheets being inlined. This might for example prevent global CSS rules with universal selector (*) to be inlined on each node.

oliverklee

Thanks a lot for this contribution!

I've added some review comments.

And could you also please add a changelog entry (newest on top, and with a reference to the PR ID)? Thanks!

tests/Unit/CssInlinerTest.php

src/CssInliner.php

JakeQZ

I like the neatness of this being an almost one-line change.

Some usage documentation for the feature should also be added to the README.md file.

JakeQZ · 2023-01-06T23:41:48Z

src/CssInliner.php

+            if (\count(\array_intersect($cssRule->getSelectors(), \array_keys($this->excludedCssSelectors)))) {
+                continue;
+            }


I'm assuming the excluded selector list matches a full selector from a possibly comma-separated list, so .example will not exclude p.example or p .example. It would be good to confirm though that p .example would also exclude selectors written with varying whitespace (e.g. newline or multiple spaces), perhaps using a dataProvider for the currently-named addExcludedCssSelector test, so it can be run with a few different excluded selectors vs target selectors.

This has been taken care of here: https://github.com/MyIntervals/emogrifier/pull/1202/files#diff-d57870a239314b85bdf5a191a3629018f9c95e6019b3405584d36b3b9abddd48R638-R642

And tested here: https://github.com/MyIntervals/emogrifier/pull/1202/files#diff-2eda2d27b064a67021c7004fd070acb00ac6bf08695521484af6c087c6eec0baR3188-R3197

Note that I didn't normalized excluded CSS selectors, I assume developers are able to fill this with the right characters.

I assume developers are able to fill this with the right characters.

My point was just to add a couple of extra tests to help define and isolate the precise behaviour of the new functionality. Thanks for continued engagement and persuing this. Your idea is good.

I'll look into your latest changes in the next few days, and expect @oliverklee will too.

Thanks again <3

My point was just to add a couple of extra tests to help define and isolate the precise behaviour of the new functionality

And it was totally legit, I wasn't sure it was meant to be handled on the user input (addExcludedCssSelector('.example p')), or on the selectors found in the provided CSS. I went for the provided CSS only since weird spaces/lines breaks or very unlikely passed in addExcludedCssSelector method. Let me know if you feel like it normalization should be applied on both.

I wasn't sure it was meant to be handled on the user input (addExcludedCssSelector('.example p')), or on the selectors found in the provided CSS.

Not quite sure what you mean by this. Aren't they both user input? In context, the most usery of input will be the CSS. The parameter to the function, whilst also user input, will likely be more carefully chosen ;)

Edit: OK, got it. We are on the same wavelength. The user input is the CSS. The parameter to the function, whilst techically also user input, I think should be considered more as programmer input.

The parameter to the function, whilst also user input, will likely be more carefully chosen ;)

I think w're both on the same side here :)

Let me know if you feel like it normalization should be applied on both.

Sorry, I've only just fully read and understood your comment. Though that doesn't change anything, other than that my inrigue is now piqued as to whether a programmer error in the whitespacing department is covered too (it may well be).

nlemoine · 2023-01-15T13:37:09Z

Thank you both for your reviews and comments. I pushed some changes that will address those issues.

JakeQZ · 2023-09-08T00:02:38Z

Sorry, we both seem to have overlooked this. Maybe GitHub never sent us the notification email (it is notoriously flaky).

I'll try and take a look in the next few days, and remember what it was about.

JakeQZ

Sorry it's taken so long to get back to you.

We've been busy and somehow missed your post-review changes for months.

Hope you're still around and able to complete this change. It would be useful functionality.

You can use \s++ to efficiently match a sequence of one or more whitespace characters in a regular expression. Doing so would greatly simplify this change and avoid the need for an ALL_SPACES constant.

JakeQZ · 2023-09-08T00:28:50Z

src/CssInliner.php

+                    $selector = \str_replace(["\r", "\n", "\t"], ' ', $selector);
+                    $selector = \preg_replace('@[ ' . self::ALL_SPACES . ']+@mu', ' ', $selector);


Surely you can just use \s++ in the regular expression to match all whitespace sequences (without having to reinvent the wheel, so to speak) - ?

(Also, you don't need the m modifier as you are not matching . (any character) at all.)

No problem, regex are not my strong suit, I changed it. Let me know if you see any other issues.

nlemoine · 2023-09-08T08:27:09Z

Sorry it's taken so long to get back to you.

It's open source, no deadline or expectations ;)

@JakeQZ I just made the requested changes, let me know if it works for you.

JakeQZ

There's a few minor issues that need to be addressed, but otherwise looks good.

Oh, and it will need to be rebased.

JakeQZ · 2023-09-08T18:26:10Z

README.md

+* `->addExcludedCssSelector(string $selector)` - Contrary to `addExcludedSelector`
+  which excludes HTML nodes, this method excludes CSS selectors from
+  being inlined. This is for example useful if you don't want your CSS reset rules
+  to be inlined on each HTML node (e.g. `* { margin: 0; padding: 0; font-size: 100% }`).
+  Note that these selectors must precisely match the selectors you wish to exclude.
+  Meaning that excluding `.example` will not exclude `p .example`.
+  ```php
+  $cssInliner->addExcludedCssSelector('*');
+  $cssInliner->addExcludedCssSelector('form');
+  ```


removeExcludedCssSelector should also be (briefly) documented.

JakeQZ · 2023-09-08T18:31:59Z

src/CssInliner.php

+    /**
+     * regular expression component to match all kinds of whitespace
+     *
+     * @see https://github.com/jolicode/JoliTypo/blob/8799bae6cda2280b319750d2fe764bfd8e8f6ba2/src/JoliTypo/Fixer.php#L37
+     *
+     * @var string
+     */
+    private const ALL_SPACES = '\\xE2\\x80\\xAF|\\xC2\\xAD|\\xC2\\xA0|\\s';
+


This isn't needed now.

JakeQZ · 2023-09-08T18:40:26Z

src/CssInliner.php

+                    $selector = \preg_replace('@\s++@u', ' ', $selector);
+                    return $selector;


I'd prefer not to modify the parameter, as this can cause confusion when debugging. Either a different local variable can be assigned and returned, or the result of preg_replace can simply be returned.

Also the \ should be escaped (as \\).

oliverklee · 2023-09-11T13:48:04Z

@nlemoine Could you please rebase, resolve the merge conflicts and repush? Thanks!

nlemoine · 2023-09-19T20:37:38Z

@JakeQZ @oliverklee Sorry, I accidentally screwed up my rebase and this ended in my original PR to be closed. Please see #1236 for requested changes.

JakeQZ · 2023-09-19T22:40:49Z

@JakeQZ @oliverklee Sorry, I accidentally screwed up my rebase and this ended in my original PR to be closed. Please see #1236 for requested changes.

No worries. Thanks for sorting it out. I guess you fell foul at step 7 of https://github.com/MyIntervals/emogrifier/blob/main/.github/CONTRIBUTING.md#rebasing, when git incorrectly suggests you do a pull (which does totally screw up the branch).

oliverklee added the enhancement label Jan 6, 2023

oliverklee assigned nlemoine Jan 6, 2023

oliverklee added this to the 8.0.0 milestone Jan 6, 2023

oliverklee requested changes Jan 6, 2023

View reviewed changes

tests/Unit/CssInlinerTest.php Outdated Show resolved Hide resolved

src/CssInliner.php Outdated Show resolved Hide resolved

JakeQZ requested changes Jan 6, 2023

View reviewed changes

nlemoine force-pushed the feature/excluded-css-selectors branch from c60ce2f to a4abcb2 Compare January 16, 2023 08:04

JakeQZ requested changes Sep 8, 2023

View reviewed changes

nlemoine force-pushed the feature/excluded-css-selectors branch from a4abcb2 to ae1142f Compare September 8, 2023 08:23

JakeQZ requested changes Sep 8, 2023

View reviewed changes

nlemoine force-pushed the feature/excluded-css-selectors branch from 8274ffd to d59a873 Compare September 19, 2023 20:16

nlemoine closed this Sep 19, 2023

nlemoine force-pushed the feature/excluded-css-selectors branch from 007cb28 to 06785d1 Compare September 19, 2023 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add the ability to excluded CSS selectors from being inlined #1202

[FEATURE] Add the ability to excluded CSS selectors from being inlined #1202

nlemoine commented Jan 5, 2023

oliverklee left a comment

JakeQZ left a comment

JakeQZ Jan 6, 2023 •

edited

Loading

nlemoine Jan 15, 2023

nlemoine Jan 15, 2023

nlemoine Jan 15, 2023

JakeQZ Jan 16, 2023 •

edited

Loading

nlemoine Jan 16, 2023 •

edited

Loading

JakeQZ Jan 16, 2023 •

edited

Loading

nlemoine Jan 16, 2023

JakeQZ Jan 16, 2023

nlemoine commented Jan 15, 2023

JakeQZ commented Sep 8, 2023

JakeQZ left a comment

JakeQZ Sep 8, 2023

nlemoine Sep 8, 2023

nlemoine commented Sep 8, 2023

JakeQZ left a comment •

edited

Loading

JakeQZ Sep 8, 2023

nlemoine Sep 19, 2023

JakeQZ Sep 8, 2023

nlemoine Sep 19, 2023

JakeQZ Sep 8, 2023

nlemoine Sep 19, 2023

oliverklee commented Sep 11, 2023

nlemoine commented Sep 19, 2023

JakeQZ commented Sep 19, 2023

		$selector = \str_replace(["\r", "\n", "\t"], ' ', $selector);
		$selector = \preg_replace('@[ ' . self::ALL_SPACES . ']+@mu', ' ', $selector);

		$selector = \preg_replace('@\s++@u', ' ', $selector);
		return $selector;

[FEATURE] Add the ability to excluded CSS selectors from being inlined #1202

[FEATURE] Add the ability to excluded CSS selectors from being inlined #1202

Conversation

nlemoine commented Jan 5, 2023

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment

JakeQZ Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

nlemoine Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

JakeQZ Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlemoine commented Jan 15, 2023

JakeQZ commented Sep 8, 2023

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlemoine commented Sep 8, 2023

JakeQZ left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Sep 11, 2023

nlemoine commented Sep 19, 2023

JakeQZ commented Sep 19, 2023

JakeQZ Jan 6, 2023 •

edited

Loading

JakeQZ Jan 16, 2023 •

edited

Loading

nlemoine Jan 16, 2023 •

edited

Loading

JakeQZ Jan 16, 2023 •

edited

Loading

JakeQZ left a comment •

edited

Loading