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

Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json #40875

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 49 additions & 35 deletions lib/block-supports/layout.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,14 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= "$selector .alignfull { max-width: none; }";
}

$style .= "$selector > .alignleft { float: left; margin-inline-start: 0; margin-inline-end: 2em; }";
$style .= "$selector > .alignright { float: right; margin-inline-start: 2em; margin-inline-end: 0; }";
$style .= "$selector > .aligncenter { margin-left: auto !important; margin-right: auto !important; }";
if ( $has_block_gap_support ) {
if ( is_array( $gap_value ) ) {
$gap_value = isset( $gap_value['top'] ) ? $gap_value['top'] : null;
}
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : 'var( --wp--style--block-gap )';
$style .= "$selector > * { margin-block-start: 0; margin-block-end: 0; }";
$style .= "$selector > * + * { margin-block-start: $gap_style; margin-block-end: 0; }";
if ( $gap_value && ! $should_skip_gap_serialization ) {
$style .= "$selector > * { margin-block-start: 0; margin-block-end: 0; }";
$style .= "$selector > * + * { margin-block-start: $gap_value; margin-block-end: 0; }";
}
}
} elseif ( 'flex' === $layout_type ) {
$layout_orientation = isset( $layout['orientation'] ) ? $layout['orientation'] : 'horizontal';
Expand All @@ -94,52 +92,50 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$justify_content_options += array( 'space-between' => 'space-between' );
}

$flex_wrap_options = array( 'wrap', 'nowrap' );
$flex_wrap = ! empty( $layout['flexWrap'] ) && in_array( $layout['flexWrap'], $flex_wrap_options, true ) ?
$layout['flexWrap'] :
'wrap';
if ( ! empty( $layout['flexWrap'] ) && 'nowrap' === $layout['flexWrap'] ) {
$style .= "$selector { flex-wrap: nowrap; }";
}

$style = "$selector {";
$style .= 'display: flex;';
if ( $has_block_gap_support ) {
if ( is_array( $gap_value ) ) {
$gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : $fallback_gap_value;
$gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : $fallback_gap_value;
$gap_value = $gap_row === $gap_column ? $gap_row : $gap_row . ' ' . $gap_column;
}
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : "var( --wp--style--block-gap, $fallback_gap_value )";
$style .= "gap: $gap_style;";
} else {
$style .= "gap: $fallback_gap_value;";
if ( $gap_value && ! $should_skip_gap_serialization ) {
Copy link
Member

@ramonjd ramonjd Jul 6, 2022

Choose a reason for hiding this comment

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

I might have lost some context on this issue. When testing the fallback value for the Columns block, I noticed that it wasn't making it through the conditions.

Should we be setting $gap_value when it's null? Something like this?

Suggested change
if ( $gap_value && ! $should_skip_gap_serialization ) {
if ( ! $should_skip_gap_serialization ) {
$gap_value = $gap_value ? $gap_value : $fallback_gap_value;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks @ramonjd, I forgot about that one. This is a tricky part of the problem, and I'm not quite sure what the right solution is. We can't use the fallback value here, or it'll overwrite the root gap value so that if a base blockGap value exists then the fallback would take precedence. The objective in this function should be "if there is no gap value at the block level, do not output a rule", and a solution for fallback probably needs to be handled elsewhere, I think?

There's separate handling in this PR for Classic themes and the Columns block fallback value, but I couldn't quite work out a good solution for blocks-based themes and still respecting the fallback value. Let me know if you can think of any other ways of handling it, and I'll do some more 🤔 too!

Copy link
Member

Choose a reason for hiding this comment

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

Oh dang, thanks for the explainer. I thought I was missing something. I'll have a think.

Copy link
Member

@ramonjd ramonjd Jul 7, 2022

Choose a reason for hiding this comment

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

The objective in this function should be "if there is no gap value at the block level, do not output a rule",

Does this mean we need to be consistent in the above condition (is_array( $gap_value )) and check that at least one vertical value set by the block exists? Otherwise the fallback would take precedence as well. 🤔

Oh wait, sorry. We're still expecting a single string value in theme.json. 👍

Copy link
Member

@ramonjd ramonjd Jul 7, 2022

Choose a reason for hiding this comment

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

@andrewserong

This seems to work for me (so far). Still testing.

Diff to check for block.json default values in layout
diff --git a/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php b/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
index f8b82319d1..4d4fa857c3 100644
--- a/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
+++ b/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
@@ -689,15 +689,13 @@ class WP_Theme_JSON_6_1 extends WP_Theme_JSON_6_0 {
 
 		// 4. Generate Layout block gap styles.
 		$has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null;
-		$has_block_gap_value   = _wp_array_get( $node, array( 'spacing', 'blockGap' ), false );
 
 		if (
 			static::ROOT_BLOCK_SELECTOR !== $selector &&
 			$has_block_gap_support &&
-			$has_block_gap_value &&
 			! empty( $block_metadata['name'] )
 		) {
-			$block_rules .= $this->get_layout_styles( $block_metadata );
+			$block_rules    .= $this->get_layout_styles( $block_metadata );
 		}
 
 		if ( static::ROOT_BLOCK_SELECTOR === $selector ) {
@@ -1164,16 +1162,14 @@ class WP_Theme_JSON_6_1 extends WP_Theme_JSON_6_0 {
 		// Gap styles will only be output if the theme has block gap support, or supports `wp-block-styles`.
 		// In this way, we tie the concept of gap styles to the styles that ship with core blocks.
 		// Default layout gap styles will be skipped for themes that do not explicitly opt-in to blockGap with a `true` or `false` value.
+
 		if ( $has_block_gap_support || $has_block_styles_support ) {
-			$block_gap_value = null;
+			$block_gap_value     = _wp_array_get( $node, array( 'spacing', 'blockGap' ), null );
+			$block_default_value = ! empty( $block_type ) ? _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), null ) : null;
+			$block_gap_value     = ! $block_gap_value && $block_default_value ? $block_default_value : $block_gap_value;
 			// Use a fallback gap value if block gap support is not available.
 			if ( ! $has_block_gap_support ) {
-				$block_gap_value = '0.5em';
-				if ( ! empty( $block_type ) ) {
-					$block_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), '0.5em' );
-				}
-			} else {
-				$block_gap_value = _wp_array_get( $node, array( 'spacing', 'blockGap' ), null );
+				$block_gap_value = $block_default_value ? $block_default_value : '0.5em';
 			}
 
 			// Support split row / column values and concatenate to a shorthand value.

Copy link
Contributor Author

@andrewserong andrewserong Jul 8, 2022

Choose a reason for hiding this comment

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

Thanks for re-testing!

what's the purpose of the 0.5em gap value set on the body element

That's the one that gets attached to body .is-layout-flex, so it's the root default gap that is used by Buttons and Social Icons blocks (or any of the blocks that use the Flex layout). They don't need an explicit __experimentalDefault since they're happy using the common base fallback gap. In this PR, rather than always outputting that 0.5em value at render time of the individual block, we use this root rule so we can avoid duplication.

I liked your idea in one of the other comments of a follow-up PR to remove the tie to the body element for generating the base layout styles, though, it might make the relationship here a little clearer 🙂 (in effect, we're currently using the body element as a "please only output this rule once and at the root" piece of logic, which could be abstracted in a clearer way, potentially)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, looking at the other blocks with flex layout, 0.5em seems to work for them. I'd thought Navigation might be an exception, but since nav items already have a fair bit of padding around them, it works well 😄

(in effect, we're currently using the body element as a "please only output this rule once and at the root" piece of logic, which could be abstracted in a clearer way, potentially)

Yeah, we're using it to add global rules that don't need to be attached to any particular block/HTML element. Good thing to look at in a follow-up!

Copy link
Member

Choose a reason for hiding this comment

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

I think I've come up with a way to handle the fallback gap in a way that doesn't feel too hacky. In the Resolver class, where individual blocks are iterated over, I've added a section that puts in a null placeholder for the blockGap value for that particular block, if the block provides a default value.

Thanks! Nice work.

It took me a while to realize that I had to remove blockGap block support from theme.json (even though I don't know why?) After that it tests very well!

I think my assumptions must be wrong on this issue: isn't the point of the columns default fallback to provide a fallback if there is no blockGap value set anywhere, regardless of whether a theme has opted into spacing.blockGap or not?

If not, and I'm way off, then it seems like a good approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking another look at this! 🙇

isn't the point of the columns default fallback to provide a fallback if there is no blockGap value set anywhere, regardless of whether a theme has opted into spacing.blockGap or not

It's a good question of what the desired behaviour is here, but in principle the behaviour in this PR should now match trunk. Because Gutenberg/core's theme.json provides a base spacing.blockGap value, the fallback gap will never be reached unless gap support is completely switched off. My current thinking is that if a theme uses gap support (which is opt-in), then it is the theme that's responsible for determining its own Columns spacing, rather than relying on a fallback value.

Or to put it slightly differently, the columns default fallback value is for backwards compatibility, and as of this PR, with the ability to set gap at the block-level in theme.json, if a theme uses blockGap then (ideally 😅) it should use the appropriate core/columns block in styles.blocks rather than depending on fallback styles, if the theme needs to have a particular value for the gap.

It's totally worth re-examining those assumptions if they're incorrect, though! I suppose my preference would be to see if a) the mechanism in this PR is viable, so that b) we feel comfortable enough that we can tweak the behaviour in follow-up PRs if need be 🙂

Copy link
Member

Choose a reason for hiding this comment

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

It's a good question of what the desired behaviour is here, but in principle the behaviour in this PR should now match trunk. Because Gutenberg/core's theme.json provides a base spacing.blockGap value, the fallback gap will never be reached unless gap support is completely switched off. My current thinking is that if a theme uses gap support (which is opt-in), then it is the theme that's responsible for determining its own Columns spacing, rather than relying on a fallback value.

Ah thanks for setting me straight (again 😄)

That makes total sense.

$style .= "$selector {";
$style .= "gap: $gap_value;";
$style .= '}';
}
}

$style .= "flex-wrap: $flex_wrap;";
if ( 'horizontal' === $layout_orientation ) {
/**
* Add this style only if is not empty for backwards compatibility,
* since we intend to convert blocks that had flex layout implemented
* by custom css.
*/
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "$selector {";
$style .= "justify-content: {$justify_content_options[ $layout['justifyContent'] ]};";
$style .= '}';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to output these? I'm seeing the same styles already applied through semantic classnames, both in editor and front end:
Screen Shot 2022-06-22 at 11 52 24 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe for the moment we do. Those other styles are coming from the Buttons block's hard-coded styles.scss file, so I thought we would look at implementing the semantic classnames properly in a separate follow-up PR, mostly to try to reduce the scope for this PR 🙂

The existing semantic classnames that get added are only adding the classnames but not the values associated with them. (The semantic classnames were added back in, in #41487)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see! That makes sense. I so look forward to getting rid of those container classes 😄

}

if ( ! empty( $layout['verticalAlignment'] ) && array_key_exists( $layout['verticalAlignment'], $vertical_alignment_options ) ) {
$style .= "$selector {";
$style .= "align-items: {$vertical_alignment_options[ $layout['verticalAlignment'] ]};";
} else {
$style .= 'align-items: center;';
$style .= '}';
}
} else {
$style .= "$selector {";
$style .= 'flex-direction: column;';
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "align-items: {$justify_content_options[ $layout['justifyContent'] ]};";
} else {
$style .= 'align-items: flex-start;';
}
$style .= '}';
}
$style .= '}';

$style .= "$selector > * { margin: 0; }";
}

return $style;
Expand All @@ -160,21 +156,23 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
return $block_content;
}

$block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) );
$default_layout = gutenberg_get_global_settings( array( 'layout' ) );
$has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false;
$default_block_layout = _wp_array_get( $block_type->supports, array( '__experimentalLayout', 'default' ), array() );
$used_layout = isset( $block['attrs']['layout'] ) ? $block['attrs']['layout'] : $default_block_layout;
$block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) );
$global_layout_settings = gutenberg_get_global_settings( array( 'layout' ) );
$has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false;
$default_block_layout = _wp_array_get( $block_type->supports, array( '__experimentalLayout', 'default' ), array() );
$used_layout = isset( $block['attrs']['layout'] ) ? $block['attrs']['layout'] : $default_block_layout;
if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] ) {
if ( ! $default_layout ) {
if ( ! $global_layout_settings ) {
return $block_content;
}
$used_layout = $default_layout;
$used_layout = $global_layout_settings;
}

$class_names = array();
$container_class = wp_unique_id( 'wp-container-' );
$class_names[] = $container_class;
$class_names = array();
$layout_definitions = _wp_array_get( $global_layout_settings, array( 'definitions' ), array() );
$block_classname = wp_get_block_default_classname( $block['blockName'] );
$container_class = wp_unique_id( 'wp-container-' );
$layout_classname = '';

// The following section was added to reintroduce a small set of layout classnames that were
// removed in the 5.9 release (https://github.com/WordPress/gutenberg/issues/38719). It is
Expand All @@ -192,6 +190,17 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
$class_names[] = 'is-nowrap';
}

// Get classname for layout type.
if ( isset( $used_layout['type'] ) ) {
$layout_classname = _wp_array_get( $layout_definitions, array( $used_layout['type'], 'className' ), '' );
} else {
$layout_classname = _wp_array_get( $layout_definitions, array( 'default', 'className' ), '' );
}

if ( $layout_classname && is_string( $layout_classname ) ) {
$class_names[] = sanitize_title( $layout_classname );
}

$gap_value = _wp_array_get( $block, array( 'attrs', 'style', 'spacing', 'blockGap' ) );
// Skip if gap value contains unsupported characters.
// Regex for CSS value borrowed from `safecss_filter_attr`, and used here
Expand All @@ -209,7 +218,14 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
$should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' );
$style = gutenberg_get_layout_style( ".$container_class", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
$style = gutenberg_get_layout_style( ".$block_classname.$container_class", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
andrewserong marked this conversation as resolved.
Show resolved Hide resolved

// Only add container class and enqueue block support styles if unique styles were generated.
if ( ! empty( $style ) ) {
$class_names[] = $container_class;
wp_enqueue_block_support_styles( $style );
}

// This assumes the hook only applies to blocks with a single wrapper.
// I think this is a reasonable limitation for that particular hook.
$content = preg_replace(
Expand All @@ -219,8 +235,6 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
1
);

wp_enqueue_block_support_styles( $style );

return $content;
}

Expand Down
69 changes: 0 additions & 69 deletions lib/compat/wordpress-6.0/get-global-styles-and-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,75 +63,6 @@ function gutenberg_get_global_styles( $path = array(), $context = array() ) {
return _wp_array_get( $styles, $path, $styles );
}

/**
* Returns the stylesheet resulting of merging core, theme, and user data.
*
* @param array $types Types of styles to load. Optional.
* It accepts 'variables', 'styles', 'presets' as values.
* If empty, it'll load all for themes with theme.json support
* and only [ 'variables', 'presets' ] for themes without theme.json support.
*
* @return string Stylesheet.
*/
function gutenberg_get_global_stylesheet( $types = array() ) {
// Return cached value if it can be used and exists.
// It's cached by theme to make sure that theme switching clears the cache.
$can_use_cached = (
( empty( $types ) ) &&
( ! defined( 'WP_DEBUG' ) || ! WP_DEBUG ) &&
( ! defined( 'SCRIPT_DEBUG' ) || ! SCRIPT_DEBUG ) &&
( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) &&
! is_admin()
);
$transient_name = 'gutenberg_global_styles_' . get_stylesheet();
if ( $can_use_cached ) {
$cached = get_transient( $transient_name );
if ( $cached ) {
return $cached;
}
}
$tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
$supports_theme_json = WP_Theme_JSON_Resolver_Gutenberg::theme_has_support();
if ( empty( $types ) ) {
$types = array( 'variables', 'styles', 'presets' );
}

/*
* If variables are part of the stylesheet,
* we add them for all origins (default, theme, user).
* This is so themes without a theme.json still work as before 5.9:
* they can override the default presets.
* See https://core.trac.wordpress.org/ticket/54782
*/
$styles_variables = '';
if ( in_array( 'variables', $types, true ) ) {
$styles_variables = $tree->get_stylesheet( array( 'variables' ) );
$types = array_diff( $types, array( 'variables' ) );
}

/*
* For the remaining types (presets, styles), we do consider origins:
*
* - themes without theme.json: only the classes for the presets defined by core
* - themes with theme.json: the presets and styles classes, both from core and the theme
*/
$styles_rest = '';
if ( ! empty( $types ) ) {
$origins = array( 'default', 'theme', 'custom' );
if ( ! $supports_theme_json ) {
$origins = array( 'default' );
}
$styles_rest = $tree->get_stylesheet( $types, $origins );
}
$stylesheet = $styles_variables . $styles_rest;
if ( $can_use_cached ) {
// Cache for a minute.
// This cache doesn't need to be any longer, we only want to avoid spikes on high-traffic sites.
set_transient( $transient_name, $stylesheet, MINUTE_IN_SECONDS );
}
return $stylesheet;
}

/**
* Returns a string containing the SVGs to be referenced as filters (duotone).
*
Expand Down
12 changes: 12 additions & 0 deletions lib/compat/wordpress-6.1/block-editor-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,18 @@ function gutenberg_get_block_editor_settings( $settings ) {
$block_classes['css'] = $actual_css;
$new_global_styles[] = $block_classes;
}
} else {
// If there is no `theme.json` file, ensure base layout styles are still available.
$block_classes = array(
'css' => 'base-layout-styles',
'__unstableType' => 'theme',
'isGlobalStyles' => true,
);
$actual_css = gutenberg_get_global_stylesheet( array( $block_classes['css'] ) );
if ( '' !== $actual_css ) {
$block_classes['css'] = $actual_css;
$new_global_styles[] = $block_classes;
}
}

$settings['styles'] = array_merge( $new_global_styles, $styles_without_existing_global_styles );
Expand Down
20 changes: 20 additions & 0 deletions lib/compat/wordpress-6.1/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,26 @@
* @package gutenberg
*/

/**
* Update allowed inline style attributes list.
*
* Note: This should be removed when the minimum required WP version is >= 6.1.
*
* @param string[] $attrs Array of allowed CSS attributes.
* @return string[] CSS attributes.
*/
function gutenberg_safe_style_attrs_6_1( $attrs ) {
Copy link
Contributor Author

@andrewserong andrewserong Jul 1, 2022

Choose a reason for hiding this comment

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

To see if there are any blockers with adding in support for these properties, I've opened up a Trac ticket here: https://core.trac.wordpress.org/ticket/56122#ticket and a corresponding PR: WordPress/wordpress-develop#2928

$attrs[] = 'flex-wrap';
$attrs[] = 'gap';
$attrs[] = 'margin-block-start';
$attrs[] = 'margin-block-end';
$attrs[] = 'margin-inline-start';
$attrs[] = 'margin-inline-end';

return $attrs;
}
add_filter( 'safe_style_css', 'gutenberg_safe_style_attrs_6_1' );

/**
* Registers view scripts for core blocks if handling is missing in WordPress core.
*
Expand Down
Loading