Global Styles: kebab-case preset slugs when converting references to custom properties - #80583
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +250 B (0%) Total Size: 7.75 MB 📦 View Changed
|
9af04a6 to
a9bed26
Compare
080b939 to
f56e5a2
Compare
f56e5a2 to
8b57be5
Compare
8b57be5 to
7660324
Compare
0a13f68 to
fe6424d
Compare
|
Flaky tests detected in 4b41da4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/30922290165
|
fe6424d to
983d964
Compare
ramonjd
left a comment
There was a problem hiding this comment.
Testing well for me. Checked a bunch of preset configurations.
Not a blocker, but covering the duotone changes might be worth adding back in.
How bad is the bug? I believe 7.1 is still in beta if it's urgent.
Cheers!
| /* | ||
| * The slug of a preset reference is kebab-cased so the resulting | ||
| * custom property matches the one generated from the preset, | ||
| * whose slug is also kebab-cased (see `get_settings_values_by_slug()`). |
There was a problem hiding this comment.
Optional and a nit: For duotone, custom properties are generated separately in WP_Duotone_Gutenberg::get_all_global_styles_presets(), right?
There was a problem hiding this comment.
Hi @ramonjd, yes, you are right 👍 theme.json itself never generates the duotone custom properties. The comment was updated to note the exception.
| * not exist (`--wp--preset--font-family--n27` instead of the | ||
| * generated `--wp--preset--font-family--n-27`). | ||
| */ | ||
| if ( 3 === count( $parts ) && 'preset' === $parts[0] ) { |
There was a problem hiding this comment.
After removing 'duotone' !== $parts[1] is it still worth adding a duotone case to the tests? Or putting back the blueOrange2 tests?
There was a problem hiding this comment.
Good point, the blueOrange2 case was restored.
…custom properties WP_Theme_JSON_Gutenberg kebab-cases preset slugs when generating the preset custom properties (get_settings_values_by_slug()), but convert_custom_properties() converts stored references (var:preset|font-family|n27) with a verbatim | to -- replacement. For a slug that changes when kebab-cased, the reference (--wp--preset--font-family--n27) never matches the generated custom property (--wp--preset--font-family--n-27), so the preset does not apply on the front end. Kebab-case the slug of preset references in convert_custom_properties() with the same _wp_to_kebab_case() used for the generated custom properties. References that render correctly today are unchanged: they only work when the slug is already a kebab-case fixed point, on which _wp_to_kebab_case() is the identity. Duotone references keep the raw slug: they are resolved by slug lookup in WP_Duotone, not through the generated custom properties.
983d964 to
feb17b7
Compare
Thank you a lot for the review @ramonjd, I think all your feedback was addressed. This bug was not introduced in 7.1, it is an old one affecting all versions since WordPress 5.8, so it could not be included in a beta as it is not a regression of the release. It will be part of 7.2. |
Ah great, thanks for the confirmation! 🚢 |
…custom properties (#80583) Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
What?
Fixes #53695.
When a preset slug changes under kebab-casing (e.g. a font family with slug
n27), styles referencing that preset from theme.json or Global Styles never apply on the front end. The same mismatch affects every preset type referenced this way: colors, gradients, font sizes, font families, spacing, shadows.This PR fixes it in the one place responsible: the PHP conversion of stored preset references to CSS custom properties.
Why?
PHP kebab-cases the slug in one place but not in the other:
When generating the preset custom properties, the slug is kebab-cased with
_wp_to_kebab_case()— seeget_settings_values_by_slug():When converting stored references (
var:preset|font-family|n27) tovar()usages,convert_custom_properties()performs a verbatim|→--string replacement with no kebab-casing:The declared name has
n-27, the reference asks forn27, so the browser finds no such custom property and the style falls back. For slugs that don't change under kebab-casing (vivid-red,x-large,40) the two paths coincidentally agree, which is why the bug only surfaces with slugs liken27.convert_custom_properties()is the odd one out among all the serializers: the PHP block supports / style engine (wp_typography_get_preset_inline_style_value(),WP_Style_Engine::get_slug_from_preset_value()) kebab-case — which is why block-level markup renders correctly — and so do the client-side style engine and global styles engine, which is why the editor canvas renders correctly. Only this Global Styles reference conversion disagrees.How?
convert_custom_properties()now kebab-cases the slug segment ofvar:preset|<type>|<slug>references with the same_wp_to_kebab_case()the custom property generation uses, so both sides always agree. Only exact three-segmentpresetreferences are touched; any othervar:value converts byte-identically to before.Backward compatibility
References that render correctly today are provably unchanged: a reference only works today when its raw slug already equals the kebab-cased declaration name (i.e. the slug is a kebab-case fixed point), and
_wp_to_kebab_case()is the identity on exactly those slugs. Since both sides use the same function, the change can only converge a reference toward its declaration — never move a matching one away.The behavioral deltas are limited to references that are currently dead:
As a side effect,
WP_Theme_JSON::resolve_variables()can now actually resolve these references (previously the constructed name didn't exist in the computed vars).The editor is unaffected: the conversion runs when a config is sanitized for rendering; the global styles REST controller returns raw stored user data, so the client keeps seeing the same
var:preset|…values it does today. Since references keep being stored with raw slugs (no data format change), nothing changes for existing content beyond the fixed rendering.Core sync PR: WordPress/wordpress-develop#12656.
Testing Instructions
theme.json, register a font family whose slug changes when kebab-cased, e.g.:{ "fontFamily": "N27, sans-serif", "name": "N27", "slug": "n27" }font-familynow referencesvar(--wp--preset--font-family--n-27), matching the generated custom property, and the font applies. Without this PR the reference is--wp--preset--font-family--n27and the font falls back. Note that with this fix, previously saved (broken) references heal without a re-save, since the fix is applied at render time.PHP tests:
npm run test:unit:php:base -- -- --filter WP_Theme_JSON_Gutenberg_Test(222 tests passing, including new coverage asserting the kebab-cased reference output for font family and spacing). The full PHP suite was compared against trunk: identical results (all pre-existing failures, none introduced).