Move block css from supports > __experimentalStyle to a top level style key in block.json

[adamziel]

What problem does this PR solve?

In #34180 we moved block CSS to blockJson.supports.__experimentalStyle. This PR moves it under blockJson.style at the root level. The goal is to only have a single location where styles are defined.

See WordPress/wordpress-develop#2853 for more context

cc @getdave @scruffian @draganescu

[scruffian]

LGTM

[getdave]

Lots of failing tests. Worth a rebase.

[gziolo]

It looks like there are now other core blocks that use supports.__experimentalStyle and they should get refactored as part of this PR:

• Audio
• Embed
• Image
• Navigation
• Table
• Video

[gziolo]

Lots of failing tests. Worth a rebase.

This is a valid concern because of JSON schema for block.json doesn't allow objects as array values for style property.

Related code:

gutenberg/schemas/json/block.json

Lines 458 to 485 in 1a33c0d

	"editorStyle": {
	"description": "Block type editor style definition. It will only be enqueued in the context of the editor.",
	"oneOf": [
	{
	"type": "string"
	},
	{
	"type": "array",
	"items": {
	"type": "string"
	}
	}
	]
	},
	"style": {
	"description": "Block type frontend style definition. It will be enqueued both in the editor and when viewing the content on the front of the site.",
	"oneOf": [
	{
	"type": "string"
	},
	{
	"type": "array",
	"items": {
	"type": "string"
	}
	}
	]
	},

The remaining task is documentation, as it will be very important as we extend the existing API:

https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#editor-style

https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#style

but also:

https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#wpdefinedasset

maybe it would be worth covering also in:

https://github.com/WordPress/gutenberg/blob/trunk/docs/how-to-guides/block-tutorial/applying-styles-with-stylesheets.md

In general, it's important to reference how styles can be represented with the same format as in theme.json.

[scruffian]

I added a commit to fix the schema, but I'm not sure if will work, because the test references the canonical URL of that file.

[adamziel]

@scruffian I couldn't see that commit on the list so I added this one: 1119b4c

[adamziel]

@mtias brought this up:

Styles are just block attributes; block attributes support defaults; so why wouldn’t we specify default style attributes in the attributes object?

It could look like this:

	"attributes": {
		"style": {
			"default": {
				"color": {
					"text": "#fff",
					"background": "#32373c",
				},
			},
		},
	},

It does make sense to me. At the same time, I don't like how:

• It would have special semantics based on its name (or a new special type such as style).
• It would have to be of type object (or style) and couldn't be of any other type.
• The source attribute feature couldn't be used here even though it is a valid attribute feature.
• Same goes for enum.

This tells me it's not like any other attribute – should we define it alongside other attributes then? I'd love to hear more opinions on this one – what do you think @gziolo, @dmsnell ?

[gziolo]

It never occurred to me that we could move the value stored in __experimentalStyle to the default value of the style attribute. That's a very interesting path to explore. It might be compatible with the style attribute that gets automatically injected with this code:

gutenberg/packages/block-editor/src/hooks/style.js

Lines 126 to 132 in 8b9db3d

	// Allow blocks to specify their own attribute definition with default values if needed.
	if ( ! settings.attributes.style ) {
	Object.assign( settings.attributes, {
	style: {
	type: 'object',
	},
	} );

It's defined as the type object. I don't think that we should worry about enum or source here as it's never used with style anyway.

My question here would be whether the values stored in __experimentalStyle are 100% compatible with what the block editor stores in the style attribute.

Assuming that the answer is yes, the more important consideration is the practical implications of using the default field. If we were to use that construct as is, we would encounter some limitations:

• if the block has any value set for the style then the settings from the default value (current __experimentalStyle) would get completely ignored
• if the default value for the style (current __experimentalStyle) ever changes, then it will take effect only for blocks that don't have any value set (no style modifications applied)

It isn't impossible to make it work, but we would need to explicitly define that the default value needs to get merged with the current value of the style attribute like this:

const style = { ...blockType.attributes.style.default, ...block.attributes.style };

[scruffian]

Whatever is stored in this key (whether it's __experimentalStyle, style or attributes), I don't think it should be output inline in the block. These settings can be output as CSS against the .wp-block-* selector, so they are only needed once per block type, rather than once for each block instance.

[adamziel]

@scruffian changing just the padding of the style attribute stores all the defaults with the block instance, unfortunately:

<div class="wp-block-buttons"><!-- wp:button {"style":{"border":{"//":"100% causes an oval, but any explicit but really high value retains the pill shape.","radius":"9999px"},"color":{"text":"#fff","background":"#32373c"},"typography":{"fontSize":"1.125em","textDecoration":"none"},"spacing":{"padding":{"//":"The extra 2px are added to size solids the same as the outline versions.","top":"10px","right":"calc(1.333em + 2px)","bottom":"10px","left":"calc(1.333em + 2px)"}}}} -->
<div class="wp-block-button has-custom-font-size" style="font-size:1.125em;text-decoration:none"><a class="wp-block-button__link has-text-color has-background wp-element-button" style="border-radius:9999px;color:#fff;background-color:#32373c;padding-top:10px;padding-right:calc(1.333em + 2px);padding-bottom:10px;padding-left:calc(1.333em + 2px)">text</a></div>

Storing just the updated values would require a new switch for object attributes, like:

{
    "attributes": {
        "style": {
            "type": "object",
            "default": { /* ... */ },
            "persist": "updated-properties" // Only store the changes
        },
        "another-attribute": {
            "type": "object",
            "default": { /* ... */ },
            "persist": "all-properties" // Store everything
        }
    }
}

Here's another concern: Currently, the styles are determined using the following hierarchy:

1. Core styles
2. __experimentalStyles from block.json
3. Theme.json
4. User styles
5. Block instance style attribute

Moving the styles from 2 to 5 would change that hierarchy – the block.json styles would matter more than the theme.json styles.

[MaggieCabrera]

Just a heads up that we have two places where we are putting these kinds of styles: block.json and core theme.json. It's essential that we are consistent with our approach. /cc @scruffian

Having the ability to define the styles in block.json is good for plugin devs. Deciding to move the block styles to theme.json means that the block as a component will lack those styles, even though in general it seems more convenient to have all blocks define those styles in the same place.

[scruffian]

@adamziel those seem like deal breakers. cc @mtias

[luisherranz]

I really like @mtias proposal because I love when things just work out of the box. Sadly, if it needs a separate implementation from the rest of the attributes, it kind of undermines its purpose.

From a DX point of view, if I'm not mistaken, the attribute.style attribute has been successfully abstracted from developers so far. Sure, they could learn about it (and that way, they would be learning a bit more about how styles internals work, something that I usually like), but for the sake of reducing the concepts to the minimum, I also see a benefit in mimicking the theme.json shape here and go with styles.

From an API point of view, this is quite close to the work of the descendant styles/settings (this PR and this PR from the work derived from @mtias' sections proposal). I'm worried that whenever we want to open block.json to define those descendant styles/settings defaults for a given block, we end up with conflicts or inconsistencies.

For example, would this change the background of the block, or the background of its child blocks?:

{
  "styles": {
    "color": {
      "background": "red"
    }
  }
}

The same problem exists if we go with attributes.style.

I think the decision of what API to use here should be taken with that work into consideration. /cc @mcsf @jorgefilipecosta

[oandregal]

What if we approach this the same way we did for "style variations" in themes?

• A block could have a top-level styles folder like a theme, which can store different theme.json files for each variation the block wants to register.
• The variation called styles/defaults.json is used as the block's defaults in the merge algorithm (step 2 here), so it provides defaults that the theme and user can override easily.
• In addition to automatically register the variations for the block toolbar, we could also register them in the global styles toolbar, enabling users to easily switch the style of a block type.

Conceptually, it's probably simpler for the block author to understand that the block CSS is moved from style.css into styles/defaults.json. It also makes block style variations more straightforward to declare and help us to tie them with theme.json (see a different attempt at #33232).

[adamziel]

The variation called styles/defaults.json is used as the block's defaults in the merge algorithm (#41873 (comment)), so it provides defaults that the theme and user can override easily.

The meaning of the "default" variation isn't very clear to me. Say I switch to a theme where everything has rounded corners – it would make sense to display the image blocks using their "rounded" style variation. If we do that, however, the user will see two style variation: the selected one called "rounded" and the other one called "default". This is to say that the word "default" is contextual so using a file called defaults.json could bite us later.

This requires a separate issue and explorations, though. If I compressed this too much, @MaggieCabrera can explain it much better than I can.

[luisherranz]

I like André's folder idea.

I guess what he is proposing is to add the styles directly to the JSON itself (aren't you?):

{
  "color": {
    "background": "red"
  }
}

I like that because it's very simple. But maybe we could use the block.json schema instead, so:

• It can include other fields, like name, label, or extra style or viewScript enqueues.
• There is no need for a /styles/default.json file and the defaults can be defined in the main block.json.

// block.json
{
  "name": "core/button",
  "...": "...",
  "styles": {
    "color": {
      "text": "black",
      "background": "red"
    }
  }
}

// styles/fancy-yellow-button/block.json
{
  "name": "fancy-yellow-button",
  "label": "Fancy yellow button",
  "styles": {
    "color": {
      // text remains black
      "background": "yellow"
    }
  },
  "style": "file:./fancy-yellow-button-style.css",
  "viewScript": "file:./fancy-yellow-button.js",
}

---

Actually, I've been thinking about the folders pattern for a while. Specifically, a /blocks folder that would be similar to the /patterns or /templates folder, auto-registering all blocks found inside it. It would have two levels, the project name and the block slug. Plugins should also be able to extend the block styles or block variations of other blocks (if they exist).

I also had an interesting conversation with @sunyatasattva last week about their need to override some block.json fields of the Query Loop for WooCommerce's upcoming Product Query variation which I think could be solved elegantly with this folder structure as well.

// WooCommerce Blocks plugin folder structure

/blocks
  /woocommerce
    /cart
      - block.json
      - edit.js
      - save.js
      - ...
    /checkout
      - block.json
      - edit.js
      - save.js
      - ...
  /core
    /heading
      /styles
        /product-header
          - block.json
          - product-header-styles.css // <-- required by the new style
    /query
      /variations
        /product-grid
          - block.json
          - view.js // <-- required by the new variation
          - styles.css // <-- required by the new variation

---

Anyway, I wouldn't like to distract the conversation away from this PR.

If the default styles definition can stay in the main block.json, André's folder proposal can be discussed later.

@oandregal: do you see any strong reason for using /styles/default.json instead of the main block.json?

[luisherranz]

I just read this comment from @MaggieCabrera in another issue:

Right now block style variations are a class with an associated stylesheet, but the idea is that we want to start expressing those styles using theme.json instead [...] being able to decide what style variation a block has by default.

Doing that means that the concept of "default" block style won't mean the same as it does today and it will be impossible to target because default equals no block style class name at all

I guess there's still a need for a set of default styles, isn't it? Maggie, would you mind clarifying that part?

[MaggieCabrera]

I guess there's still a need for a set of default styles, isn't it? Maggie, would you mind clarifying that part?

yes, for variations there's a few things that would be good to have:

• Be able to express what a variation is/looks like using block.json instead of CSS. Be able to customize those from the theme.json too.
• Be able to set a variation as default from theme.json, ie: my theme wants to have rounded images or outline buttons as a default (now we can set this from theme.json but we end up making the variations useless since we end up having default look the same as rounded, or fill looking like outline in this example).

For the second to work, we need to redefine what "default" means, since right now it means the absence of a class and its associated CSS, but if suddenly the default style variation for image is rounded, we don't have a way to reset the border radius to 0 if the user chooses to change their images.

[oandregal]

@oandregal: do you see any strong reason for using /styles/default.json instead of the main block.json?

No, I'm not opinionated about how a block define its styles (either within block.json or a external file).

My concerns about the current proposal (overloading the top-level style key) are:

• style is a pre-existing key that has an use case (bundling assets) and has a editorStyle counterpart. Adding new semantics to one but not the other makes things more difficult to understand.
• My major concern is: do we need to stabilize right now the __experimentalStyle? From what I've seen, it has been shipped a few weeks ago, we have barely used it yet and we've actually removed it from the first use case (button). It doesn't seem very stable or widely tested. A good test before stabilizing would be having a couple of blocks using it without any CSS. Creating an issue for listing what CSS from the core blocks needs migrating would be a good first step, so other people can help.

Additionally, we need to consider how to absorb block style variations into the merging algorithm as well. Perhaps I was too ahead of myself sharing the folders idea. I think it's promising but we should focus first on understanding how __experimentalStyles performs first.

[adamziel]

There is not enough alignment to merge this PR so I'm closing it. Feel free to take over!

[oandregal]

My major concern is: do we need to stabilize right now the __experimentalStyle? From what I've seen, it has been shipped a few weeks ago, we have barely used it yet and we've actually removed it from the first use case (#41934). It doesn't seem very stable or widely tested. A good test before stabilizing would be having a couple of blocks using it without any CSS. Creating an issue for listing what CSS from the core blocks needs migrating would be a good first step, so other people can help.

I have gone ahead and created an issue to track the next steps for this experimental API #45198