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

Unable to change the block spacing within Global Styles when KSES is active #45520

Closed
mmtr opened this issue Nov 3, 2022 · 5 comments · Fixed by #46388
Closed

Unable to change the block spacing within Global Styles when KSES is active #45520

mmtr opened this issue Nov 3, 2022 · 5 comments · Fixed by #46388
Assignees
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@mmtr
Copy link
Contributor

mmtr commented Nov 3, 2022

Description

The KSES filters don't allow to change the block spacing via the Global Styles sidebar in the site editor.

These filters are only registered when a user doesn't have the unfiltered_html capability (which is the case for multisite regular admins).

When these filters are active, any change to the block spacing property is reverted back to its previous value.

Step-by-step reproduction instructions

  1. Activate latest Gutenberg on a multisite install, or test with a user not having the unfiltered_html capability, or enable the KSES filters with add_action( 'init', 'kses_init_filters' );
  2. Go to Appearance > Editor
  3. Open the Styles sidebar
  4. Select "Layout"
  5. Change the "Block spacing" value
  6. Save the changes
  7. ⚠️ Note how the changes are reverted

Screenshots, screen recording, code snippet

Screen.Recording.2022-11-03.at.16.04.24.mov

Environment info

  • WordPress: 6.1
  • Gutenberg: 14.4.0
  • Theme: Twenty Twenty-Two 1.3

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@mmtr mmtr added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Nov 3, 2022
@mmtr
Copy link
Contributor Author

mmtr commented Nov 3, 2022

Looks like this issue is also preventing the dimensions from being changed:

Screen.Recording.2022-11-03.at.16.15.02.mov

@mmtr
Copy link
Contributor Author

mmtr commented Dec 7, 2022

I investigated this further, and these are my findings.

This is not an issue caused by the lack of some HTML attributes allowed by KSES. The problem is that the wp_filter_global_styles_post function apparently has a bug, and that function runs when the KSES filters are active: https://github.com/WordPress/wordpress-develop/blob/ae42add90ef5b083368b87580fabbec1ada481f3/src/wp-includes/kses.php#L2196-L2198

Looking deeper, the function is considering the blockGap property as insecure here: https://github.com/WordPress/wordpress-develop/blob/ae42add90ef5b083368b87580fabbec1ada481f3/src/wp-includes/class-wp-theme-json.php#L2615

That happens because remove_insecure_styles is unable to compute the styles property for blockGap: https://github.com/WordPress/wordpress-develop/blob/ae42add90ef5b083368b87580fabbec1ada481f3/src/wp-includes/class-wp-theme-json.php#L2739

And the underlying issue seems to be that the list of PROPERTIES_METADATA doesn’t include anything for blockGap (hence compute_style_properties returns nothing). But apparently that’s an intended change according to the changelog?

@since 6.1.0 [...] removed the `--wp--style--block-gap` property

I see the --wp--style--block-gap property was removed in #40875, so maybe that PR introduced a regression? @andrewserong would you mind taking a look?

@andrewserong
Copy link
Contributor

Thanks for digging @mmtr, that appears to get to the root of the problem! Definitely an oversight in #40875.

Some quick initial thoughts:

  • blockGap was removed from PROPERTIES_METADATA because it shouldn't be output via compute_style_properties as its value will be handled later in the style output process via get_layout_styles.
  • PROPERTIES_METADATA maps a CSS property to a path in the style object, which isn't really applicable to how blockGap works, so:
  • It sounds like we need to update remove_insecure_styles to handle style properties that are not covered by (and/or) not meant to be handled by compute_style_properties. While currently that's just blockGap, I imagine in the future we'll either have more properties that need special handling, or we'll need to update the logic a little anyway if and when we migrate global styles output to use the style engine for processing. That latter point is an issue for further down the track.

I'll do a little more digging, but let me know if there's a particular way you think this should be handled.

For more background: the removal of the --wp--style--block-gap property was so that we can support outputting direct CSS values for block gap, based on the requirements of each layout type, to fix some nuanced issues with gap and the CSS cascade. So, (aside from this bug) the newer approach to block gap allows for it to be used for either vertical margins or CSS gap, depending on the layout type.

CC: @ramonjd and @tellthemachines for visibility since they've also worked on layout and styles related stuff recently.

@andrewserong andrewserong added the [Feature] Layout Layout block support, its UI controls, and style output. label Dec 7, 2022
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Dec 8, 2022
@andrewserong
Copy link
Contributor

I've opened a tentative fix over in #46388 — if that approach looks viable, we'll need to backport it, too. Happy for any feedback / ideas if folks think there's a different solution worth pursuing!

@andrewserong
Copy link
Contributor

Update:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
3 participants