[WIP] Style engine: add elements and states to the backend

[ramonjd]

What?

Updates the Style Engine to add support for:

• elements styles - limited to link and button.
• individual element "states" - e.g., a:hover, focus and so on (limited to link only).

Initially only link elements are supported with the :hover pseudo selector. However the system is open to extension so in the future it will be easy to add support for more selectors.

Background context: #41383 (comment)

Related to:

• Style engine: elements backend support #40987

wp_style_engine_generate(
    array(
        'elements' => array(
            'link' => array(
                'color'  => array(
                    'text'       => '#fff',
                    'background' => '#000',
                ),				
                ':hover' => array(
                    'color' => array(
                        'text'       => '#000',
                        'background' => '#fff',
                    ),
                ),				
            ),
        ),
    )
, array( 'selector' => '.la-sinistra' ) );


/*

Returns: 

array(
    'css' => '.la-sinistra a { color: #fff; background-color: #000; } .la-sinistra a:hover { color: #000; background-color: #fff; }',
)

*/

Why?

Theme developers and user are requesting the ability to control interactivity states. By adding support to the Style Engine we prepare the ground for the code that handles both editor and front end styles to output the correct CSS based on the theme.json.

In the future this will also open the door to a UI for configuring interactivity states.

How?

The style engine will now iterate over elements key of the block styles and generate the appropriate CSS based on the rules found within those definitions.

In addition, any pseudo selectors found within elements will be generated for the given element, having first been checked against an "allowed list" (to avoid things like h1:hover which is not a best practice and should be avoided).

Testing Instructions

Follow the test instructions over at #40987.

There should be no change in the way we output link styles in elements.php

In relation to the new "states", e.g., :hover, currently this is only backend support and adding rules to theme.json will have no effect. This PR updates the style engine so check the unit tests make sense and cover the key scenarios.

npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

[ramonjd]

e33be11 adds button to the elements and also experiments with an effects => transition property, which is, unfortunately, filtered out by safecss_filter_attr() at the moment.

[getdave]

Let's avoid this and instead filter the rules defined in Core:

add_filter(
	'safe_style_css',
	function( $safe_rules ) {
		$safe_rules[] = 'transition';
		return $safe_rules;
	}
);

I added the above to load.php and it worked. There may be a more appropriate location to store this in the short term.

[ramonjd]

Nice work with the filter! Looks like a sound approach.

Thanks! 🙇

[getdave]

This is looking like a really good start. We should probably look to commit before it gets too complex.

What I didn't like is adding support for :hover without :focus so I added that. I also added :disabled for button although we could do without that for now.

[getdave]

If we go with the suggested filter approach to allow the transition property we can restore this.

[getdave]

I was wondering whether to reduce verbosity we could auto generated these states.

Something like:

'states' => generate_state_defs('link', ['hover','focus','disabled'])

Maybe that's a bit unusual in a const though?

Another option is to generate them at runtime in generate_elements_styles.

[getdave]

Ok I went ahead and dynamically generated them within the generate_elements_styles. It makes it a lot less verbose. Hope you don't mind 🙇

The only thing I'd say is that we sacrifice some measure of control for brevity. Can you think of any state selectors that we wouldn't want to map 1:1 from the state definition (i.e. hover, focus...etc) to the CSS pseudo selector equivalent (e.g. :hover, :focus .etc)?

[ramonjd]

Ok I went ahead and dynamically generated them within the generate_elements_styles. It makes it a lot less verbose. Hope you don't mind 🙇

Good one! Yeah, the first pass was rough just to check if and how such a model would work.

Can you think of any state selectors that we wouldn't want to map 1:1 from the state definition (i.e. hover, focus...etc) to the CSS pseudo selector equivalent (e.g. :hover, :focus .etc)?

I sat and stared at my keyboard for about 2 mins and couldn't come up with anything, which means there are probably zillions. But if nothing comes immediately to mind for both of us then maybe we're safe for now?

[getdave]

🤞 we should be ok. If it does I guess we could make it optional to pass an options config where you can supply more fine grained control over the mapping. But for now a simple 1:1 seems to work.

[getdave]

⚠️ Obviously this is a temporary hack and will need to find a better home. Do we have a patch open for the Core function? Do we know why it excludes transition?

[ramonjd]

It's not in the allowed attributes list but I don't know why 'transition' isn't allowed.

I wonder if there's some security concern?

There's no patch yet since this was all experimental to see if it floats.

[ramonjd]

Thanks for you help on this @getdave !

I think there's some concern with the addition of a state object, and opening up support for umpteen pseudo selectors on elements.

From what I can gather, the desire is to target :hover only, and then only on the color object.... and then only for <a /> and possibly <button />.

Does that sound right @mtias and @draganescu?

I haven't tested anything yet, or thought about it for longer than half my cup of tea, but I'm imagining extending the color object to describe the hover state:

"link": {
  "color": {
    "text": "salmon-pink",
    "background": "oyster-grey",
    "hover": {
        "text": "barramundi-white",
        "background": "mussel-brown",
     }
  },
}

Whereby we'd constrain the effect to the link element (and possibly button) on the backend.

I think this permits flexibility while avoiding adding a top-level state element.

Would that satisfy the constraints while allowing for the styles we need for :hover?

Anyway, that means this PR might be just what is was intended to be: an learning experiment destined for the circular file cabinet 🗑️ 😄

[getdave]

I think there's some concern with the addition of a state object, and opening up support for umpteen pseudo selectors on elements.

Do we actually "open up" things with a states object though? We have a hard coded const BLOCK_STYLE_DEFINITIONS_METADATA which defines exactly which states are to be supported by which elements.

So for example currently in this PR:

• link supports hover and focus.
• button supports hover, focus and disabled.

I'll add a test to ensure that simply adding a psuedo selector to your theme.json doesn't just allow the property to be generated anyway (i.e. it should be ignored unless defined in the constant "allow list").

One argument in favour of states is that it potentially makes the theme.json more readable because "state" properties don't end up mixed in with other top level properties within the element. It's clearly "these are states" and "these are defaults". I guess we also risk a potential key naming clash if we allow states to move up one level.

Also when authoring CSS my mental model for a given selector is usually:

1. Write default states
2. Add interactivity states.

When I do this I usually colocate this as follows:

.my-selector {
    color: red;
    background: blue;
}

.my-selector:hover,
.my-selector:focus {
    color: hotpink;
    background: black;
}

It might be nice to have theme.json follow this convention which is how I see the states object working.

With the alternative suggestion, the mental model breaks down a bit to become something more akin to:

.my-selector {
    color: red;
}

.my-selector:hover,
.my-selector:focus {
    color: hotpink;
}

.my-selector {
    background: blue;
}

.my-selector:hover,
.my-selector:focus {
    background: black;
}

Anyway, that means this PR might be just what is was intended to be: an learning experiment destined for the circular file cabinet 🗑️ 😄

I'm not sure it is! Seems like we can iterate towards a workable solution right here.

[ramonjd]

Do we actually "open up" things with a states object though? We have a hard coded const BLOCK_STYLE_DEFINITIONS_METADATA which defines exactly which states are to be supported by which elements.

Thanks for the comments and explanations @getdave

In relation to the application of states I'd defer to you and @draganescu, given that your brains are deep in the use case.

I haven't tested the latest changes yet – it's on my list for tomorrow – but I don't have any problems with the PR as it's currently looking (own-PR bias? 😇).

It opens us up to flexibility later, and, as you said, the style engine can take care of the constraints via the BLOCK_STYLE_DEFINITIONS_METADATA dictionary.

I was just trying to publicly capture reservations and move the debate to a public forum just in case there were things that I'd missed, like the "bigger picture"... which I always miss. 😄

[getdave]

I was just trying to publicly capture reservations and move the debate to a public forum just in case there were things that I'd missed, like the "bigger picture"... which I always miss. 😄

Absolutely 100% behind this motivation. The back and forth in this PR and in the Issue is great to show that we've considered other options.

I'm not saying I'm necessarily correct. Indeed I fully expect someone to come along later and explain to me why I'm totally wrong 😄 . Is there a way we could expose this feature as experimental until we're sure we've settled on the states format?

[getdave]

@ramonjd in the interests of simplicity let's do away with the states key. It was confusing and added a new API which we didn't need. Sorry to have persuaded that so strongly initially only to double back now 😓

I am now looking to simplify this PR to merge. I hope you don't mind but I have:

• updated to use the link > :hover > color structure.
• states has been dropped.
• simplified the PR to drop transitions, anything that is not a link element, and any pseudo selector other than :hover.

If you agree with the changes could you possible help me reinstate the tests that you previously disabled?

I also updated the PR description. As the author, I'll leave it up to you as to when you feel comfortable marking as "ready for review".

[ramonjd]

Thanks for updating the PR to fit the latest, suggested model @getdave 🙇

Just for the record, there is another alternative to this PR, which is to do nothing 😆

I mean to say, call the style engine for as many state styles as one needs:

$link = gutenberg_style_engine_generate(
    'color'   => array(
        'text' => '#fff',
    ),
    array(
        'selector' => ".some-selector a",
    )
);

$link_hover = gutenberg_style_engine_generate(
    'color'   => array(
        'text' => '#000',
    ),
    array(
        'selector' => ".some-selector a:hover",
    )
);

$my_sweet_style = "$link $link_hover";
// .some-selector a { color: #fff; } .some-selector a:hover { color: #000; }

I believe it already tried out in https://github.com/WordPress/gutenberg/pull/41383/files#diff-b85df651c33208862897162a87698bad76f15535f6432b97b022a72e99230ede.

The code is more verbose, but it avoids locking ourselves into a pattern that one day might need to be changed.

Just a thought.

By the way, I've had it on my list to port elements to the frontend implementation, so I'll take a look at that next.

I see you've been mud wrestling with that over in #41708 (comment), and have come up against similar opponents in the hooks/style.js file as I did (that's why I put it off 😄 )

[ramonjd]

When we start introducing more elements and element styles, we'll have to rebuild each element with "skip serialization" checks for all their style properties.

TODO: add a comment to the code

[getdave]

Yeh I noticed. I'd like to understand more about why link color serlization is potentially skipped. What's the deal there?

[ramonjd]

It's a block supports feature introduced a while back. With it, you can specify at the block.json level whether a block receives a particular style.

See:

• Block Supports: Allow skipping serialization of individual features #36293

[andrewserong]

Just for the record, there is another alternative to this PR, which is to do nothing

Oh, interesting idea — it keeps the data structure the same, but means that we don't need to encode additional logic in the style engine when we can defer that to the block support itself. The ability to pass in a selector to the style engine is already pretty powerful, and it'd help reduce introducing special handling.

Given our current goals for the style engine of first refactoring existing working features before trying to add new ones in, I quite like this idea of calling the style engine twice from within the elements block support.

However, we also need to think about how we'd like to handle things in the theme.json / global styles class, which currently looks up properties directly. When we get to consolidating style output with global styles, how would we expect it to work? E.g. should the check for :hover state happen in global styles or in the style engine?

For consolidating between both usages (global styles and the block support for individually rendered blocks), maybe it does make sense to keep the logic in the style engine? I suppose I'm a little torn between the two ideas!

[getdave]

Thanks @ramonjd and @andrewserong. This is a good discussion and I recognise the need to try and avoid tying ourselves to an implementation.

I mean to say, call the style engine for as many state styles as one needs

I understand. I dont have the necessary information to provide an informed response on this.

I would suggest that we need explore this alternative route in another PR to compare and contrast. Given that I'm 99.9999% convinced that we'll need to support more than just :hover on more than just link, we need to stress test our ideas against that need. So can we call the style engine repeatedly and use information stored in Global Styles/Theme JSON to determine which states are allowed on which elements?

It might well be that the style engine should remain ignorant of this complexity and rather simply generate styles provided with a given selector. My difficulty in refactoring of the front end style engine suggests this may well be the case!

I'll experiment more today and come back to you. Be assured I'm on board with the idea that less is more 😄

[ramonjd]

So can we call the style engine repeatedly and use information stored in Global Styles/Theme JSON to determine which states are allowed on which elements?

Theoretically for sure! That we're only supporting "link" in the editor for now is great help too 😅

Given that I'm 99.9999% convinced that we'll need to support more than just :hover on more than just link, we need to stress test our ideas against that need.

💯

It might well be that the style engine should remain ignorant of this complexity and rather simply generate styles provided with a given selector.

That's the general principle I'm trying to follow now for simplicity's sake.

Maybe one day, when things are working and the style engine is already integrated into WP_Theme_JSON (I'm playing around with that in #40955), one could throw an entire theme.json at the style engine and it will spit back a stylesheet. 🦄 Solve world peace too, I don't know.

It's not just you: it's all still a bit fuzzy in general. The primary goal is to consolidate where Guteneberg generates styles for now so hopefully implementing the big ideas will be more accessible to us later.

[getdave]

Just to update here, in #41786 I went down the road of keeping most of the code ignorant of pseudo selectors. Rather the code which generates the CSS rules just knows...

I have a selector and some rules for that selector and I need to turn them into valid CSS

This has proven to be the most effective option. I manipulate the data prior to feeding into the generating functions.

Would the Theme JSON generator eventually use the Style Engine? If so we should align on that method.

[ramonjd]

Thanks for the update @getdave!

Would the Theme JSON generator eventually use the Style Engine? If so we should align on that method.

That's the idea. I've got a proof of concept over at #40955. It's pretty dirty, and needs some love. I hope to clean it up soon.

By the way, I pinged you over at #42028. It'd been good to exploit your experience and ideas so far to make sure we don't engineer ourselves into a corner.

Anyway, I'm going to close this PR for now. 🎤