Transform CSS selectors according to sanitizer HTML element to AMP component conversions

[westonruter]

In Twenty Ten without AMP, the header looks like this:

In AMP, however, it looks like this:

Notice the white gap appearing in AMP where a border appears on the non-AMP version. This is due in part to Twenty Ten having a stylesheet with:

#branding img {
    border-top: 4px solid #000;
    border-bottom: 1px solid #000;
    display: block;
    float: left;
}

When the image sanitizer converts img to amp-img, this style selector then stops working as expected. Really, the style sanitizer should be converting img elements in stylesheets to amp-img. All such conversions which are done by the sanitizers should be communicated down to the style sanitizer so that it can make the necessary replacements when it processes a stylesheet.

In other words, I don't think this is something that a theme necessarily should be expected to fix by switching to use a CSS class name or manipulate with a custom sanitizer (e.g. #1074).

[westonruter]

Importantly, this also mostly fixes the the header image in Twenty Seventeen. That theme has styles like .has-header-image .custom-header-media img which are broken after the style sanitizer runs. The header image in Twenty Seventeen mostly renders properly when this patch is applied (which is not what should be merged):

--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -1292,6 +1292,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
 						$selectors = array_keys( $selectors_parsed );
 					}
 					if ( ! empty( $selectors ) ) {
+						$selectors = array_map(
+							function( $selector ) {
+								return preg_replace( '/(^|>|~|\s)img\b/', '\1amp-img', $selector );
+							},
+							$selectors
+						);
+
 						$stylesheet .= implode( ',', $selectors ) . $declaration_block;
 					}
 				}

The replacement should actually be done in a similar way to how URLs are normalized:

https://github.com/Automattic/amp-wp/blob/1e2cd22f1bfe8ab9d833c901e02111120f52fe72/includes/sanitizers/class-amp-style-sanitizer.php#L582-L590

Except instead of getAllValues we'd want to getAllDeclarationBlocks. Or better, it should be done as part of the existing process_css_declaration_block method.

[miina]

@westonruter Not sure exactly how the tree shaking works but could we in theory duplicate the CSS rules here originally and then let the tree shaking remove what's not needed instead of communicating the conversions down?

For example if there's this:

#branding img {
    border-top: 4px solid #000;
    border-bottom: 1px solid #000;
    display: block;
    float: left;
}

and we would also add this:

#branding amp-img {
    border-top: 4px solid #000;
    border-bottom: 1px solid #000;
    display: block;
    float: left;
}

¿would the tree shaking then remove the first CSS rule in case the #branding img previously gets replaced by #branding amp-img?

Edit:
I guess that's what doing something like this would do: https://github.com/Automattic/amp-wp/blob/1e2cd22f1bfe8ab9d833c901e02111120f52fe72/includes/sanitizers/class-amp-style-sanitizer.php#L582-L590

[westonruter]

The tree-shaking is done in the style sanitizer after the other sanitizers run (other than whitelist sanitizer). So when the tree shaking is done, it will see amp-img elements and not img elements. So yes, tree shaking would delete the rule with the #branding img selector.

So yes, looping over all the selectors is the right way to go. You can use \Sabberworm\CSS\CSSList\Document::getAllDeclarationBlocks() for this. And then for each declaration block get the selectors via \Sabberworm\CSS\RuleSet\DeclarationBlock::getSelectors(), and then update them via \Sabberworm\CSS\RuleSet\DeclarationBlock::setSelectors().

[kienstra]

Test Steps

Hi @csossi,
Could you please test this?

1. Visit a paired mode page, like this
2. Ensure there's no space between the header image and the menu, as shown in this comment

[csossi]

Verified in QA