Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#58467 closed defect (bug) (fixed)

`wp_get_global_styles`: return the standard format for CSS Custom Properties

Reported by: oandregal's profile oandregal Owned by: oandregal's profile oandregal
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by oandregal)

The wp_get_global_styles functions returns an internal format for CSS Custom Properties instead of the proper CSS format.

Step-by-step reproduction instructions

  • Use a theme that has a theme.json.
  • Paste the following under styles.blocks:
"core/post-terms": {
    "typography": { "fontSize": "var(--wp--preset--font-size--small)" },
    "color":{ "background": "var:preset|color|secondary" }
}
  • Use the wp_get_global_styles functions to retrieve those styles. For example, paste the following in functions.php of the theme:
<?php
add_action( 'init', function(){
        error_log( print_r( wp_get_global_styles( array(), array('block_name'=>'core/post-terms') ), true ) );
} );

The result will be:

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

when it should have been

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

Note the color.background value. It should return the value in the standard CSS Custom Property format, not the shortened internal format.

Change History (17)

This ticket was mentioned in PR #4556 on WordPress/wordpress-develop by @oandregal.


9 months ago
#1

  • Keywords has-patch has-unit-tests added

Trac ticket https://core.trac.wordpress.org/ticket/58467
Backports https://github.com/WordPress/gutenberg/pull/50366 and https://github.com/WordPress/gutenberg/pull/50527
Part of https://github.com/WordPress/gutenberg/issues/45171

## What?

This PR fixes wp_get_global_styles so that it always return the standard format for CSS Custom Properties. For a theme.json dataset with the following data:

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

the wp_get_global_styles function should return:

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

## Why?

See https://github.com/WordPress/gutenberg/issues/49693 The internal syntax shouldn't leak out, so consumers of this function only have to deal with the standard CSS Custom format.

## How?

This PR extract the already existing logic that converts var:preset|color|secondary to var(--wp--preset--font-size--small) to a separate method, then uses the same method to sanitize the output of wp_get_global_styles to only include custom CSS variables and not internal variable syntax.

## Testing Instructions

Make sure the tests pass.

#2 @oandregal
9 months ago

  • Description modified (diff)
  • Keywords has-patch has-unit-tests removed

#3 @oandregal
9 months ago

  • Keywords has-patch has-unit-tests added
  • Milestone changed from Awaiting Review to 6.3

#4 @oandregal
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55959:

wp_get_global_styles: return the standard format for CSS Custom Properties.

Props samnajian, hellofromtonya, isabel_brison.
Fixes #58467.

#6 @spacedmonkey
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#7 @spacedmonkey
8 months ago

@oandregal Do you mind changing 0 === strpos( $value, $prefix ) to str_starts_with? We are trying to get rid of usage of strpos. See #58012.

CC @SergeyBiryukov

Last edited 8 months ago by spacedmonkey (previous) (diff)

#8 @oandregal
8 months ago

Thanks for bringing that to my attention, I wasn't aware we were polyfilling those functions that are only available in PHP8+.

I've looked at that ticket and https://core.trac.wordpress.org/ticket/58012 It seems there is debate about it being the right direction. I've also seen that there has been issues with these changes with load-styles.php due to the polyfills for those functions not being available. It requires wider testing.

The timing is a bit off for me: we're right just before beta 6.3, and this sounds like a risky/involved change to do with no benefits. The opportunity cost is high. I wouldn't mind investigating and preparing an update to more modern PHP coding standards at some point, though it's not the right time for me to help with this refactoring.

#9 @spacedmonkey
8 months ago

@oandregal I have created a PR - https://github.com/WordPress/wordpress-develop/pull/4663. If you can take a look, I will commit it for you.

#10 @oandregal
8 months ago

If you can take a look, I will commit it for you.

Unfortunately, I don't know how I can help with this: my concerns are about testing that it doesn't break anything because the polyfills not being available everywhere (as others pointed out in the ticket and the core-performance channel https://wordpress.slack.com/archives/C02KGN5K076/p1687362710229799).

I cannot justify investing on this right now, so I'd rather hold this sort of changes off for when other work is not such time-sensitive. It's fine by me if others want to pursue it and make sure it works properly.

#11 @spacedmonkey
8 months ago

@oandregal As a core committer of this, you have a responsibility to review and action feedback on your commit. As this is your commit, you should look at the patch and merge it before looking at other work. You have to own the life cycle of a core commit and any feedback that comes out of it.

#12 @oandregal
8 months ago

@oandregal As a core committer of this, you have a responsibility to review and action feedback on your commit. As this is your commit, you should look at the patch and merge it before looking at other work. You have to own the life cycle of a core commit and any feedback that comes out of it.

Absolutely. I just disagree this is the right moment to do that experiment that has proven problematic and performance-wise it has dubious effects (probably negative ones). The direction doesn't have full support from core committers. When it does, I'm happy to follow up with this change and any other that's suggested.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


8 months ago

#14 @oglekler
8 months ago

This ticket was discussed during the bug scrub, and it was agreed that this ticket should be closed in 6.3 milestone and further work need to be moved into another ticket.

@spacedmonkey can you make this, please? 🙏

Props @mrinal, @audrasjb

This ticket was mentioned in Slack in #core by oglekler. View the logs.


8 months ago

#16 @spacedmonkey
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

This ticket was fixed in other commits.

Note: See TracTickets for help on using tickets.