Preserve Inline Styles by coreymckrill · Pull Request #470 · ampproject/amp-wp

coreymckrill · 2016-09-13T19:49:16Z

Adds the AMP_Style_Sanitizer sanitizer class, which runs before the Blacklist class, and finds all of the element nodes within the content that have a style attribute. For each one, it stores the contents of style and generates a unique class name, which it adds to the element. Then, during template rendering, it outputs these class names and their styles in the amp-custom stylesheet in the document head.

This preserves critical style information that gets stripped from the elements by the Blacklist class, which appears to be the root cause of the problems in #344, #358, #362, and #395.

This also includes unit tests for the new sanitizer class. Testing on the performance impact of this additional sanitizer class was done using this script.

This adds a new sanitizer class that collects the contents of style attributes, adds a unique class to each element with a style attribute, and then outputs it all in the `amp-custom` stylesheet.

Filter the styles using `safecss_filter_attr()` to make sure there aren't any funky properties. Also alphabetize the valid properties so that variable order doesn't cause unnecessary duplication.

Make sure that if there is nothing left after the inline styles are processed, no class is added to the element.

mjangda · 2016-09-16T22:26:30Z

tests/test-amp-style-sanitizer.php

+	private function wrap_style( $array ) {
+		ob_start(); ?>
+
+/* Inline Styles */


We're duplicating the generate_class_name and style output behaviours between the sanitizer and the tests, which means that if the implementation in the sanitizer changes but has a bug, we could end up not catching it.

Good point. Fixed with 28092c4

mjangda · 2016-09-16T22:26:55Z

includes/sanitizers/class-amp-style-sanitizer.php

+	}
+
+	private function generate_class_name( $string ) {
+		return 'amp-inline-style-' . md5( $string );


It would be nice to prefix this as amp-wp- for consistency

Fixed with 43c6972

mjangda · 2016-09-16T22:27:26Z

includes/sanitizers/class-amp-style-sanitizer.php

+			return '';
+		}
+
+		return implode( ";\n\t", $arr ) . ';';


I don't think we should worry about whitespace.

The reasoning is that this helps avoid duplication of generated style rules in the stylesheet. Without it, color: #fff; and color : #fff; would count as two separate styles.

It is an edge case, so if you feel strongly that it's not worth the extra processing, I'm happy to remove it.

Not sure I understand your example. What would the original style attribute look like in this edge case?

AMP has limits on how much CSS you can include (although, not 100% sure if whitespace counts) so we should try to keep it as slim as possible. Plus, no need to send extra bytes if we can avoid it.

What would the original style attribute look like in this edge case?

Example markup:

I really like the color green. Miguel really likes green also.

Without whitespace normalizing, the resulting amp-custom styles would be:

