Open ockham opened 1 year ago
Here's the inline style grammar BTW: https://www.w3.org/TR/css-style-attr/#syntax.
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!
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? 😊
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:
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?
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.
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;' );
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')
.
What problem does this address?
The
WP_HTML_Tag_processor
class allows setting and getting of generic attribute values via itsset_attribute
andget_attribute
methods, respectively. (Changes made viaset_attribute
can be persisted to the underlying HTML by calling theget_updated_html
method.)For class names, it provides the
add_class
andremove_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, theclass
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:As a next step, we'd like to implement a
wp-style
directive: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 forclass
, I thought it might make sense to at least discussstyle
😄What is your proposed solution?
Probably some API like