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.

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

Global Styles: Don't output preset classes for colors defined by the theme #35514

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

scruffian
Copy link
Contributor

Description

I think this is a regression introduced in #34334.

Consider a classic theme without a theme.json file, which adds a theme support for editor-color-palette. Because these colors get included in the theme.json via get_from_editor_settings, when we call get_preset_classes , these color classes from the theme origin are being output. So for example if you are using the "Canard" theme, Global Styles outputs some CSS like this:

.has-color-red {
	color: var(--wp--preset--color--red) !important;
}

However the CSS variable isn't defined, but it doesn't get ignored, instead it seems to fall back to the broswer default.

So the root of the problem is that the CSS variables used by get_preset_classes don't match the variables output by get_css_variables. This is because get_css_variables only outputs CSS variables defined on the core origin, whereas get_preset_classes doesn't filter out origins.

This PR adds a filter to get_settings_slugs which is used by get_preset_classes to only output settings that are defined on the passed origins. The alternative approach would be to remove the filter from get_settings_values_by_slug and output the CSS variables for both the theme and core origins. In some ways this is preferable as it lightens the load for themes to opt into Global Styles, but it might create unexpected changes.

How has this been tested?

Screenshots

Before:
Screenshot 2021-10-11 at 14 52 30

After:
Screenshot 2021-10-11 at 14 52 55

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@scruffian scruffian added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Oct 11, 2021
@scruffian scruffian self-assigned this Oct 11, 2021
@@ -739,11 +739,11 @@ private static function get_settings_values_by_slug( $settings, $preset_metadata
*
* @return array Array of presets where the key and value are both the slug.
*/
private static function get_settings_slugs( $settings, $preset_metadata ) {
private static function get_settings_slugs( $settings, $preset_metadata, $origins = self::VALID_ORIGINS ) {
Copy link
Member

Choose a reason for hiding this comment

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

Was trying to understand how this change was different than the existing code, but then I realized I was missing the bigger picture: how get_settings_slugs was being called.

It looks like when we introduced this function from a refactoring we forgot to add the last parameter (see).

@oandregal
Copy link
Member

oandregal commented Oct 11, 2021

This is what happened as far as I understand: we introduced the origins parameter in 11.5, at #34955 (see get_merged_preset_by_slug method), so they are working fine in 11.5 and 11.6. In #34667 slated to 11.7 we introduced a refactoring that updated the code to use a new function called get_settings_slugs which tried to replicate the later. However, we forgot to actually allow that third-parameter in the function declaration (see).

@oandregal oandregal added this to the Gutenberg 11.7 milestone Oct 11, 2021
@oandregal
Copy link
Member

I've milestoned this for 11.7 as it fixes a bug we detected during the testing period for a PR introduced in 11.7 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants