-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transform CSS selectors according to sanitizer HTML element to AMP component conversions #1094
Comments
Importantly, this also mostly fixes the the header image in Twenty Seventeen. That theme has styles like --- 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: Except instead of |
@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:
and we would also add this:
¿would the tree shaking then remove the first CSS rule in case the Edit: |
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 So yes, looping over all the selectors is the right way to go. You can use |
Test Steps Hi @csossi,
|
Verified in QA |
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:
When the image sanitizer converts
img
toamp-img
, this style selector then stops working as expected. Really, the style sanitizer should be convertingimg
elements in stylesheets toamp-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).
The text was updated successfully, but these errors were encountered: