Add block attribute selector for inline style

[westonruter]

In ampproject/amp-wp#1380 a background color attribute was added to a block. The attribute is defined as:

backgroundColor: {
	type: 'string',
	default: '#ffffff'
}

The static markup generated by the save function includes this:

<!-- wp:amp/amp-story-page {"backgroundColor":"#f78da7"} -->
<amp-story-page style="background-color:#f78da7">
...

Notice the duplication of the color.

Has there been discussion of adding a block attribute selector for inline styles? I know that inline styles have been disfavored at times, so maybe this isn't a good idea. But for this case it would eliminate the duplication.

Consider if the backgroundColor attribute could be defined instead as follows:

backgroundColor: {
	type: 'string',
	default: '#ffffff',
	selector: 'amp-story-page',
	attribute: 'style',
	property: 'background-color'
}

Then the static markup could eliminate the duplication and be stored instead as:

<!-- wp:amp/amp-story-page -->
<amp-story-page style="background-color:#f78da7">
...

[youknowriad]

I can see how this helps eliminate the duplication but we want to keep the matchers as limited as possible to allow an eventual php implementation of these matchers in the future.

I don't really think duplication in this case is harmful in any case. I'd be in favor of closing this issue for now.

[westonruter]

The PHP implementation could read the style properties too, no?

[youknowriad]

@westonruter yes it could, but given how fragile it is to parse HTML in PHP, It's safer to keep the matchers to the minimum possible set IMO (for now at least).

[westonruter]

Wouldn't the HTML be parsed via DOMDocument?

[chrisvanpatten]

There’s also some related discussion in #11212 and #10444. I think ultimately it comes down to the philosophical question of whether the markup is ultimately an artefact, like the output of a build tool, or itself an intrinsic part of the block’s “identity”?

Closely coupling data with its position in the markup makes it harder for the markup structure to change, and makes it harder to represent a block outside of an HTML context.

(I recognise there are some places where we have to store block data in markup, e.g. inner blocks and RichText, but it’s not hard to imagine a future when that changes too and an entire block tree can be interpreted without parsing markup.)

[talldan]

I actually did this in #10074, but didn't see this issue at the time, so didn't link the two together. Some information on why it never went ahead was commented by @aduth:

I think we should be very cautious about adding new matchers, for a few reasons:

• There's some desire that we could support our set of matchers in server-side parsing (Block API: Server-side awareness of block types #2751). The difficulty as it exists today is in having a reliable / performant HTML parse. Anything we add as a matcher which moves us further from this should be scrutinized. In this case, supporting inline styles would require a CSS parse implementation to recreate on the server. This is also part of why the underlying property matcher was removed in Block API: Deprecate property source #7349, since recreating a DOM object server-side would be near-impossible.
• Limiting the set of matchers that a block developer would be expected to learn, particularly in cases where alternatives may exist. Having multiple options to achieve the same goal does not seem like a necessarily good thing.
• Encouraging best practices by making the bad practices less accessible. I'm not sure whether it applies here, but in most cases we'd probably want to discourage use of inline styles in favor of classes which are (a) arguably more semantically understandable, (b) open to future revisions without invalidations, and (c) easier to extend than style values.

In this case, is it something we could have tracked as a comment attribute, even if that's then duplicated from there and the markup? Given above, I'd be more okay with the smaller trade-off of some redundancy in the comment attributes.

Is it worth keeping this issue open?

[aduth]

I think the majority of discussion here has been unfavorable / skeptical. The issue can be reopened if there's further compelling arguments, but for now it does not appear to be actionable.