Repository navigation
Enable style state-specific custom CSS - #83539
Conversation
tellthemachines
added
[Type] Enhancement
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: +457 B (+0.01%) Total Size: 8.51 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. refuses the drop and uploads nothing in
|
73fb2f1 to
34db686
Compare
tellthemachines
changed the title
| $processed_css = ''; | ||
| foreach ( $state_entries as $entry ) { | ||
| $entry_selector = null !== $entry['pseudo'] ? $selector . $entry['pseudo'] : $selector; | ||
| $entry_css = WP_Theme_JSON_Gutenberg::process_blocks_custom_css( $entry['css'], $entry_selector ); |
There was a problem hiding this comment.
The thing that stands out to me is the amount of looping over the same data, making a small change each time.
The first time is building the entries.
The second time is stripping the html.
The third time is generating the css.
Could it be one loop? For example, for pseudo selectors:
$processed_css = '';
foreach ( $supported_pseudo_states as $pseudo_state ) {
$pseudo_style = $style[ $pseudo_state ] ?? null;
if ( ! has_valid_custom_css( $pseudo_style ) ) {
continue;
}
$css = WP_Theme_JSON_Gutenberg::process_blocks_custom_css( $pseudo_style['css'], $selector . $entry['pseudo']);
if ( ! empty( $css ) ) {
$processed_css .= $css;
}
}Feels like the classic LLM thing where it adds way more code than is needed.
There was a problem hiding this comment.
Yeah, well that was an LLM decision 😄 but the readability didn't bother me. No strong opinion though so happy to look into reducing the looping.
There was a problem hiding this comment.
Ok I've updated this to consolidate the validation and generation of CSS into one loop. Building the entries needs to be separate on the JS side, where we're using the built entries to determine whether to show a warning notice. For consistency I decided to keep it on the PHP side too.
| // Process the custom CSS using the same method as global styles. | ||
| // Process the custom CSS using the same method as global styles, for every state. | ||
| $selector = '.' . $class_name; | ||
| $processed_css = WP_Theme_JSON_Gutenberg::process_blocks_custom_css( $custom_css, $selector ); |
There was a problem hiding this comment.
Doesn't the base CSS still need to be generated? Maybe I'm missing where that's done now.
There was a problem hiding this comment.
It does work, so I must be missing it 😄
There was a problem hiding this comment.
Ok, I see it now ($add_entry( $style['css'] ?? null, null, null );)
|
|
||
| /** | ||
| * Removes `css` keys from a block's `style` attribute, including any nested | ||
| * under pseudo-state (`:hover`) or viewport-state (`@mobile`) sub-objects. |
There was a problem hiding this comment.
I'm wondering why this needs to be done. The original code doesn't explain it either. 😅
There was a problem hiding this comment.
This is applied to users without edit CSS permissions! I worked it out from the commit history, but yes it's probably worth adding a comment in the actual code.
There was a problem hiding this comment.
Actually there's already a comment below where the filters are registered.
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
Sorry, I accidentally submitted the previous comments early before finishing the review.
| } | ||
| return processCSSNesting( customCSS, customCSSSelector ); | ||
| }, [ customCSS, customCSSSelector, isValidCSS ] ); | ||
| return renderCustomCSSStateEntries( |
There was a problem hiding this comment.
Similar to the PHP, I'm not convinced all the helper functions and repeated looping is beneficial. It makes the code harder to read for me.
| cssFeatureSelector && | ||
| typeof cssFeatureSelector === 'object' | ||
| ) { | ||
| resolvedCssSelector = ( cssFeatureSelector as { root?: string } ) |
There was a problem hiding this comment.
Could the if statement check for the existence of the root property here?
The typing is really awkward this code. I really don't know it well enough to have good suggestions on improving it though. Feels like it should be typed properly from the top down rather than casting sub-properties like this.
There was a problem hiding this comment.
I pushed a change that I think makes it a bit more readable, let me know if it improves matters!
| if ( typedNode?.css ) { | ||
| blockStyles.css = typedNode.css; | ||
| } |
There was a problem hiding this comment.
I don't really understand any of this tbh 😬
There was a problem hiding this comment.
As far as I understand it, css isn't included in STYLE_KEYS because it requires its own rendering path, being an actual CSS string as opposed to a style object. This PR just moves the custom CSS processing from generateGlobalStyles to renderStylesNode so it gets generated for all the style states.
| <div className="block-editor-hooks-custom-css__state-panel"> | ||
| { panel } | ||
| </div> |
There was a problem hiding this comment.
This part is missing a PanelBody.
The 'advanced' InspectorControls group usually renders its own around all its fills.
Because this code uses a different group the control is orphaned in the sidebar without any PanelBody around it.
There was a problem hiding this comment.
Oh is that why it looked weird 😅
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Thanks for getting this started!
|
|
||
| $supported_pseudo_states = WP_Theme_JSON_Gutenberg::VALID_BLOCK_PSEUDO_SELECTORS[ $block_name ] ?? array(); | ||
|
|
||
| static $responsive_media_queries = null; |
There was a problem hiding this comment.
Why static here? Would this survive a theme switch?
Other supports just assign a $var to gutenberg_get_global_settings(), which I think caches anyway.
There was a problem hiding this comment.
Yeah this might not be needed
| unset( $style['css'] ); | ||
|
|
||
| foreach ( $style as $key => $value ) { | ||
| if ( is_array( $value ) && ( str_starts_with( $key, ':' ) || str_starts_with( $key, '@' ) ) ) { |
There was a problem hiding this comment.
Do we still care about the old mobile {"mobile": rules?
Just wondering if we need to call gutenberg_resolve_style_state_aliases here like elsewhere
There was a problem hiding this comment.
No, we shouldn't be calling that function anywhere new. The places it was added to initially were meant for back compat with the plugin versions where we shipped the initial, unprefixed states. We could probably remove it altogether now (it was never meant for core); i'm not 100% sure what size deprecation window @talldan was considering.
There was a problem hiding this comment.
Two plugin versions, so we can remove it 👍
There was a problem hiding this comment.
I'll do a follow-up!
| settings: InspectorControlsDefault, // Alias for default. | ||
| styles: InspectorControlsStyles, | ||
| typography: InspectorControlsTypography, | ||
| viewport: InspectorControlsViewport, |
There was a problem hiding this comment.
I guess it's only two releases ago, but could anybody be using <InspectorControls group="viewport">?
Probably low risk, but the options is to add an alias like settings does above.
There was a problem hiding this comment.
Veloria says no and I'm going to assume that's correct rather than add another aliasing logic that will end up as tech debt 😅
| addEntry( style?.[ pseudoState ], pseudoState ) | ||
| ); | ||
|
|
||
| const breakpoints = viewportSettings |
There was a problem hiding this comment.
viewportSettings is undefined.
Wondering if we should add defaults to lib/theme.json.
E.g., "viewport": { "mobile": "480px", "tablet": "782px" } (or whatever).
Or just get the default media queries when the theme sets nothing
const breakpoints = Object.entries(
getResponsiveMediaQueries( viewportSettings )
);To match frontend.
Otherwise, in the editor, I'm seeing the following:
- For a para, set Additional CSS to
background: yellow; - At mobile breakpoint give it
background: red; - Switch back to desktop and the background is still red.
There was a problem hiding this comment.
Oh good catch. viewportSettings is only undefined if the theme doesn't set them, but there seems to be a bug where breakpoints contains both breakpoints and media queries when viewportSettings is defined, and only breakpoints otherwise.
| $this->assertMatchesRegularExpression( '/wp-custom-css-/', $result['attrs']['className'], 'className should contain wp-custom-css- prefix.' ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Do any of these tests cover the new behaviour, e.g., custom CSS read from the block and output with the right selectors / and media queries?
There was a problem hiding this comment.
they cover edge cases of new behaviour 😄 but might be worth adding a new generic test
| styles.css, | ||
| `:root :where(${ effectiveSelector })` | ||
| ); | ||
| // A block can declare a dedicated selector for custom CSS (e.g. to |
There was a problem hiding this comment.
This is something Claude picked up after another round.
The editor and front end now use different selectors for a style variation's custom CSS (basically the editor doesn't use a block's "css").
So for blocks like core/icon that define a "css" selector, variations won't use it and use the root one instead (only in the editor)
core/icon is the only one so far.
I tested by adding a variation:
add_action( 'init', function () {
register_block_style( 'core/icon', array( 'name' => 'framed', 'label' => 'Framed', 'style_data' => array( 'css' => 'outline: 4px dashed red;' ) ) );
} );The applying the variation "Framed" to an icon block.
| Editor | Frontend |
|---|---|
 |
 |
Should blocks use their own variation selector like the PHP version does?
There was a problem hiding this comment.
Oh, looks like this is a bug in the PHP logic. If there's a feature selector for custom CSS, then block style variations should use it, and currently it's being ignored on the PHP side.
Hm. Given this PR doesn't touch the theme json class at all, I'm tempted to fix this separately.
c793852 to
006b045
Compare
|
Ok I think all the feedback has been addressed now; this is ready for another review! |
006b045 to
802759f
Compare
| if ( ! is_array( $pseudo_style ) ) { | ||
| continue; | ||
| } | ||
| $add_entry( $pseudo_style['css'] ?? null, $pseudo_state, null ); |
There was a problem hiding this comment.
Up to you, but I think $add_entry doesn't really add much compared to this:
if ( isset( $pseudo_style['css'] ) && is_string( $pseudo_style['css'] ) && '' !== trim( $pseudo_style['css'] ) ) {
$entries[] = array(
'css' => $pseudo_style['css'],
'pseudo' => $pseudo_state
);
}It also has some weird qualities, like some of the checks on $pseudo_style['css'] are inside and some are outside the function. The function gets called with null as the first param most of the time and does nothing.
For more brevity it could be reduced to something like and the same could be true of the JS:
if ( has_valid_custom_css( $pseudo_style ) ) {
$entries[] = array(
'css' => $pseudo_style['css'],
'pseudo' => $pseudo_state
);
}| // Validate CSS doesn't contain HTML markup (same validation as global styles REST API). | ||
| if ( preg_match( '#</?\w+#', $custom_css ) ) { | ||
| $viewport_settings = gutenberg_get_global_settings( array( 'viewport' ) ); | ||
| $responsive_media_queries = WP_Theme_JSON_Gutenberg::get_viewport_media_queries( $viewport_settings ); |
There was a problem hiding this comment.
$responsive_media_queries doesn't get used again and gutenberg_get_custom_css_state_entries is only called from one place, so maybe it'd be tidier to move this code into gutenberg_get_custom_css_state_entries rather than getting it here and passing in as an arg.
There was a problem hiding this comment.
I moved it here so it would match other places where we do essentially the same: layout and position, and even states.php. Prior to this convo it was a static variable in gutenberg_get_custom_css_state_entries 😅
I don't know how valuable consistency is at this level though, it might not make much difference.
There was a problem hiding this comment.
It's definitely a nit, so feel free to leave it!
| unset( $style['css'] ); | ||
|
|
||
| foreach ( $style as $key => $value ) { | ||
| if ( is_array( $value ) && ( str_starts_with( $key, ':' ) || str_starts_with( $key, '@' ) ) ) { |
There was a problem hiding this comment.
Two plugin versions, so we can remove it 👍
| useEffect( () => { | ||
| if ( ! canEditCSS && hasCustomCSS ) { | ||
| createWarningNotice( | ||
| __( | ||
| 'This post contains blocks with custom CSS. You do not have permission to edit CSS. If you save this post, the custom CSS will be removed.' | ||
| ), | ||
| { | ||
| id: CUSTOM_CSS_WARNING_NOTICE_ID, | ||
| isDismissible: true, | ||
| } | ||
| ); | ||
| } | ||
| }, [ canEditCSS, hasCustomCSS, createWarningNotice ] ); |
There was a problem hiding this comment.
Unrelated to the PR, but this whole thing is really bizarre. I get why, but it feels wrong that a user with lower capabilities can delete the work of a one with more capabilities so easily. What would be the point of such a user editing/saving if they'll destroy something someone else worked on?
Feels like instead it should prevent the edit / save - "You can't save this post, since it contains custom CSS that your account does not have permission to save".
There was a problem hiding this comment.
Yeah there was a whole argument about it in #76650 and this is where it ended up 🤷
| help={ cssHelpText } | ||
| /> | ||
| <InspectorControls group={ inspectorGroup }> | ||
| { isStateSelected ? <PanelBody>{ panel }</PanelBody> : panel } |
There was a problem hiding this comment.
PanelBody components usually have a title which makes them expandable/collapsible. I think this is also why the padding around the control looks off.
I think it should be at the end if it's called 'Advanced'. On the gallery at the moment it has 'Settings' after it:
There was a problem hiding this comment.
Because they're both in the style states slot, and settings are added by the Gallery block. I think whatever the block adds to that slot goes in after whatever the block supports add? Because we're providing the slot to extenders, I'm not sure we can fully control how things appear. This is why I was asking whether we should make additional CSS a ToolsPanel 😅
There was a problem hiding this comment.
Would it be worth creating another slot just for a style states enabled "advanced" panel? I can't think of any better solution
There was a problem hiding this comment.
Yeah, tricky one. I guess another slot is the right approach. It could be a private slot. I believe 'Manage allowed blocks' did similar to ensure it was the last item.
There was a problem hiding this comment.
Ok I added a private slot for style states. We can use it for any "advanced" tools (maybe also "apply globally" if we make it work per-state at some point)
6320949 to
6ffc08b
Compare
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
6ffc08b to
73f8e01
Compare
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
The inspector panels work nicely now, thanks for addressing all the feedback.
| * Removes `css` keys from a block's `style` attribute, including any nested | ||
| * under pseudo-state (`:hover`) or viewport-state (`@mobile`) sub-objects. | ||
| * | ||
| * @since 7.1.0 |
There was a problem hiding this comment.
Just noticed this while testing WordPress/wordpress-develop#14197
| * @since 7.1.0 | |
| * @since 7.2.0 |
3 participants
What?
Part of #80388.
Testing Instructions
Use of AI Tools
used copilot with gpt-6 luna and claude with sonnet 5.