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.

Sign up for GitHub

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

Jump to bottom

[Try]: Style Engine: Alternative approach to rendering common layout styles from presets #39374

Closed
wants to merge 6 commits into from
Closed

[Try]: Style Engine: Alternative approach to rendering common layout styles from presets #39374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43fa6cf
Try: Alternative approach to rendering common layout styles from presets
andrewserong Mar 11, 2022
dca45e8
Move style definitions to theme.json
andrewserong Mar 15, 2022
c2780bf
Add classnames to output
andrewserong Mar 15, 2022
8313c31
Try: move controlled sets of styles to theme.json
andrewserong Mar 16, 2022
c0375f9
Remove additional type key
andrewserong Mar 17, 2022
3b1609c
Add in remaining classes, split classes and styles in get_layout_styl…
andrewserong Mar 17, 2022
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
138 changes: 84 additions & 54 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,92 @@ function gutenberg_register_layout_support( $block_type ) {
}
}

function gutenberg_get_layout_preset_styles( $preset_metadata, $presets ) {
$style_engine = WP_Style_Engine_Gutenberg::get_instance();
$style_engine->reset();

foreach ( $presets as $key => $preset ) {
if ( ! empty( $preset['type'] ) && ! empty( $preset['styles'] ) ) {
$slug = ! empty( $preset['slug'] ) ? $preset['slug'] : $preset['type'];
$base_class = 'wp-layout-' . sanitize_title( $slug );

if ( ! empty( $preset['styles'] ) ) {
foreach ( $preset['styles'] as $style ) {
if ( ! empty( $style['rules'] ) && is_array( $style['rules'] ) ) {
$args = array(
'selector' => ! empty( $style['selector'] ) ? $style['selector'] : null,
'suffix' => ! empty( $style['suffix'] ) ? sanitize_title( $style['suffix'] ) : null,
'rules' => $style['rules'],
);
$style_engine->add_style( $base_class, $args );
}
}
}

if ( ! empty( $preset['controlledSets'] ) ) {
foreach ( $preset['controlledSets'] as $controlled_set ) {
$property = ! empty( $controlled_set['property'] ) ? sanitize_title( $controlled_set['property'] ) : null;
$suffix = ! empty( $controlled_set['suffix'] ) ? sanitize_title( $controlled_set['suffix'] ) : null;

if ( $property && $suffix && ! empty( $controlled_set['options'] ) ) {
foreach( $controlled_set['options'] as $option_setting => $option_value ) {
$args = array(
'suffix' => array( $suffix, sanitize_title( $option_setting ) ),
'rules' => array( $property => $option_value ),
);
$style_engine->add_style( $base_class, $args );
}
}
}
}
}
}

return $style_engine->get_generated_styles();
}

/**
* Generates the CSS corresponding to the provided layout.
* Generates the CSS and classes corresponding to the provided layout.
*
* @param string $selector CSS selector.
* @param array $layout Layout object. The one that is passed has already checked the existence of default block layout.
* @param boolean $has_block_gap_support Whether the theme has support for the block gap.
* @param string $gap_value The block gap value to apply.
*
* @return string CSS style.
* @return array CSS style and CSS classes.
*/
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null ) {
$layout_type = isset( $layout['type'] ) ? $layout['type'] : 'default';

// Generate classes for preset styles.
$classes = array();
if ( 'default' === $layout_type ) {
$classes[] = 'wp-layout-flow';
if ( $has_block_gap_support ) {
$classes[] = 'wp-layout-flow--global-gap';
}
} else if ( 'flex' === $layout_type ) {
$classes[] = 'wp-layout-flex';

$layout_orientation = isset( $layout['orientation'] ) ? $layout['orientation'] : 'horizontal';
$orientation_key = 'horizontal' === $layout_orientation ? 'h-justify' : 'v-justify';

$classes[] = "wp-layout-flex--$orientation_key";

if ( ! empty( $layout['flexWrap'] ) ) {
$classes[] = 'wp-layout-flex--wrap--' . sanitize_key( $layout['flexWrap'] );
}

if ( ! empty( $layout['justifyContent'] ) ) {
$classes[] = sprintf(
'wp-layout-flex--%s--%s',
$orientation_key,
sanitize_key( $layout['justifyContent'] )
);
}
}

// Generate one-off style values.
$style = '';
if ( 'default' === $layout_type ) {
$content_size = isset( $layout['contentSize'] ) ? $layout['contentSize'] : '';
Expand All @@ -59,68 +132,25 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= '}';

$style .= "$selector > .alignwide { max-width: " . esc_html( $wide_max_width_value ) . ';}';
$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 ) {
$gap_style = $gap_value ? $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; }";
}
} elseif ( 'flex' === $layout_type ) {
$layout_orientation = isset( $layout['orientation'] ) ? $layout['orientation'] : 'horizontal';

$justify_content_options = array(
'left' => 'flex-start',
'right' => 'flex-end',
'center' => 'center',
);

if ( 'horizontal' === $layout_orientation ) {
$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';

$style = "$selector {";
$style .= 'display: flex;';
if ( $has_block_gap_support ) {
$style = "$selector {";
$gap_style = $gap_value ? $gap_value : 'var( --wp--style--block-gap, 0.5em )';
$style .= "gap: $gap_style;";
} else {
$style .= 'gap: 0.5em;';
}
$style .= "flex-wrap: $flex_wrap;";
if ( 'horizontal' === $layout_orientation ) {
$style .= 'align-items: center;';
/**
* 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 .= "justify-content: {$justify_content_options[ $layout['justifyContent'] ]};";
}
} else {
$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;
return array(
'style' => $style,
'classes' => $classes
);
}

/**
Expand Down Expand Up @@ -156,17 +186,17 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
// Regex for CSS value borrowed from `safecss_filter_attr`, and used here
// because we only want to match against the value, not the CSS attribute.
$gap_value = preg_match( '%[\\\(&=}]|/\*%', $gap_value ) ? null : $gap_value;
$style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value );
$styles = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value );
// 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(
'/' . preg_quote( 'class="', '/' ) . '/',
'class="' . esc_attr( $class_name ) . ' ',
'class="' . esc_attr( implode( ' ', array_merge( array( $class_name ), $styles['classes'] ) ) ) . ' ',
$block_content,
1
);

gutenberg_enqueue_block_support_styles( $style );
gutenberg_enqueue_block_support_styles( $styles['style'] );

return $content;
}
Expand Down
116 changes: 116 additions & 0 deletions lib/class-wp-style-engine-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
<?php
/**
* WP_Style_Engine class
*
* @package Gutenberg
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll focus my review on this file, as If I'm not wrong, this is the "style engine" package equivalent. Well I guess my first question is, should we try to move this file to the package (like we do for "parser", and "block-library")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we try to move this file to the package (like we do for "parser", and "block-library")

Good question! I suppose it depends on what will be easiest for when the feature needs to be merged into core. I see Ramon asked a similar question over on: #39446 (comment) — given that PR will be in better shape to land before this one, it might be better for us to explore where the file goes in that PR. For the moment, I mostly just put it here for convenience while hacking around!


/**
* Singleton class representing the style engine.
*
* Consolidates rendering block styles to reduce duplication and streamline
* CSS styles generation.
*
* @since 6.0.0
*/
class WP_Style_Engine_Gutenberg {
/**
* Registered CSS styles.
*
* @since 5.5.0
* @var array
*/
private $registered_styles = array();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the style engine need to be stateful? The JS version is stateless, so I was wondering if we should have the same signature/API?

I guess the question here is what is $resitered_styles for?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Expanded on my questioning here on my next comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the style engine need to be stateful?

It's a great question, and I think the answer is most likely a "yes".

I'll give a more thorough answer to the other comment, but there are two main reasons to have state:

  • To consolidate and de-dupe styles.
  • To enable outputting a single set of styles to a single style tag or stylesheet. (Instead of appending an additional style tag when each block is rendered).

In my experimenting so far, there doesn't appear to be a way around collecting the styles we would like to later output without having some kind of state.


/**
* Container for the main instance of the class.
*
* @since 5.5.0
* @var WP_Style_Engine_Gutenberg|null
*/
private static $instance = null;

/**
* Register action for outputting styles when the class is constructed.
*/
public function __construct() {
// Borrows the logic from `gutenberg_enqueue_block_support_styles`.
// $action_hook_name = 'wp_footer';
// if ( wp_is_block_theme() ) {
// $action_hook_name = 'wp_enqueue_scripts';
// }
// add_action(
// $action_hook_name,
// array( $this, 'output_styles' )
// );
}

/**
* Utility method to retrieve the main instance of the class.
*
* The instance will be created if it does not exist yet.
*
* @return WP_Style_Engine_Gutenberg The main instance.
*/
public static function get_instance() {
if ( null === self::$instance ) {
self::$instance = new self();
}

return self::$instance;
}

public function reset() {
$this->registered_styles = array();
}

public function get_generated_styles() {
$output = '';
foreach ( $this->registered_styles as $selector => $rules ) {
$output .= "{$selector} {\n";

if ( is_string( $rules ) ) {
$output .= ' ';
$output .= $rules;
} else {
foreach ( $rules as $rule => $value ) {
$output .= " {$rule}: {$value};\n";
}
}
$output .= "}\n";
}
return $output;
}

/**
* Stores style rules for a given CSS selector (the key) and returns an associated classname.
*
* @param string $key A class name used to construct a key.
* @param array $options An array of options, rules, and selector for constructing the rules.
*
* @return string The class name for the added style.
*/
public function add_style( $key, $options ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so If I'm understanding properly, the style engine here has a different meaning than the style engine package in JS.

The style engine package in JS is responsible for "style object -> css rules" transformation (potentially also css rules -> css string) while the current php style engine is more for "css rules -> css string". Please correct me if I'm wrong.

I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).

That said, that's fine, if we're focusing for the second part of the style engine, I'm wondering why are we building the API like that:

php

$engine->add_style( $rule );
$engine->add_style( $rule );
$engine->add_style( $rule );

$css = $engine-> output_styles();

instead of just

$css = engine_output_styles( $rules );

the latter being stateless and also more aligned with what I think will be the output of the first step of the style engine (an array of rules).

I guess the proposed API is motivated by the fact that the current block supports are "discrete" php files but personally I feel like they shouldn't be discrete, it's just the way they were implemented originally. I guess we can try to discuss this.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking this out!

I'll let @andrewserong elaborate with specific reference to this PR, but I have some general thoughts:

I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).

Agree. The first step is happening over at:

#39446 👍

I see this following the stateless approach you mention.

This PR (and #38974) is part of our explorations into what's possible and what we (think we) want to see down the track as well (CSS consolidation, optimisation and a better developer experience).

I think we also wanted to noncommittally test some of the hypotheses/feature requests that have been floating around, e.g.,

  • obfuscation
  • consisten css classname hooks
  • deduping

Layout was a good starting point for this given the amount of CSS we generate on the frontend and rule duplication.

I guess the question here is what is $resitered_styles for?

I believe it stems from the long-term idea that we could render all CSS in one style block on the frontend (instead of many), and, in doing so, leave open the possibility for pre-render processing/filtering of CSS rules.

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 the questions @youknowriad! I'll answer slightly out of order to hopefully explain some of my thinking 🙂

At the outset, I do want to flag that in the current state, this PR is weird and experimental! 😁 My approach at the moment is to stuff this PR full of ideas / hack things in there to get them working, and then gradually pull things out into something neater.

Just echoing what @ramonjd wrote above, but the goal of this PR was to take a bit of a further out look to see what kind of end point we'll need to reach if we want to achieve some of the things we're hoping to get out of the style engine work. In Ramon's PR (#39446), the stateless / closer to the JS package version is being explored, and that'll likely be in good shape to merge much sooner than this PR.

I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).

Absolutely. But I think that optimisation step could be harder to implement if we don't design for it — I'm keen for us to improve the style output (primarily de-duping and consolidating to a single stylesheet), and we get quite a bit for free if we add in the internal state and separate out the two steps (adding styles and output).

I think being able to separate generating and outputting styles into separate steps via some internal state will probably allow us some good flexibility for doing things like:

  • De-duping styles
  • Sharing classnames or styles across functions / at different execution times
  • Being able to access generated styles separately — for example, if we want to explore at some point making generated styles available via an API endpoint, having easy access to that state will make those explorations much easier

While those ideas sound very "optimise-ey" — i.e. things we'd usually think to do in later steps, and not do prematurely — here, we've got some concrete optimisations that we know we need to make and solve for, so I think it's worth it to dig in a bit to see what might look viable.

I guess the proposed API is motivated by the fact that the current block supports are "discrete" php files but personally I feel like they shouldn't be discrete, it's just the way they were implemented originally. I guess we can try to discuss this.

It's less motivated by the separate PHP files, and more motivated by styles being output one block at a time as each block is rendered, resulting in one style tag per rendered block (that uses the container id approach). We currently abstract that away in a small way in gutenberg_enqueue_block_support_styles — but my hunch at the moment is that we'll benefit from having some more smarts around this rather than relying on the wp_footer and wp_head hooks directly.

However, this is all still very much experimental! I'm more than happy for us to see how far we can get with the stateless approach in the shorter-term — my main interest right now is to ensure that if we do want to add in the stateful approach it's easy enough for us to migrate it in there to address optimisation, rather than having to rewrite everything! So, at least from my perspective, the current approach has pointed toward using a class for the style engine so that we can encapsulate (and namespace) our logic, and make it easy to add in the internal state if / when we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks that make sense.

It's less motivated by the separate PHP files, and more motivated by styles being output one block at a time as each block is rendered, resulting in one style tag per rendered block (that uses the container id approach).

That makes sense but personally what I don't like is the singleton because it's very hard to reason about depending on where you are (regular php template, api...). I'm just nitpicking at this point but I'd have a small preference for something more explicit.

Basically, what I don't like about the singleton approach (and I guess all regular WP style php) is that it's all dependent on order of execution of things, actions... It's not really clear when are we allowed to add styles to the instance, from where and who can do it. So we end up with hidden bugs, for instance what happens if I add a style to the singleton after the generate function has been called?

My "weird" mind prefers something more dumb :P

function render_blocks( $blocks ) {
   $markup = '';
   $rules = array();
   foreach( $blocks as $block ) {
      list( $css_rules, $block_markup ) = render_block( $block );
      $markup .= $block_markup;
      $rules = array_concat( $rules, $css_rules );
   }

   return [ $rules, $markup ];
}

list( $rules, $markup ) = render_blocks( $my_block_list );
$css = generate_optimized_stylesheet( $rules );

I guess it might not be easy to implement because it kinds of touches the fundamental of the existing do_blocks but anyway it's worth sharing just in case 🤷

we can also imagine that later we could also return JS in addition to "css_rules" and "markup" if we want to do JS optimizations.

list( $css_rules, $block_markup, $scripts ) = render_block( $block );

Wanted to clarify that there's no blockers from me here, I'm just basically sharing my mental model in case it helps in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just basically sharing my mental model in case it helps in any way.

Thank you, it's always very much appreciated! 😊 That code has a more JS-ey flavour to it that immediately feels more at home to me, too.

You make great points about the challenges with singletons, actions, and the order of execution. I hadn't even thought of seeing if we could mess with the render_block API 😄 — while changing the return type there could open up a can of worms, this is some very helpful nudging for us to try to keep the API we design explicit and hopefully simple to work with. I'll continue having a play!

Copy link
Member

Choose a reason for hiding this comment

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

Basically, what I don't like about the singleton approach (and I guess all regular WP style php) is that it's all dependent on order of execution of things, actions... It's not really clear when are we allowed to add styles to the instance, from where and who can do it. So we end up with hidden bugs, for instance what happens if I add a style to the singleton after the generate function has been called?

Hmm, valid point, and the example is very helpful!

I suppose we haven't really decided where we'd dump the contents of every registered style 😄 It's a good excuse to dig into render_block too to test the idea.

$suffix = ! empty( $options['suffix'] ) && is_array( $options['suffix'] ) ? implode( '--', $options['suffix'] ) : $options['suffix'];
$class = ! empty( $suffix ) ? $key . '--' . sanitize_key( $suffix ) : $key;
$selector = ! empty( $options['selector'] ) ? ' ' . trim( $options['selector'] ) : '';
$rules = ! empty( $options['rules'] ) ? $options['rules'] : array();
$prefix = ! empty( $options['prefix'] ) ? $options['prefix'] : '.';

if ( ! $class ) {
return;
}

$this->registered_styles[ $prefix . $class . $selector ] = $rules;

return $class;
}

/**
* Render registered styles as key { ...rules } for final output.
*/
public function output_styles() {
$output = $this->get_generated_styles();
echo "<style>\n$output</style>\n";
}
}
17 changes: 17 additions & 0 deletions lib/compat/wordpress-5.9/class-wp-theme-json-5-9.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,20 @@ protected static function compute_preset_classes( $settings, $selector, $origins

$stylesheet = '';
foreach ( static::PRESETS_METADATA as $preset_metadata ) {
if ( ! empty( $preset_metadata['path'] ) && 'layout' === $preset_metadata['path'][0] ) {

$preset_per_origin = _wp_array_get( $settings, $preset_metadata['path'], array() );

if ( ! empty( $preset_per_origin ) ) {
if ( isset( $preset_metadata['value_func'] ) &&
is_callable( $preset_metadata['value_func'] )
) {
$value_func = $preset_metadata['value_func'];
$stylesheet .= call_user_func( $value_func, $preset_metadata, $preset_per_origin );
continue;
}
}
}
$slugs = static::get_settings_slugs( $settings, $preset_metadata, $origins );
foreach ( $preset_metadata['classes'] as $class => $property ) {
foreach ( $slugs as $slug ) {
Expand Down Expand Up @@ -1135,6 +1149,9 @@ protected static function replace_slug_in_string( $input, $slug ) {
protected static function compute_preset_vars( $settings, $origins ) {
$declarations = array();
foreach ( static::PRESETS_METADATA as $preset_metadata ) {
if ( isset( $preset_metadata['type'] ) && 'layout' === $preset_metadata['type'] ) {
continue;
}
$values_by_slug = static::get_settings_values_by_slug( $settings, $preset_metadata, $origins );
foreach ( $values_by_slug as $slug => $value ) {
$declarations[] = array(
Expand Down
Loading