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

Duotone: Use the style engine to generate CSS for Duotone #48281

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Feb 21, 2023

What?

This changes the custom approach for block supports to use the style engine. This means that the CSS is output as part of the block supports CSS block rather than in its own inline style.

Why?

It's better to use the same code to generate CSS as its more efficient. It also will be slightly more performant as the output on the client is a little smaller.

How?

Using gutenberg_style_engine_get_stylesheet_from_css_rules

Testing Instructions

  1. Apply this core patch: wp_kses: Add filter to the list of css_url_data_types wordpress-develop#4108
  2. Add an image block to a post
  3. Add a duotone filter to the image block
  4. Open the post in the frontend and confirm that the image block has the correct filter applied.

Testing Instructions for Keyboard

Check that the image has CSS applied to it in the form:
filter: url('#wp-duotone-*') !important;

Note

Requires this core patch: WordPress/wordpress-develop#4108. I don't see a way to get this fix in to Gutenberg before its in core...

@scruffian scruffian added the [Package] Style Engine /packages/style-engine label Feb 21, 2023
@scruffian scruffian self-assigned this Feb 21, 2023
@github-actions
Copy link

github-actions bot commented Feb 21, 2023

Flaky tests detected in e1ab155.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4240273800
📝 Reported issues:

@getdave
Copy link
Contributor

getdave commented Feb 21, 2023

I don't see a way to get this fix in to Gutenberg before its in core...

Agreed. This allow urls declaration isn't filterable.

* @param string $css_test_string The CSS to test.
*/
function allow_filter_in_styles( $allow_css, $css_test_string ) {
if ( strpos( $css_test_string, "filter: url('#wp-duotone" ) !== false ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: does this mean I could create a CSS string that included "filter: url('#wp-duotone" in order to allow any CSS rule to bypass the filtering rules.

Assume we'll want to ditch this asap in order to get the Core patch landed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we should

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but it's not in order to get the core patch landed. the core patch should land for the reasons described by @scruffian in the ticket. This is here so we can work on this without core trunk

Copy link
Contributor

Choose a reason for hiding this comment

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

In core, when a url attribute is used, the value is parsed with some more complex regex (here: https://github.com/WordPress/wordpress-develop/blob/365efb13c2f2f9de572e53a4c115a0d59232e54f/src/wp-includes/kses.php#L2525). Would it be safer to copy + paste the regexes from there so that we can ensure the same level of parsing occurs for the the filter: rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( strpos( $css_test_string, "filter: url('#wp-duotone" ) !== false ) {
if ( preg_match( "^filter: url\('#wp-[-a-zA-Z0-9]+'\);$", $css_test_string ) ) {

Might be a bit safer to use a a regex and only allow 100% correct urls rather than just a strpos check

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like an elegant way to do it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't test that regex, so it might need some tweaks if it isn't processing line by line or newlines have already been stripped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point. I think the string in preg_match will need a/ delimiter at the beginning and end, too, to flag that it's a pattern, but overall I think that's a better and simpler way to do it than copying the more complex regex from core.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm with a good preg match we could in theory even live with this in the plugin for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in e1ab155

@scruffian
Copy link
Contributor Author

Thanks for the fixes @draganescu, I'm happy for this to merge if you are :)

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Let's merge this and see where it gets us!

@draganescu draganescu merged commit 2fbe9a0 into trunk Feb 22, 2023
@draganescu draganescu deleted the update/use-style-engine-for-duotone-block-supports branch February 22, 2023 12:56
@github-actions github-actions bot added this to the Gutenberg 15.3 milestone Feb 22, 2023
@DaisyOlsen DaisyOlsen added the [Type] Enhancement A suggestion for improvement. label Feb 22, 2023
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Mar 20, 2023
…()`.

CSS filters can accept `url()` as a reference to an SVG filter element:
{{{
filter: url( file.svg#filter-element-id );
}}}
This commit allows for that syntax to be used in inline CSS.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#48281 #48281 Duotone: Use the style engine to generate CSS for Duotone]

References:
* [https://developer.mozilla.org/en-US/docs/Web/CSS/filter MDN Web Docs: filter()]
* [https://developer.mozilla.org/en-US/docs/Web/CSS/url MDN Web Docs: url()]

Follow-up to [44136], [52049].

Props scruffian, jeryj, ironprogrammer, azaozz, hellofromTonya, SergeyBiryukov.
Fixes #57780.
Built from https://develop.svn.wordpress.org/trunk@55564


git-svn-id: http://core.svn.wordpress.org/trunk@55076 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Mar 20, 2023
…()`.

CSS filters can accept `url()` as a reference to an SVG filter element:
{{{
filter: url( file.svg#filter-element-id );
}}}
This commit allows for that syntax to be used in inline CSS.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#48281 #48281 Duotone: Use the style engine to generate CSS for Duotone]

References:
* [https://developer.mozilla.org/en-US/docs/Web/CSS/filter MDN Web Docs: filter()]
* [https://developer.mozilla.org/en-US/docs/Web/CSS/url MDN Web Docs: url()]

Follow-up to [44136], [52049].

Props scruffian, jeryj, ironprogrammer, azaozz, hellofromTonya, SergeyBiryukov.
Fixes #57780.
Built from https://develop.svn.wordpress.org/trunk@55564


git-svn-id: https://core.svn.wordpress.org/trunk@55076 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@ironprogrammer
Copy link
Contributor

For visibility: Core support for URLs in CSS filter has been added to 6.3 trunk (also see WordPress/wordpress-develop#4108 (comment)).

@scruffian
Copy link
Contributor Author

I created a follow to remove the filter but I think we'll need it for a while yet: #49488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants