Fix for classic themes using default presets

[oandregal]

Fixes https://core.trac.wordpress.org/ticket/54782
Alternative to #2140 #2158 and #2154

What this PR does

This PR proposes that the presets provided by the theme via add_theme_support always create CSS Custom Properties, whether or not the theme has a theme.json file.

The end result is that, for a theme like TwentyTwenty, these are the changes:

Note how the existing defaults provided by the theme are set to the theme value plus there are new CSS Custom Properties for the presets defined by the theme but not by core.

Context

WordPress 5.9 introduced a change for themes with no theme.json that redefined the default presets provided by WordPress. Before, the default presets were:

.has-small-font-size {
  font-size: <raw_value>;
}

In WordPress 5.9 they are:

.has-small-font-size {
  font-size: var(--wp--preset--font-size--small) !important;
}

The rationale for this change was to fix the following issue: some blocks (and theme) selectors can have higher specificity than user styles attached to blocks.

For example, .wp-block-post-title h1 has higher specificity than .has-small-font-size. However, we want user styles applied to a block to prevail at all times over any other style. We considered alternatives but none were satisfactory. It was also discussed in this thread.

Other options considered

The current approach to this issue has been: communication (e.g.: WordPress 5.9 devnote in addition to sharing it previously in GitHub/Slack/forums/etc). Eventually, themes that want to redefine the default presets provided by WordPress and don't use theme.json would need to be updated.

This PR takes a different approach: can WordPress set the CSS Custom Properties for these themes automatically?

How to test

Test this both using WordPress 5.9 and the TwentyTwenty theme (can use any other theme that doesn't have a theme.json and redefines the default presets provided by core).

Font sizes

• Create a post that contains 5 paragraphs with the following content:
  • Small (18)
  • Normal (21)
  • Default
  • Large (26.25)
  • Larger (32)
• To each paragraph apply the font size that corresponds to its content: apply small to the 1st paragraph, normal to the 2nd, nothing to the third, large to the 4th, and larger to the 5th.

The expected result is that the font size for each paragraph has the assigned value, both in the editor and front-end.

Colors

• Create a post.
• Create a paragraph and apply custom color classes by pasting this text has-white-background-color has-black-color into the "Additional classes" text field of the "Advanced" section of the block sidebar.
• Create a new paragraph and apply custom color classes by pasting this text has-black-background-color has-white-color into the "Additional classes" text field of the "Advanced" section of the block sidebar.

Via the browser devtools make sure that the first paragraph's color is rgb(0,0,0) and its background is rgb(255,255,255). The second paragraph should be the opposite.

[johnstonphilip]

Using !important is a hack which highlights that this is a bigger issue that needs to be resolved.

If there are multiple ways to set a value that are conflicting and cause CSS specificity issues, then those need to become united so they can be resolved and reduce the bloat being output to the screen.

If I am understanding correctly, the example you provided here:

/* block classes that themes use to set styles */
.wp-block-post-title h1 { /* this has higher specificity than the preset class */
  color: green;
}

/*
 * Preset class that is only attached to blocks by the user,
 * otherwise they're not present. If they're attached,
 * they should override any other rule the theme has set.
 */
.has-red-color {
  color: red;
}

The real problem seems to be that color: green is attached to .wp-block-post-title h1, when it should be attached to a class like .has-green-color. Obviously that is a bigger problem, but we can't just use !important to solve it. That makes it worse.

[webmandesign]

What about responsive typography styles? Should existing themes suffer due to !important just because it works with block themes currently?

[johnstonphilip]

That being said, I realize this is a PR for doing a quick fix for https://core.trac.wordpress.org/ticket/54782 and WordPress/gutenberg#38252

The issue of !important can be discussed in other issues. Appreciate the work being done here to fix the bug affecting live sites right now.

[oandregal]

While the PR is working fine for all my tests (twentytwentytwo, twentytwenty, twentynineteen, storefront), I'm struggling to understand how to make the PHP unit tests pass.

This is what I have:

• If I run the tests locally, all of them pass but the test_variables_in_classic_theme_with_presets_* ones.
• All tests pass in CI for the following PHP environments: 5.6, 8.1.
• All tests fail in CI (7.2, 7.3, 7.4, 8.0) as well as in the local execution. They report an error upon calling switch_theme( 'theme-name' ):

There were 9 errors:

Tests_Global_Stylesheet::test_block_theme_using_variables

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:9

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_block_theme_using_presets

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:22

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_block_theme_using_defaults

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:35

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_no_presets_using_variables

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:48

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_no_presets_using_presets

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:60

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_no_presets_using_defaults

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:72

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_presets_using_variables

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:84

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_presets_using_presets

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:96

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_presets_using_defaults

count(): Parameter must be an array or an object that implements Countable

/var/www/src/wp-includes/theme.php:787

/var/www/tests/phpunit/tests/theme/globalStylesheet.php:108

phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

• Some tests fail in CI for the PHP environments: 7.0, 7.1. It seems that some font sizes are not registered:

There were 4 failures:

Tests_Global_Stylesheet::test_block_theme_using_variables

custom font size is 100px
Failed asserting that false is true.
/var/www/tests/phpunit/tests/theme/globalStylesheet.php:16
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:52

Tests_Global_Stylesheet::test_block_theme_using_defaults

custom font size is 100px
Failed asserting that false is true.
/var/www/tests/phpunit/tests/theme/globalStylesheet.php:42
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:52

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_presets_using_variables

small font size is 18px
Failed asserting that false is true.
/var/www/tests/phpunit/tests/theme/globalStylesheet.php:87
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:52

Tests_Global_Stylesheet::test_variables_in_classic_theme_with_presets_using_defaults

small font size is 18px
Failed asserting that false is true.
/var/www/tests/phpunit/tests/theme/globalStylesheet.php:111
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:52

[oandregal]

It sounds like the test environment is not deterministic (was set up in some jobs but not for others), and that's why there were inconsistencies in the tests. Setting the theme dir in the class makes all the tests that fail to fail for the same reasons. There's still some that pass, but this is progress. It narrows down the issues.

[oandregal]

This is now ready, tests have been fixed.

[jorgefilipecosta]

LGTM 👍 The change looks good and worked well on my tests!

[jorgefilipecosta]

Committed at b7aa3a0.

[billerickson]

Should this PR be tagged Backport to WP Minor Release so it makes it in 5.9.1?

The original issue WordPress/gutenberg#38252 has the tag but not the PR.

[oandregal]

@billerickson Unlike the gutenberg repository, this one doesn't use tags, everything happens in trac. See the corresponding ticket were this PR was discussed https://core.trac.wordpress.org/ticket/54782

This was ported to the 5.9 branch at https://core.trac.wordpress.org/changeset/52678