WP_HTML_Tag_processor: Add method for style attribute handling?

[ockham]

What problem does this address?

The WP_HTML_Tag_processor class allows setting and getting of generic attribute values via its set_attribute and get_attribute methods, respectively. (Changes made via set_attribute can be persisted to the underlying HTML by calling the get_updated_html method.)

For class names, it provides the add_class and remove_class methods. They include some extra logic to make sure that e.g. class names are deduplicated, and that upon removing the last existing class name, the class attribute is removed altogether.

This comes in very handy for the Block Interactivity API project, where we're implementing a wp-class directive, with semantics as follows:

<div wp-class:red="myblock.someValue" class="blue"></div>

is transformed to:

<div wp-class:red="myblock.truthyValue" class="blue red"></div>

As a next step, we'd like to implement a wp-style directive:

<div wp-style:color="myblock.someValue" style="background: blue;"></div>

should be transformed to:

<div
  wp-style:color="myblock.greenValue"
  style="background: blue; color: green;"
></div>

I can of course implement the relevant logic manually in the Block Interactivity API SSR handling code, but I wanted to ask if y'all @adamziel @dmsnell would be open to implement it "natively" in the WP_HTML_Tag_Processor. Arguably, it's just syntactic sugar, but since there's already some logic for class, I thought it might make sense to at least discuss style 😄

What is your proposed solution?

Probably some API like

$p->get_style( 'color' );
$p->set_style( 'color', 'green' );
$p->remove_style( 'color' );

[ockham]

Here's the inline style grammar BTW: https://www.w3.org/TR/css-style-attr/#syntax.

[dmsnell]

We discussed this but as of yet have not felt comfortable taking on the work. It may be that properly handling that is viable, but it seems to pose a potential for large scope creep.

Specifically I have had concerns with the semantics involved escalating beyond what we are able to support, for things like the properties which have multiple representations:

$p->set_style( 'border', '0 1em' );
$p->set_style( 'border-top', '0.5em' );
$p->set_style( 'border': 'dashed' );

In this case I'm not sure what the outcome should be; it seems like (a) that's not actually valid inline CSS based on testing in Safari (b) it might be blanket replacing prior declarations with the duplicated ones based on further testing with padding as the compound property.

It could be this is easier than we realize, but without knowing that (via working through the CSS spec) I haven't been able to think properly about adding it. If it's truly as simple as appending the style to the end of the string as I think it might be then I question if there's much here beyond $p->set_attribute( 'style', ($p->get_attribute('style') ?? '') . " {$new_style}")

I've considered an updater function like set_attribute( 'style', function ( $prev ) { return ( $prev ?? '' ) . " border: 1em;"; } ) but there are enough catches with that in PHP (for example, the default of not enclosing variables or the syntactical cumbersomeness of it) that I have avoided it so far.

$p->append_to_attribute( 'style', ' border: 4px' ) is an option but I'm not sure if that's generic enough to be widely applicable, or similarly if $p->add_inline_style() is broad enough to be wildly valuable.

Thoughts are welcome!

[ockham]

I was leaning towards something modeled after set_attribute / get_attribute: I'd make set_style smart enough to avoid duplicating styles by locating an existing style declaration and, if present, replacing its value with the new one -- and otherwise, appending it.

Applying this to your example...

$p->set_style( 'border', '0 1em' );
$p->set_style( 'border-top', '0.5em' );
$p->set_style( 'border': 'dashed' );

... this would result in:

border: 0 1em;
border: 0 1em; border-top: 0.5em;
border: dashed; border-top: 0.5em;

it seems like (a) that's not actually valid inline CSS based on testing in Safari

Curious what would make it invalid -- would it be the border, if indeed duplicated, as in border: 0 1em; border-top: 0.5em; border: dashed?)

I've considered an updater function like set_attribute( 'style', function ( $prev ) { return ( $prev ?? '' ) . " border: 1em;"; } ) but there are enough catches with that in PHP (for example, the default of not enclosing variables or the syntactical cumbersomeness of it) that I have avoided it so far.

Yeah, not a fan of that syntax 😬

$p->append_to_attribute( 'style', ' border: 4px' ) is an option but I'm not sure if that's generic enough to be widely applicable, or similarly if $p->add_inline_style() is broad enough to be wildly valuable.

Not sure if that append_to_attribute adds enough value over calling $old_style = get_attribute( 'style' ); and set_attribute( 'style', $old_style . ' border: 4px' ) subsequently. (The main value proposition of dedicated get_style()/set_style methods would be deduplication of style declaration for me, as detailed in the first paragraph.)

---

Anyway, my thinking is of course shaped by the my use case. I've started implementing what I've described here; how about we see how that evolves and depending on how much we like it, we can consider it for inclusion in WP_HTML_Tag_Processor later? 😊

[dmsnell]

I'd make set_style smart enough to avoid duplicating styles by locating an existing style declaration and, if present, replacing its value with the new one

This is what scares me, largely because of my ignorance. I have been worried that if we replace border: 1px dashed with border: 3px we might be removing something we don't want to. It appears like it should be safe, but so far that's been based on a few random tests in Safari.

The border funny business was me trying to think of test cases for the combined rules and realizing that border isn't the easiest one to use - better to just ignore what I mentioned in (a) about invalid CSS.

At first I thought replacing and duplicating would be identical, but I think that's wrong, and also that your example produces the wrong output. Of note, in HTML duplicated attributes are ignored and the first attribute of a given comparable name is picked as the one providing a value. For inline CSS this is not the case and duplicated rules overwrite the semantics of any rule that would set the same property which comes before it.

<span style="border: 3px dashed; border-top: 9px solid red; border: blue;">Welcome to the world’s</span>

<span style="border: 3px dashed; border-top: 9px solid red;">Welcome to the world’s</span>