/* Inline Styles */ .amp-wp-inline-style-04a69db6d1a130e8e077df8a467e51dc { color: #00ff00; } .amp-wp-inline-style-bb01159393134c225a1df0d44226c3d2 { color: #00ff00; }

With whitespace normalizing:

/* Inline Styles */ .amp-wp-inline-style-bb01159393134c225a1df0d44226c3d2 { color: #00ff00; }

Oh, I think we're talking about different things :)

My comment (a bit unclear, I realize) was just referring to the addition of \n\t; not the use of trim (which is a good idea as you've outlined).

Oh! Gotcha. The \n\t just makes it easier to read the style rules when they're printed in the doc header, but I'm fine taking them out.

Fixed with a952797

mjangda · 2016-09-16T22:50:03Z

includes/sanitizers/class-amp-style-sanitizer.php

+		$this->collect_styles_recursive( $body );
+
+		if ( ! empty( $this->styles ) ) {
+			add_action( 'amp_post_template_css', array( $this, 'append_styles' ), 99 );


Any ideas on how we could skip the need to hook into actions here and pass back the generated styles some other way? (We've tried to keep Sanitizers pretty focused on just DOM manipulation and kept them free from actions/filters for various reasons.)

I initially had the same thought. I wanted to pass the data to AMP_Post_Template and add the action there, but I couldn't figure out a way to add data to the class without changing the access of some of the methods from private to public. Do you think it would be worth modifying AMP_Post_Template to make this possible?

What if we just added get_styles to the Base Sanitizer?

@mjangda is this what you're thinking of? bcc1a85

Nice, exactly!

mjangda · 2016-09-16T22:50:55Z

includes/sanitizers/class-amp-style-sanitizer.php

+					$new_class  = trim( $class . ' ' . $class_name );
+
+					$node->setAttribute( 'class', $new_class );
+					$this->styles[ $class_name ] = $style;


Why don't we just remove the style attribute as well?

Since the Blacklist sanitizer is already designed to remove attributes, including style, I thought it best followed the single responsibility principle to leave the removal part out of the Style sanitizer.

There could be a scenario where someone changes the order of the sanitizers and the Blacklist runs first, but if that happens, the Style sanitizer won't do anything anyway, since there won't be any remaining style attributes.

Another approach could be to not remove the style attribute in Blacklist, and just handle everything in the Style class. It seems like this could be confusing to other devs, though, since the Blacklist is designed specifically for removing tags and attributes...

I think in this particular case, it's probably fine for the Style Sanitizer to remove the attribute once it's done.

👍 Fixed with 7288395

Instead of duplicating the Style sanitizer's methods for generating class names and outputting style rules, the expected values are just strings. This helps avoid testing issues in the future if any of the methods change. See #470 (review)

Instead of hooking into `amp_post_template_css` directly from the sanitizer, this creates a generic `get_styles` method in the Base sanitizer class, which is then used to pass the styles as a string back to `AMP_Post_Template`, where it can be output in a post template action. This creates the possibility for other sanitizers to generate and output dynamic styles as well.

mjangda · 2016-09-26T19:46:47Z

includes/sanitizers/class-amp-style-sanitizer.php

+	}
+
+	private function generate_class_name( $string ) {
+		return 'amp-wp-inline-style-' . md5( $string );


Minor: let's remove -style from the class name.

Fixed with 4d8fc50

mjangda · 2016-09-27T03:48:02Z

includes/sanitizers/class-amp-style-sanitizer.php

+	}
+
+	public function get_styles() {
+		if ( ! empty( $this->styles ) ) {


Let's actually move have this just return the style array and have our amp_post_template_add_styles callback handle the rendering. That way, we maintain consistency with how get_scripts() works and avoid the use of ob_ (and likely makes the tests a bit cleaner too).

Yeah, this does sound a bit cleaner. Ok, stay tuned.

Instead of rendering the styles into a CSS block in the sanitizer, keep them in array form until they are loaded by the function hooked to `amp_post_template_css`.

mjangda · 2016-09-28T02:48:14Z

Just two small notes and I think we're good to merge!

* Use `printf` to make the `amp_post_template_add_styles` function more readable * Output the CSS in a compact format to minimize data footprint and whitespace

coreymckrill · 2016-09-28T03:27:19Z

Cool! Here are the changes: 5d69505

mjangda · 2016-09-28T14:24:55Z

💥 💥 💥

Thanks @coreymckrill!

coreymckrill added 6 commits September 8, 2016 19:22

Preserve inline styles: proof of concept

36670c4

This adds a new sanitizer class that collects the contents of style attributes, adds a unique class to each element with a style attribute, and then outputs it all in the `amp-custom` stylesheet.

Preserve inline styles: add routine to process styles before output

de2a303

Filter the styles using `safecss_filter_attr()` to make sure there aren't any funky properties. Also alphabetize the valid properties so that variable order doesn't cause unnecessary duplication.

Preserve inline styles: handle the AMP spec's no scrollbar rule

898b393

Preserve inline styles: normalize the whitespace within style rules

fc20773

Preserve inline styles: handle cases when processor leaves no styles

7a57c36

Make sure that if there is nothing left after the inline styles are processed, no class is added to the element.

Preserve inline styles: add unit tests

5e084e8

mjangda reviewed Sep 16, 2016

View reviewed changes

coreymckrill added 4 commits September 19, 2016 10:31

Preserve inline styles: use amp-wp- class prefix for consistency

43c6972

Preserve inline styles: remove style attribute after processing

7288395

mjangda reviewed Sep 26, 2016

View reviewed changes

coreymckrill added 2 commits September 26, 2016 16:50

Preserve inline styles: shorten generated class name

4d8fc50

Preserve inline styles: render style declarations on one line

a952797

mjangda suggested changes Sep 27, 2016

View reviewed changes

Preserve inline styles: Render styles at action hook

914ecc7

Instead of rendering the styles into a CSS block in the sanitizer, keep them in array form until they are loaded by the function hooked to `amp_post_template_css`.

mjangda approved these changes Sep 28, 2016

View reviewed changes

Preserve inline styles: Improve code readability, compact CSS

5d69505

* Use `printf` to make the `amp_post_template_add_styles` function more readable * Output the CSS in a compact format to minimize data footprint and whitespace

mjangda merged commit 867da75 into ampproject:master Sep 28, 2016

mjangda mentioned this pull request Sep 30, 2016

Aligned images with inline width larger than max get displayed off-center #494

Closed

Conversation

coreymckrill commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjangda commented Sep 28, 2016

Uh oh!

coreymckrill commented Sep 28, 2016

Uh oh!

mjangda commented Sep 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants