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

block.json API: consider graduating __experimentalSelector and __experimentalDuotone from experimental to stable #45194

Closed
oandregal opened this issue Oct 21, 2022 · 14 comments
Assignees
Labels
[Feature] Block API API that allows to express the block paradigm. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality

Comments

@oandregal
Copy link
Member

oandregal commented Oct 21, 2022

Part of #45171

What

The __experimentalSelector and __experimentalDuotone for block.json should become stable APIs.

Why

  • __experimentalSelector has been introduced in WordPress 5.8 and evolved since. It covers certain use cases for which there is no other alternative (some blocks don't use the wp-block-<block-name> class for various reasons.
  • __experimentalDuotone has been introduced in WordPress 5.8 and evolved since. It covers the use case of providing selectors for a specific style property (duotone) and there has been no alternatives considered.

Related docs: architecture of styles.

How

Things to consider:

  • The ability to have a general selector for the whole block VS having a selector per style property. Is this fully working as expected?
  • They both cover a similar use case, providing a selector for a given style property. Can both coalesce into a single "block selector API"?
  • We should evaluate creating a migration from the old mechanisms (__experimentalSelector, __experimentalDuotone) to the new one, or communicate that we'll support both old (experimental) and new (stable) for a given period of time.
@oandregal oandregal added [Feature] Block API API that allows to express the block paradigm. [Type] Code Quality Issues or PRs that relate to code quality labels Oct 21, 2022
@oandregal oandregal changed the title block.json API: consider graduating __experimentalSelector from experimental to stable block.json API: consider graduating __experimentalSelector and __experimentalDuotone from experimental to stable Oct 21, 2022
@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Oct 21, 2022
@oandregal
Copy link
Member Author

cc @jorgefilipecosta @aaronrobertshaw @ajlende @andrewserong @ramonjd @t-hamano @tellthemachines as people that implemented or reviewed work in this area.

@ajlende
Copy link
Contributor

ajlende commented Oct 25, 2022

I've had a couple of ideas for duotone.

CSS Custom Properties

Frist, it would be possible to define a CSS custom property in the block styles to select the inner element to apply the filter to. The global styles would then set this custom property at the block level.

/* cover block style.css */
.wp-block-cover {
	/*
	 * Required to reset the filter for inner blocks
	 * so they don't have the filter applied.
	 */
	--wp--style--filter: initial;
	> .wp-block-cover__image-background,
	> .wp-block-cover__video-background {
		filter: var(--wp--style--filter);
	}
}

I started working on this one in #42870 and was running into problems with a cascade of dependencies to get it working.

I think this option is better because it better aligns with how other styles are applied to blocks—applying the style at the top level and using inheritance to style the inner parts. It has the drawback that adding duotone to a block is no longer a one line change.

Named selectors

Second, is a generic selectors option in block.json which names various landmarks within a block like media or text.

	"selectors": {
		// This would replace `__experimentalSelector`.
		// Should root be overridable? I think no, but we could make root a
		// reserved key if we want to.
		"root": ".wp-block-image",
		// We could require block supports to have a matching selector if
		// needed to make it easy to identify all the places it is used.
		// If we allow overriding the root selector, I think we should
		// include the block class here, but if not, it could be scoped
		// automatically like `__experimentalDuotone`.
		"duotone": ".wp-block-image img",
		// Additional, generic selectors could be added for themers, although
		// you'll have to be careful with not accidentally overriding the filter
		// if you have two identical selectors with different names.
		"media": ".wp-block-image img"
	},

I have an example in a gist (the gist covers more than just the selectors, so you'll have to ignore the rest).

The benefit that this one has is that plugins would have a set of named inner selectors they could rely on and adding duotone to a block would still be a one line change. The drawback is that we'd have to create a set of conventions for block authors to follow and there is bound to be disagreement on what these landmarks should be named.

@oandregal
Copy link
Member Author

oandregal commented Oct 27, 2022

CSS Custom Properties

For styles that are global, I would advise against adding CSS Custom Properties to the CSS of blocks and prioritize work on the current direction: making block CSS part of the global styles algorithm #45198 That is the last major API update we need to finalize for global styles to work well and it's under-developed.

Named selectors

Alex, in your example, I don't fully understand what media would be for, though I presume it'd be another style property that would be mapped to CSS's filter?

I don't have a strong opinion on how to stabilize this API. Whether it should be as simple as removing the __experimental prefix, making a new selectors section within block.json, or anything else. Let's try both approaches and see how it looks like in practice. This is the test I'm doing: I'm taking all the existing use cases we have in the codebase for __experimentalSelector and __experimentalDuotone selectors, and I collocate them as if a single block had them all.

  • a dedicated section would be:
{
	"selectors": {
		"root": "p",
		"border": "img, .wp-block-image__crop-area",
		"color": "table, th",
		"duotone": "img",
		"typography": ".wp-block-search__label"
	}
}
  • making it part of the existing supports (just remove the __experimental prefix):
{
	"supports": {
		"selector": "p",
		"border": {
			"selector": "img, .wp-block-image__crop-area"
		},
		"color": {
			"selector": "table, th",
			"duotone": "img"
		},
		"typography": {
			"selector": ".wp-block-search__label"
		}
	}
}

Look at the color-related selectors. One thing that stands out to me is that we already have selectors per property in addition to general and selectors per section, just not fully developed or organized. A different conversation is whether duotone should make more sense as a supports.filter.duotone.

Now that I've compared them, the separate selectors approach makes more sense to me. If we update or modify existing block supports (for example, move duotone to a filter section), we don't need to update the selectors (which we should if it was part of the supports). It makes the block selectors stand out as a first class API. And we can also grow it easily; for example, if we later need to have different selectors for background and text colors (in addition to the whole section) we could do:

{
	"selectors": {
		"color": "div",
		"color.background": "div > span",
		"color.text": "p"
	}
}

Don't mind the key names. We could use backgroundColor instead of color.background, etc. Even group them into an object. My point is that there's a growth path should we need it.

I'd think we could start stabilizing this using the selectors approach.

To prioritize velocity, we should create PRs for migrating a single selector at a time (duotone, border, color, root) until we are done. In case we don't get them all for 6.2, we perhaps could start by duotone or the root one (it'd be weird to have color but not root in the 1st version of the API, for example). We should also make sure that blocks using the __experimental approach still work as expected. We don't need to migrate all blocks at once, and perhaps it's best if we don't in the first PR, as it serves to test that both approaches still work.

What do you think?

@aaronrobertshaw
Copy link
Contributor

I'd think we could start stabilizing this using the selectors approach.

The proposed approach sounds very promising to me 👌

The current use of selectors for individual block supports is pretty heavily tied to skipping the serialization of support styles. Is it worth considering how best to stabilize the __experimentalSkipSerialization properties as part of our current plan?

For most block supports, you only need the custom selector if it isn't being applied to the block's root element, therefore also skipping that normal serialization.

@oandregal
Copy link
Member Author

The current use of selectors for individual block supports is pretty heavily tied to skipping the serialization of support styles. Is it worth considering how best to stabilize the __experimentalSkipSerialization properties as part of our current plan?

For most block supports, you only need the custom selector if it isn't being applied to the block's root element, therefore also skipping that normal serialization.

Oh, good point.

My understanding is that if the block uses __experimentalSkipSerialization there should be at least one selector declared (the top-level or the section one, and both approaches are in use in the codebase). Otherwise, the styles won't work or will be incorrectly output in global styles. Is this correct? It's been a while since I touched any of that.

I've found use cases where blocks don't have a selector but use skip serialization. Not a full list, but some I've found. Are these blocks working well with global styles (either theme.json or the GS sidebar)?:

There are also use cases where blocks have a selector but don't skip serialization:

There's also another bit about skipping serialization I don't understand yet. It's no longer a boolean, but there are cases where it uses an array. I presume it's for skip the serialization of individual properties but not the whole section?

I agree selectors and skip serialization are related and worth considering together, though I don't see an immediate 1:1 relationship. This needs to be thought a bit more. I suppose we also need to consider the state of the style engine work, which is to replace how block support styles are rendered as far as my understanding goes, hence we may no longer need __experimentalSkipSerialization?

@aaronrobertshaw
Copy link
Contributor

My understanding is that if the block uses __experimentalSkipSerialization there should be at least one selector declared (the top-level or the section one, and both approaches are in use in the codebase). Otherwise, the styles won't work or will be incorrectly output in global styles. Is this correct?

My general understanding is if the block support styles are not serialized on the outer wrapper via useBlockProps, they'll manually be applied both in the block's edit and save functions. That covers individual blocks.

For global styles, if they must target an inner element, hence skipping serialization on the wrapper, they should have a selector.

Before the introduction of feature-level selectors in #42087, there wasn't an easy way for a block to apply some support styles to a different selector than the rest. The first example I could find is the Calendar block. It needed its color styles on the table, th elements, while the typography styles needed to be on the root element.

At a quick glance, it appears that the avatar blocks, and the separator, adopted supports before feature-level selectors were available and didn't consider global styles or accepted they might not work.

The Search block might be a special case; as I recall, it actually needs to adjust a border radius value under certain conditions. Something global styles can't do at this stage.

There are also use cases where blocks have a selector but don't skip serialization:

These cases are defining root selectors where all global styles should apply. They are arbitrary, whereas other blocks rely on the default root selector of .wp-block-<name>.

I see the other selectors you proposed, keyed by the block support name, as the ones indicating if that block support skips serialization. If you are defining a custom selector to apply color support styles to, it's because it isn't the root selector; if it's not the root element, it would need to have serialization skipped for individual blocks.

Hope that logic is a little clearer than mud; feel free to poke holes in it 🙂

I presume it's for skip the serialization of individual properties but not the whole section?

From memory, that's correct. I think the Navigation block skips serialization of text decoration support so it can apply it via a CSS class name. A block author could also conceivably have both padding and margin support on a block. They might leave the margin being serialized as it makes sense on an outer wrapper, but they might skip only padding so it can be applied to an internal element.

This needs to be thought a bit more

Some extra opinions, particularly from the style engine experts @ramonjd and @andrewserong might help keep the ball rolling.

I suppose we also need to consider the state of the style engine work, which is to replace how block support styles are rendered as far as my understanding goes, hence we may no longer need __experimentalSkipSerialization?

I could be wrong, but at this stage, I think the style engine has focused more on the generation of styles rather than where and when they are rendered. Ramon and Andrew will correct me on that if I'm wrong.

The skip serialization flags are still required to my knowledge. The PHP hooks for the block supports are probably the best place to see their interaction with the style engine at the moment (e.g. colors). Those flags are used to help curate a style data object that is passed to the style engine that spits out a nice set of styles.

I might be able to do some more digging into this issue later next week if it is still required.

@oandregal
Copy link
Member Author

From memory, that's correct. I think the Navigation block skips serialization of text decoration support so it can apply it via a CSS class name. A block author could also conceivably have both padding and margin support on a block. They might leave the margin being serialized as it makes sense on an outer wrapper, but they might skip only padding so it can be applied to an internal element.

Hmm, the fact that blocks can "skip serialization" per style property (block-level styles), but they can't declare a selector per style property (global-level styles) seems problematic. It means block authors don't have a mechanism to make their blocks "global styles compatible" if they skip serialization at the style property level.

I've looked at the cases that use skip serialization per style property:

  • Navigation: it does not serialize block gap, which can't be styled via the global styles sidebar.
  • Gallery: it does not serialize text decoration, which can't be styles via the global styles sidebar.
  • Calendar: it does not serialize text & background, that can be styled via the global styles sidebar, though it provides a selector for the section and all of them (link, text, background color) work fine.

So. It's not an issue so far. But only coincidentally. When/If the global style's sidebar has support for text decoration, it would be.


Going back to the selectors proposal. I think we can start working on this, and we should not add anything here about "skip serialization": it's a different concern (block-level styles) that needs to be addressed separately.

On the other hand, it seems pressing that we provide a mechanism for declaring selectors per style property from the start to match what blocks can already do for block-level styles. There are different potential implementations. Matching the selectors object shape with the shape of any style node in theme.json is growing on me. It has a strong advantage: it's familiar to block & theme authors and extends the same shape everywhere. We still need to include something for the general selector and the selectors per section, so it's a bit different. We could do:

{
    "selector": {
        "root": "<css selector>",   
        "color": {
            "root": "<css selector>",
            "text": "<css selector>",
            "link": "<css selector>",
            "duotone": "<css selector"
        },
       "etc": { }
    }
}

Using this as base, we could offer simplifications for most common use cases, such as:

{
    "selector": "<css selector for root>",
}

or

{
  "selector": {
    "root": "<css selector for root>",
    "color": "<css selector for the color section>"
  }
}

I don't think we need those simplifications in the first PR. It'll be best to start without them for code simplicity and velocity. We can add them in a separate PR if things are working as expected.

Thoughts?

@andrewserong
Copy link
Contributor

I could be wrong, but at this stage, I think the style engine has focused more on the generation of styles rather than where and when they are rendered. Ramon and Andrew will correct me on that if I'm wrong.

Thanks for the ping! Yes, from the discussion so far, it sounds like the style engine feature set seems to be fairly separate from this. The relationship between block supports and the style engine is that the style engine receives the style object and a selector, and passes back or enqueues CSS rules. So for blocks that skip serialization, they can call the style engine directly with the CSS declarations or selector they'd like, and they become enqueued. E.g. here's where the gallery block currently does that for gap rules.

I imagine if support were to be added for using selectors for applying individual properties like padding, margin, etc, then this would happen outside of the style engine itself (i.e. in block supports code or in the block's own code) and the selector would be passed to the style engine, rather than anything in the style engine needing to be changed.

Matching the selectors object shape with the shape of any style node in theme.json is growing on me.

Matching object shapes where we can sounds good to me!

@ramonjd
Copy link
Member

ramonjd commented Oct 31, 2022

Matching the selectors object shape with the shape of any style node in theme.json is growing on me.

Also a big +1 from me. 😄

On the topic of skip serialization and block supports and their relationship to the style engine, we have a little info about it in the docs:

https://github.com/WordPress/gutenberg/blob/trunk/packages/style-engine/docs/using-the-style-engine-with-block-supports.md#checking-for-block-support-and-skip-serialization

@oandregal
Copy link
Member Author

A related thing: we may want to offer a public API method for consumers to get access to a block's selector. See this conversation.

@aaronrobertshaw
Copy link
Contributor

A related thing: we may want to offer a public API method for consumers to get access to a block's selector. See #45089 (comment).

Given the discussion on this issue to date, should that API method allow consumers access to not just the block's root selector, but also for any feature e.g. color?

@aaronrobertshaw
Copy link
Contributor

Quick update: I have picked up this issue again and will aim to have a draft PR up tomorrow.

It follows the suggested approach in matching the selectors object shape with the shape of any style node in theme.json (albeit with the addition of root selector properties).

@aaronrobertshaw
Copy link
Contributor

I have a very rough draft PR up for adding the selectors API in #46496. It doesn't address duotone at all so far, though.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 10, 2023
@aaronrobertshaw
Copy link
Contributor

Closing due to #46496 being merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

7 participants