Customizer > Custom CSS: Add support for CSS variables

[danieldudzic]

This update modifies the CSSTidy script following the example of #19935

Fixes #19669

Changes proposed in this Pull Request:

Adds support for CSS variables in Custom CSS

Jetpack product discussion

p1623934752160800-slack-calypso

Does this pull request change what data or activity we track or use?

No

Testing instructions:

1. Open the Customizer and try to add CSS variables in the Additional CSS.
2. Save
3. Make sure the CSS variables have been saved (not stripped) and they apply to the frontend

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello danieldudzic! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D63085-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Jetpack plugin:

• Next scheduled release: May 2, 2023.
• Scheduled code freeze: April 24, 2023.

[scruffian]

I added the unit tests for this from #19935. I tried to run them with jetpack docker phpunit but it didn't work. @jeherve @glendaviesnz any ideas for getting these tests running?

[scruffian]

This line is really hard to grok, could we extract it to a function?

[scruffian]

I also added a changelog entry.

[glendaviesnz]

I added the unit tests for this from #19935. I tried to run them with jetpack docker phpunit but it didn't work. @jeherve @glendaviesnz any ideas for getting these tests running?

Sorry @scruffian I didn't get chance to look at this as was busy with another priority all week, but should be able to take a look monday if you haven't sorted by then.

[jeherve]

I added the unit tests for this from #19935. I tried to run them with jetpack docker phpunit but it didn't work. @jeherve @glendaviesnz any ideas for getting these tests running?

You should be able to run them like so:

jetpack docker phpunit -- --filter=WP_Test_Jetpack_CSSTidy

Here are the results I get from the branch at the moment:

There were 3 failures:

1) WP_Test_Jetpack_CSSTidy::test_preserve_leading_zeros with data set "test_removes_leading_zeros_by_default" ('marquee {line-height:0.7;opac....25);}', 'marquee {\nline-height:.7;\no...25)\n}', false)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 'marquee {\n
 line-height:.7;\n
-opacity:.05;\n
 background-color:rgba(255,255,255,0.25)\n
 }'

/usr/local/src/jetpack-monorepo/projects/plugins/jetpack/tests/php/modules/csstidy/test-class.jetpack-csstidy.php:62

2) WP_Test_Jetpack_CSSTidy::test_preserve_leading_zeros with data set "test_preserves_leading_zeros" ('aside {line-height:0.7;backgr...0.05;}', 'aside {\nline-height:0.7;\nba....05\n}', true)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 'aside {\n
 line-height:0.7;\n
-background-color:rgba(255,255,255,0.25);\n
-opacity:0.05\n
+background-color:rgba(255,255,255,0.25)\n
 }'

/usr/local/src/jetpack-monorepo/projects/plugins/jetpack/tests/php/modules/csstidy/test-class.jetpack-csstidy.php:62

3) WP_Test_Jetpack_CSSTidy::test_custom_property_patterns with data set "test_invalid_css_properties_removed" ('div {--$//2343--3423:red;colo...3423)}', 'div {\ncolor:var(--$//2343--3423)\n}', true)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 'div {\n
+--2343--3423:red;\n
 color:var(--$//2343--3423)\n
 }'

/usr/local/src/jetpack-monorepo/projects/plugins/jetpack/tests/php/modules/csstidy/test-class.jetpack-csstidy.php:96

FAILURES!
Tests: 14, Assertions: 14, Failures: 3.

You can also see them in the checks here on GitHub:
https://github.com/Automattic/jetpack/pull/20129/checks?check_run_id=2894766149

[danieldudzic]

FYI - I have modified the original regex: https://github.com/Automattic/jetpack/pull/19935/files#diff-09336221a25ca5f791ce459fbe1f11846ebd8ace76220d89eec08dca03591b70R1196 to allow for "nested" variables like, for example: --wp--custom--color--foreground

[scruffian]

I still can't get the tests to run. I see this error:

Could not read "/var/www/html/wp-content/plugins/jetpack/phpunit.xml.dist"

[scruffian]

I managed to get this working thanks to @brbrr! I added a commit to fix the unit tests. I think this is ready for another review.

[jffng]

The unit tests specific to this module are passing for me on this PR, but I don't fully understand how to replicate the issue. That is, CSS variables added via Customizer CSS apply on my development instance both with and without this PR, which makes me think I may be not testing this properly.

[samiff]

@danieldudzic Like Jeff, I was able to add/save CSS variables in the Customizer without issue on both my local dev and WoA test site, without applying the changes in this PR 🤔

I was testing with this CSS if it helps:

:root {
  --main-bg-color: purple;
}

body {
  background-color: var(--main-bg-color);
}

[scruffian]

Danny is on leave until september.

[andrewserong]

@scruffian @jffng @samiff would any of you mind if I take over this PR, give it a rebase, and add an additional line? I think this PR will unblock being able to introduce blocks that store CSS variables in inline styles in post content, which will come in very handy for future features like introducing gap block support (here's a WIP in Gutenberg that would be unblocked by this PR).

The main change will be to add a line to the Jetpack_Safe_CSS class's filter_attr function to switch on preserve_css_variables, which in my testing appears to resolve the issue. (So, a similar change as to the one in #19993 that introduced preserving leading decimal zeros in Gutenberg).

If there are no objections to me taking over this PR, I can then come up with some testing instructions to reproduce the issue and fix on wpcom 🤞

[andrewserong]

I may have spoken too soon, as this PR would only resolve the issue of savings CSS attributes in post content for CSSTidy, and not in core for users without the unfiltered_html capability. I'll do some more thinking!

[scruffian]

Happy for you to take it over :)

[andrewserong]

It turns out there were some good arguments for not switching on arbitrary CSS variables in post content (discussion here), so for my use case, I'll look into server rendering CSS variables instead. I think this means I won't need to pursue the change in this PR after all, so I'll most likely put it back after only briefly picking it up 😄

Apologies for the false alarm!

[fullofcaffeine]

Hi folks! 👋🏻 What's preventing us from merging these additions?

Supporting CSS variables in the CSS customizer is a commonly-requested feature (see Automattic/wp-calypso#62631) and would be very useful.

I've read the security concerns from @jorgefilipecosta, but would that apply to the Custom CSS, which is a feature that only the user (or other authorized actors) have access to? Or is that because CSSTidy is currently used beyond the Custom CSS feature?* AFAIK, the only feature that officially uses CSSTidy in org, is the Custom CSS feature?

[jorgefilipecosta]

I've read the security concerns from @jorgefilipecosta, but would that apply to the Custom CSS, which is a feature that only the user (or other authorized actors) have access to? Or is that because CSSTidy is currently used beyond the Custom CSS feature?* AFAIK, the only feature that officially uses CSSTidy in org, is the Custom CSS feature?

The security concerns I raised in the past were not directly related to allow CSS vars on the custom CSS field, it was

Hi folks! 👋🏻 What's preventing us from merging these additions?

Supporting CSS variables in the CSS customizer is a commonly-requested feature (see Automattic/wp-calypso#62631) and would be very useful.

I've read the security concerns from @jorgefilipecosta, but would that apply to the Custom CSS, which is a feature that only the user (or other authorized actors) have access to? Or is that because CSSTidy is currently used beyond the Custom CSS feature?* AFAIK, the only feature that officially uses CSSTidy in org, is the Custom CSS feature?

Yes, I think my concerns do not apply to the custom CSS field. These users already have the ability to totally customize the site anyway. It may have been problematic if a user like a contributor using the editor and a group block can use arbitrary CSS variables because that user does not has customization capabilities, in the custom CSS case the user has customization capabilities so it is not the same case.

[ramonjd]

Not customizer related per se, but just to also note that this Gutenberg PR – now part of release 13.2 – introduced CSS vars to control individual borders in the Block Editor.

So rules such as border-right-color: var(--wp--preset--color--pale-pink) will appear inline.

Currently they're being stripped out:

Editor

Frontend

Is there a robust way to allow them for the editor only?

[edequalsawesome]

Updating CSS variables appears to work now on self-hosted and WoA sites, but the version of CSS Tidy on WordPress.com sites still doesn't work, which came up in a ping earlier today p1658744878657319 -- would merging this PR also update the version that runs checks against WordPress.com CSS? Or would it make more sense to open a different GitHub issue for that?

[samiff]

Given that CSS variables are working on self hosted and WoA sites, I'm not sure if this particular PR is still relevant.

For reference, there are some CSS issues for simple WordPress.com sites catalogued here: Automattic/wp-calypso#61232

@fullofcaffeine given the work outlined in p9dueE-4t8-p2 do you think we can close this PR out?

[fullofcaffeine]

@fullofcaffeine given the work outlined in p9dueE-4t8-p2 do you think we can close this PR out?

Sorry for the delay here. Yes, I think we can close this as D70703-code will take care of the underlying problem for simple sites once it's shipped. Thanks for working on this anyway! It served as a good reference on how to modify CSSTidy :)

[ryelle]

@samiff @fullofcaffeine Sorry to ping you on an older PR, but how was this fixed? Custom properties are still being stripped on WordCamp sites, but we do some custom processing using Jetpack's sanitization functions. Is it just a matter of removing those and falling back to the core CSS handling?

[iandunn]

It looks like D70703-code just removed a WPCOM hack, and at a quick glance it looks like D94103-code just removed some unused WPCOM code. So #19669 hasn't been fixed yet AFAICT.

[iandunn]

@samiff , @fullofcaffeine : Are you seeing it work on self-hosted installs with unfiltered_html disabled? It doesn't appear to for me:

1. Add define( 'DISALLOW_UNFILTERED_HTML', true ); to wp-config.php
2. Add this to Additional CSS:

   body {
   	--mycolor: blue;
   	border: 40px solid var(--mycolor) !important;
   }

3. It should look fine in the Customizer. Click Publish.
4. Reload the customizer and you'll see the var definition has been stripped out.

It works when you have unfiltered_html because of:

jetpack/projects/plugins/jetpack/modules/custom-css/custom-css.php

Lines 114 to 119 in 9f80daa

	* Core, by default, restricts this to users that have `unfiltered_html` which
	* would make the feature unusable in multi-site by non-super-admins, due to Core
	* not shipping any solid sanitization.
	*
	* We're expanding who can use it, and then conditionally applying CSSTidy
	* sanitization to users that do not have the `unfiltered_html` capability.

jetpack/projects/plugins/jetpack/modules/custom-css/custom-css.php

Line 507 in 9f80daa

if ( $args['force'] || ! current_user_can( 'unfiltered_html' ) ) {