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

Fix/wp get global styles for custom props returns internal variable #50366

Merged
merged 11 commits into from
May 10, 2023
Merged

Fix/wp get global styles for custom props returns internal variable #50366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5a53d35
Extract logic that converts internal props to css variables
samnajian May 4, 2023
de1f077
Convert the internal css variables to css custom variables
samnajian May 4, 2023
02fb5c2
Add test to assert internal css syntax is converted to CSS variable
samnajian May 5, 2023
b4b0823
Define gutenberg_get_global_styles and update usages
samnajian May 5, 2023
b035fb6
Make sure variation styles are includes when sanitizing
samnajian May 7, 2023
9564374
Add correct since annotation to gutenberg_get_global_styles
samnajian May 8, 2023
a6b0e78
Add descriptive since annotation to gutenberg_get_global_styles
samnajian May 8, 2023
a19f470
Remove usage of convert_custom_properties from get_property_value
samnajian May 8, 2023
1250ec9
Apply improvements suggested in PR review
samnajian May 8, 2023
5833401
Simplify sanitize_variables and update usage
samnajian May 9, 2023
92ed17b
Rename sanitize_variables to resolve_custom_css_format
samnajian May 9, 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
63 changes: 48 additions & 15 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,7 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n
if ( empty( $result ) ) {
unset( $output[ $subtree ] );
} else {
$output[ $subtree ] = $result;
$output[ $subtree ] = static::resolve_custom_css_format( $result );
}
}

Expand Down Expand Up @@ -1989,20 +1989,6 @@ protected static function get_property_value( $styles, $path, $theme_json = null
return $value;
}

// Convert custom CSS properties.
$prefix = 'var:';
$prefix_len = strlen( $prefix );
$token_in = '|';
$token_out = '--';
if ( 0 === strncmp( $value, $prefix, $prefix_len ) ) {
$unwrapped_name = str_replace(
$token_in,
$token_out,
substr( $value, $prefix_len )
);
$value = "var(--wp--$unwrapped_name)";
}

return $value;
}

Expand Down Expand Up @@ -3578,4 +3564,51 @@ protected function get_feature_declarations_for_node( $metadata, &$node ) {

return $declarations;
}

/**
* This is used to convert the internal representation of variables to the CSS representation.
* For example, `var:preset|color|vivid-green-cyan` becomes `var(--wp--preset--color--vivid-green-cyan)`.
*
* @since 6.3.0
* @param string $value The variable such as var:preset|color|vivid-green-cyan to convert.
* @return string The converted variable.
*/
private static function convert_custom_properties( $value ) {
$prefix = 'var:';
$prefix_len = strlen( $prefix );
$token_in = '|';
$token_out = '--';
if ( 0 === strpos( $value, $prefix ) ) {
$unwrapped_name = str_replace(
$token_in,
$token_out,
substr( $value, $prefix_len )
);
$value = "var(--wp--$unwrapped_name)";
}

return $value;
}

/**
* Given a tree, converts the internal representation of variables to the CSS representation.
* It is recursive and modifies the input in-place.
*
* @since 6.3.0
* @param array $tree Input to process.
* @return array The modified $tree.
*/
private static function resolve_custom_css_format( $tree ) {
$prefix = 'var:';

foreach ( $tree as $key => $data ) {
if ( is_string( $data ) && 0 === strpos( $data, $prefix ) ) {
$tree[ $key ] = self::convert_custom_properties( $data );
} elseif ( is_array( $data ) ) {
$tree[ $key ] = self::resolve_custom_css_format( $data );
}
}

return $tree;
}
}
34 changes: 34 additions & 0 deletions lib/compat/wordpress-6.3/get-global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,37 @@ function wp_get_block_css_selector( $block_type, $target = 'root', $fallback = f
function gutenberg_get_remote_theme_patterns() {
return WP_Theme_JSON_Resolver_Gutenberg::get_theme_data( array(), array( 'with_supports' => false ) )->get_patterns();
}

/**
* Gets the styles resulting of merging core, theme, and user data.
*
* @since 5.9.0
* @since 6.3.0 the internal link format "var:preset|color|secondary" is resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Once introduced in WP Core, this modifies the value format for the returned styles from wp_get_global_styles(). As such, it is a backwards-compatibility (BC) break for extenders expecting and using this structure.

The result could mean broken styling experiences for users.

Is this a truly a bugfix, meaning wp_get_global_styles() should always have returned the style value structure? Or is this a new improvement to avoid leaking internal structures?

cc @oandregal

Copy link
Member

@oandregal oandregal May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is why I think it's a fine change.

Let say we had the following input:

"core/post-terms": {
    "typography": { "fontSize": "var(--wp--preset--font-size--small)" },
    "color":{ "background": "var:preset|color|secondary" }
}

Before this PR, the output of wp_get_global_styles would be:

(
    [typography] => Array( [fontSize] => var(--wp--preset--font-size--small) )
    [color] => Array( [background] => var:preset|color|secondary )
)

After this PR, the output of wp_get_global_styles would be:

(
    [typography] => Array( [fontSize] => var(--wp--preset--font-size--small) )
    [color] => Array( [background] => var(--wp--preset--color--secondary) )
)

Before this PR, the consumers had to deal with two formats: var(--wp--preset--color--secondary) and var:preset|color|secondary, as a possible representation of the same value.

After this PR, they only have to deal with one format that's been always present: var(--wp--preset--color--secondary). I don't remember when the var:preset|color|secondary was introduced for people to use in theme.json but it was later and added as a convenience. It should have never leaked to consumers, in my view. I did some quick search but didn't find when it was, I can do some archeology if this is important.

An important note is that the format doesn't carry any meaning about where this data comes from. Any origin (default, block, theme, user) can use var:preset|color|secondary. Consumers cannot use that format as a hint of it being data from the user origin.

Does this help?

* to "var(--wp--preset--font-size--small)" so consumers don't have to.
*
* @param array $path Path to the specific style to retrieve. Optional.
* If empty, will return all styles.
* @param array $context {
* Metadata to know where to retrieve the $path from. Optional.
*
* @type string $block_name Which block to retrieve the styles from.
* If empty, it'll return the styles for the global context.
* @type string $origin Which origin to take data from.
* Valid values are 'all' (core, theme, and user) or 'base' (core and theme).
* If empty or unknown, 'all' is used.
* }
* @return array The styles to retrieve.
*/
function gutenberg_get_global_styles( $path = array(), $context = array() ) {
if ( ! empty( $context['block_name'] ) ) {
$path = array_merge( array( 'blocks', $context['block_name'] ), $path );
}

$origin = 'custom';
if ( isset( $context['origin'] ) && 'base' === $context['origin'] ) {
$origin = 'theme';
}
$styles = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( $origin )->get_raw_data()['styles'];

return _wp_array_get( $styles, $path, $styles );
}
2 changes: 1 addition & 1 deletion lib/experimental/block-editor-settings-mobile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function gutenberg_get_block_editor_settings_mobile( $settings ) {
'mobile' === $_GET['context']
) {
if ( wp_theme_has_theme_json() ) {
$settings['__experimentalStyles'] = wp_get_global_styles();
$settings['__experimentalStyles'] = gutenberg_get_global_styles();
}

// To tell mobile that the site uses quote v2 (inner blocks).
Expand Down
65 changes: 65 additions & 0 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2071,4 +2071,69 @@ public function data_process_blocks_custom_css() {
),
);
}

