Add layout-related CSS properties to safe_style_css_filter in kses.php

[andrewserong]

Trac ticket: https://core.trac.wordpress.org/ticket/56122

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Hi @andrewserong! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

• PHP Coding Standards
• CSS Coding Standards
• HTML Coding Standards
• JavaScript Coding Standards
• Accessibility Coding Standards
• Inline Documentation Standards

Thank you,
The WordPress Project

[peterwilsoncc]

I'd rather not allow display in it's entirety. I wasn't around for the original specification but I expect it was left out very deliberately to prevent authors and contributors from doing something like:

<p style="display: none"><a href="spammy-seo-link.html">Certainly nothing dodgy here</a></p>

Are you able to limit it to a set of values?

'display' => array( 'flex', 'grid' )

...

// Something clever in the loop below to determine whether to use the key or value as the allowed selector if this is possible.

[andrewserong]

Thanks Peter, that's exactly the kind of feedback I was hoping for on this PR! I'll have a play, yes the goal is to allow a couple of particular kinds of layouts, not to expose display: none, so I'll see what we can do to limit this to just what's required for the current and shorter-term goals of different layouts. Or, alternately, if we need to specifically handle display within the Layout support and not expose it in the kses file. I'll give it some more thought! 🤔

[andrewserong]

Update: I had another look over the Layout work in the Gutenberg repo, and I think it makes more sense for the Layout code to use its own special handling for the display property (constructing the rule based on an allow list) rather than exposing it directly in kses.php. Even if we limit the options here, the objective wasn't really to allow display to be used anywhere, it was really tied to this one use case. Whereas the goal for the other properties added in this PR is for them to be usable pretty much anywhere.

I've removed display from this PR (and added special handling for display over in the Layout PR WordPress/gutenberg#40875). But, we can always revisit this in the future if we do think it makes more sense in kses.php! 🙂

[ramonjd]

I wasn't around for the original specification but I expect it was left out very deliberately to prevent authors and contributors from doing something like:

Are there any discussions you could point to that would help us understand this concern?

I mean, what's stopping authors and contributors doing that by assigning the background color to text and links?

E.g.,

<!-- this is invisible on a white background -->
<p class="has-white-color has-text-color has-link-color"><a href="http://test.com" data-type="URL" data-id="http://test.com">Spammy text</a></p>

[ramonjd]

And if it ends up that we have to constrain values for display, would it be okay to have a custom check within safecss_filter_attr ?

[ramonjd]

@peterwilsoncc @andrewserong How do you think we could proceed with this one?

Would a compromise be a white list of display properties, excluding none, assuming the concern alluded to above is valid?

[andrewserong]

Good question, thanks for the ping @ramonjd! For 6.1, do we need display to be included? I believe the only place we're currently using it is in the layout support's rendering in the Theme JSON class (here: https://github.com/WordPress/gutenberg/blob/ffd75bd84e15b7ac611d3c25bc11e0fcd4840773/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L1410)

To avoid being blocked on display, I was wondering if the current state of this PR is good enough to land as-is, given that it addresses the remaining layout properties that the style engine will need to validate?

Alternately, if we think it's worth it to handle display as well, I think adding a list of allowed values sounds like a reasonable way to go 👍

[ramonjd]

For 6.1, do we need display to be included?

Good question! I assumed "yes" because of the eventual migration of class-wp-theme-json-6-1.php 🤷

[andrewserong]

I assumed "yes" because of the eventual migration of class-wp-theme-json-6-1.php 🤷

In the current implementation, on this line the allow-listed display mode is output directly, rather than being parsed through safecss_filter_attr, so I didn't think it was needed until we move global styles output to being run through the style engine.

[ramonjd]

Ah, thanks for clarifying! 🙇

[andrewserong]

@noisysocks just pinging you in case you get a chance to look at this one, too, since you've been involved with the (semi) related trac ticket for adding support for min() (https://core.trac.wordpress.org/ticket/55966). This one is different, in that it's adding additional CSS properties, but is also a pre-requisite for backporting the Layout block support changes in Gutenberg into core.

[ockham]

I don't know much about the Layout work in GB, but FWIW, this PR is looking pretty good to me. Think we can merge as-is to unblock other layout-related PRs, and consider display separately?

[SergeyBiryukov]

Thanks for the PR! Merged in r54102.

[andrewserong]

Thank you for merging! Much appreciated 🙇