So in your example the update border: dashed should have wiped out not only the border: 0 1em but also the border-top: 0.5em;. Instead, we've replayed the border-top and produced something that doesn't result in what we would expect if we were to manually add the inline CSS.

Or maybe we could argue that we're building an inline CSS builder system and we want it to behave differently, or argue that calling set_style() shouldn't be seen as appending inline styles to the style attribute, but rather doing something special to interpret its updates in a CSS builder convention.

I'm deeply concerned that we're going to find case after case where these nuances appear and reveal that we built an abstraction that merely hides the underlying system and traps us into a new language that's like CSS but distinct from it. Or put another way, I don't feel comfortable answering the question "what should the effect of this update be?" given that we don't have a clear and simple semantic for applying those updates, as we largely do in the case of adding or removing classes.

You may not that the class logic is broken: we don't properly handle cases where classes have been encoded with character references. It's on the TODO list to properly handle this at some point, but so far that's been a compromise we made for performance and development pace. Still, fixing that issue has a clear end: class is a whitespace-separated set of names. Names can only either exist or not exist, whereas inline style incorporates a styling engine:

• if we handle the semantics of that engine we're going to have to make a lot of hard calls and determine if we implement the complicated spec
• if we stick with the simpler approach of appending inline styles so that we preserve the browser behaviors then this abstraction isn't adding much at all

[adamziel]

Just noting $p->set_style( 'color', 'red' ) is a syntax sugar for:

$style = $p->get_attribute( 'style' );
$new_style = update_style( $style, array(
    'color' => 'red'
) );
$p->set_attribute( 'style', $new_style );

This issue is really about figuring out that update_style function. If one was readily available, I'd have nothing against adding set_style to the API of WP_HTML_Tag_Processor.

@ockham how important would you say this is for the interactivity API?

[aristath]

Just noting that we have a Style Engine we could & should use if we build this.
We need to add an add_declarations_from_string() method in WP_Style_Engine_CSS_Declarations which will parse the existing CSS of the style attribute, and then use the object's methods to add the new styles we want and get the result.

$declarations = new WP_Style_Engine_CSS_Declarations();
$declarations->add_declarations_from_string( $existing_styles_attr );
$declarations->add_declaration( 'color', 'red' );
$declarations->add_declarations( [
    'border'     => '0 1em',
    'border-top' => '0.5em',
] );
$result = $declarations->get_declarations_string();

The style-engine will automatically remove any duplicates (when defining 2 border properties, only the 2nd takes effect in browsers so the engine will remove the 1st one), sanitize/filter the CSS, and optimize it.

[dmsnell]

I'm curious what the advantages are of adding this into the HTML API vs. letting people use the style engine themselves.

$declarations = new WP_Style_Engine_CSS_Declarations();
$declarations->add_declarations_from_String( $p->get_attribute( 'style' ) ?? '' );
$declarations->add_declaration( 'color', 'red' );
$p->set_attribute( 'style', $declarations->get_declarations_string() );

It looks more verbose but also keeps a clearer separation of responsibilities between the HTML processing and the style processing. It's easily abstractable in some function if people don't like typing.

/**
 * @param WP_HTML_Tag_Processor $p Tag Processor at a given tag to modify.
 * @param Array $styles Style name/attribute pairs to add.
function add_inline_style( $p, $styles ) {
	$declarations = new WP_Style_Engine_CSS_Declarations();
	$declarations->add_declarations_from_string( $p->get_attribute( 'style' ) ?? '' );

	foreach ( $styles as $name => $value ) {
		$declarations->add_declaration( $name, $value );
	}

	$style = $declarations->get_declarations_string();
	$p->set_attribute( 'style', $style );

	return $style;
}

when defining 2 border properties, only the 2nd takes effect in browsers so the engine will remove the 1st one

This is something that still worries me about putting it directly in the HTML API, where I'd prefer to keep things as close to the spec as we can. The style engine gets this situation wrong, which is demonstrated by using a mixture of your code sample and what I mentioned above.

Handling the style attributes involves more than just parsing the syntax; I like having separate domains with different expectations for the reliability and performance characteristics. Of course it would also be great if we fixed these cases such as found in the style engine, but I'm still not acquainted with the semantic rules governing how this attribute's properties are handled.

The complexity raised by the question leads me to lean even more on the side of "don't provide anything and let people append values and leave processing to the browser."

$p->set_attribute( 'style', ( $p->get_attribute( 'style' ) ?? '' ) . '; color: red;' );

[eric-michel]

I know this thread is quite old, but I'm working on adding some CSS custom props to Cover blocks for reuse later (namely, the native height and width of the contained image). Best I can tell, something like this is still the only way to really do that:

function example_add_custom_styles_to_cover_block( $block_content, $block ) {

	$image_id = $block['attrs']['id'];
	$image_dimensions = wp_get_attachment_image_src( $image_id, 'full' );
	$image_width = $image_dimensions[1];
	$image_height = $image_dimensions[2];
	$processor = new WP_HTML_Tag_Processor( $block_content );

	if ( $processor->next_tag( array( 'class_name' => 'wp-block-cover' ) ) ) {
		$current_style = $processor->get_attribute( 'style' );
		$processor->set_attribute( 'style', '--native-width:' . $image_width . ';--native-height:' . $image_height . ';' . $current_style );
	}

	return $processor->get_updated_html();
}
add_filter( 'render_block_core/cover', 'example_add_custom_styles_to_cover_block', 10, 2 );

I really like the idea of @dmsnell's example utilizing the style engine to insert style attributes. It looks like the only thing currently missing from that method is a function to parse the declarations into an array usable by the WP_Style_Engine_CSS_Declarations class from the string returned by get_attribute('style').