public function test_internal_syntax_is_converted_to_css_variables() {
$result = new WP_Theme_JSON_Gutenberg(
array(
'version' => WP_Theme_JSON_Gutenberg::LATEST_SCHEMA,
'styles' => array(
oandregal marked this conversation as resolved.
Show resolved Hide resolved
'color' => array(
'background' => 'var:preset|color|primary',
'text' => 'var(--wp--preset--color--secondary)',
),
'elements' => array(
'link' => array(
'color' => array(
'background' => 'var:preset|color|pri',
'text' => 'var(--wp--preset--color--sec)',
),
),
),
'blocks' => array(
'core/post-terms' => array(
'typography' => array( 'fontSize' => 'var(--wp--preset--font-size--small)' ),
'color' => array( 'background' => 'var:preset|color|secondary' ),
),
'core/navigation' => array(
'elements' => array(
'link' => array(
'color' => array(
'background' => 'var:preset|color|p',
'text' => 'var(--wp--preset--color--s)',
),
),
),
),
'core/quote' => array(
'typography' => array( 'fontSize' => 'var(--wp--preset--font-size--d)' ),
'color' => array( 'background' => 'var:preset|color|d' ),
'variations' => array(
'plain' => array(
'typography' => array( 'fontSize' => 'var(--wp--preset--font-size--s)' ),
'color' => array( 'background' => 'var:preset|color|s' ),
),
),
),
),
),
)
);
$styles = $result->get_raw_data()['styles'];

$this->assertEquals( 'var(--wp--preset--color--primary)', $styles['color']['background'], 'Top level: Assert the originally correct values are still correct.' );
$this->assertEquals( 'var(--wp--preset--color--secondary)', $styles['color']['text'], 'Top level: Assert the originally correct values are still correct.' );

$this->assertEquals( 'var(--wp--preset--color--pri)', $styles['elements']['link']['color']['background'], 'Element top level: Assert the originally correct values are still correct.' );
$this->assertEquals( 'var(--wp--preset--color--sec)', $styles['elements']['link']['color']['text'], 'Element top level: Assert the originally correct values are still correct.' );

$this->assertEquals( 'var(--wp--preset--font-size--small)', $styles['blocks']['core/post-terms']['typography']['fontSize'], 'Top block level: Assert the originally correct values are still correct.' );
$this->assertEquals( 'var(--wp--preset--color--secondary)', $styles['blocks']['core/post-terms']['color']['background'], 'Top block level: Assert the internal variables are convert to CSS custom variables.' );

$this->assertEquals( 'var(--wp--preset--color--p)', $styles['blocks']['core/navigation']['elements']['link']['color']['background'], 'Elements block level: Assert the originally correct values are still correct.' );
$this->assertEquals( 'var(--wp--preset--color--s)', $styles['blocks']['core/navigation']['elements']['link']['color']['text'], 'Elements block level: Assert the originally correct values are still correct.' );

$this->assertEquals( 'var(--wp--preset--font-size--s)', $styles['blocks']['core/quote']['variations']['plain']['typography']['fontSize'], 'Style variations: Assert the originally correct values are still correct.' );
$this->assertEquals( 'var(--wp--preset--color--s)', $styles['blocks']['core/quote']['variations']['plain']['color']['background'], 'Style variations: Assert the internal variables are convert to CSS custom variables.' );

}
}