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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Feb 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08d51b9
Duotone: Use the style engine to generate CSS for Duotone
scruffian Feb 21, 2023
8dddbd2
filter for kses to allow filter css rule with url if url is of wp-duo…
draganescu Feb 21, 2023
e1ab155
comment for how style engine call works and match expression for filt…
draganescu Feb 22, 2023
c58213c
Update lib/block-supports/duotone.php
scruffian Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions lib/block-supports/duotone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,13 +464,19 @@ function gutenberg_render_duotone_support( $block_content, $block ) {

// !important is needed because these styles render before global styles,
// and they should be overriding the duotone filters set by global styles.
$filter_style = SCRIPT_DEBUG
? $selector . " {\n\tfilter: " . $filter_property . " !important;\n}\n"
: $selector . '{filter:' . $filter_property . ' !important;}';

wp_register_style( $filter_id, false, array(), true, true );
wp_add_inline_style( $filter_id, $filter_style );
wp_enqueue_style( $filter_id );
gutenberg_style_engine_get_stylesheet_from_css_rules(
array(
array(
'selector' => $selector,
'declarations' => array(
'filter' => $filter_property . ' !important',
),
),
),
array(
'context' => 'block-supports',
)
);
draganescu marked this conversation as resolved.
Show resolved Hide resolved

if ( 'unset' !== $colors ) {
$filter_svg = gutenberg_get_duotone_filter_svg( $filter_preset );
Expand Down
19 changes: 19 additions & 0 deletions lib/experimental/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,22 @@ function gutenberg_override_core_kses_init_filters() {
// The 'kses_init_filters' is usually initialized with default priority. Use higher priority to override.
add_action( 'init', 'gutenberg_override_core_kses_init_filters', 20 );
add_action( 'set_current_user', 'gutenberg_override_core_kses_init_filters' );

/**
* See https://github.com/WordPress/wordpress-develop/pull/4108
*
* Mark CSS safe if it contains a "filter: url('#wp-duotone-...')" rule.
*
* This function should not be backported to core.
*
* @param bool $allow_css Whether the CSS is allowed.
* @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

return true;
}
return $allow_css;
}

add_filter( 'safecss_filter_attr_allow_css', 'allow_filter_in_styles', 10, 2 );