-
Notifications
You must be signed in to change notification settings - Fork 799
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
CSSTidy: add support for css variable definition #19935
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 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. Jetpack plugin:
|
projects/plugins/jetpack/modules/custom-css/csstidy/class.csstidy.php
Outdated
Show resolved
Hide resolved
With Source: <!-- wp:group {"style":{"color":{"link":"var:preset|color|vivid-red"}}} -->
<div class="wp-block-group has-link-color" style="--wp--style--color--link:var(--wp--preset--color--vivid-red)"></div>
<!-- /wp:group --> Before this patch, the result after save was:
After this patch the HTML remained intact. 😄 Also checked
Would it make sense to add some unit tests so we can prove that it will pass in various scenarios? |
Good idea, will try and add some tests. |
projects/plugins/jetpack/modules/custom-css/csstidy/class.csstidy.php
Outdated
Show resolved
Hide resolved
f171c17 updates the regex to accept all alphanumeric variable names separated by Things like Also updated the tests 👍 |
Thanks @ramonjd, the changes tested well for me. |
@@ -0,0 +1,67 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding some tests! ❤️
@@ -1,4 +1,5 @@ | |||
<?php | |||
// phpcs:ignoreFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason while you set to ignore the whole file? It's full of errors at the moment, but maybe we can stick to our coding standards for new code we introduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I wasn't sure how to handle it given it was original a 3rd party file, but given it is never going to be updated externally now you are probably right, so have removed the ignore.
@jeherve Do you think this is something we should run past secops? I can't see how this would open up any sort of xss hole, etc. but is it worth getting a second opinion on it anyway? |
I have something similar over here: #19993 Will be adding tests but it'd be good to get a 👍 just in case. |
projects/plugins/jetpack/tests/php/modules/csstidy/test-class.jetpack-csstidy.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/tests/php/modules/csstidy/test-class.jetpack-csstidy.php
Outdated
Show resolved
Hide resolved
That'd be my vote as well 👍 |
…hanumeric values and that don't end in a separator. Updating test cases and all tests to use a data provider.
…idy/test-class.jetpack-csstidy.php Correct argument order Co-authored-by: Ramon <[email protected]>
…idy/test-class.jetpack-csstidy.php Correct argument order Co-authored-by: Ramon <[email protected]>
7422cee
to
8d7d1e5
Compare
The change that removed this requirement from the group block is now merged in Gutenberg. It turned out that the other blocks that use css vars use |
Fixes issue with block validations when css variables used, eg with group block custom color. Although this is fixed in Gutenberg 10.7 for the group block, there are a number of other blocks that support the css vars for custom colours and it may be a few releases before they are are updated.
There has been previous discussion about the removal of css tidy, and given the age of this library that is probably worth looking at - and maybe a more modern and maintained library like https://github.com/wikimedia/css-sanitizer could be investigated, but as previously discussed this is a much bigger project so adding this css var support may still make sense in the short term.
This would also currently allow the use of css vars in the custom css in the customizer - would that be a bad/dangerous thing?
Changes proposed in this Pull Request:
Does this pull request change what data or activity we track or use?
no
Testing instructions:
<div style="--base-color: red;color:var(--base-color)">test css var preservation</div>
--base-color
property has been preserved in editor and front end.