Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes #1111

Merged
merged 4 commits into from
May 3, 2018
Merged

Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes #1111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
784061d
Prevent CSS tree shaking from removing selectors with classes mention…
westonruter May 3, 2018
c2ccff6
Fix tests related to Gutenberg and dev-lib bug
westonruter May 3, 2018
8cc3fb7
Account for empty string used in [class] attributes
westonruter May 3, 2018
52b3616
Add todo and warning about AMP-bound attributes that contain unencode…
westonruter May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev-lib
Open in desktop
Submodule dev-lib updated 6 files
+10 −38 .travis.yml
+13 −2 check-diff.sh
+0 −3 package.json
+3 −3 phpunit-plugin-bootstrap.php
+3 −3 readme.md
+42 −0 sample-config/.travis.yml
10 changes: 9 additions & 1 deletion includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public function get_stylesheets() {
}

/**
* Get list of all the class names used in the document.
* Get list of all the class names used in the document, including those used in [class] attributes.
*
* @since 1.0
* @return array Used class names.
Expand All @@ -222,6 +222,14 @@ private function get_used_class_names() {
foreach ( $this->xpath->query( '//*/@class' ) as $class_attribute ) {
$classes .= ' ' . $class_attribute->nodeValue;
}

// Find all [class] attributes and capture the contents of any single- or double-quoted strings.
foreach ( $this->xpath->query( '//*/@' . AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'class' ) as $bound_class_attribute ) {
if ( preg_match_all( '/([\'"])([^\1]*?)\1/', $bound_class_attribute->nodeValue, $matches ) ) {
$classes .= ' ' . implode( ' ', $matches[2] );
}
}

$this->used_class_names = array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) );
}
return $this->used_class_names;
Expand Down
7 changes: 6 additions & 1 deletion includes/utils/class-amp-dom-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,13 @@ public static function convert_amp_bind_attributes( $html ) {
return '<' . $tag_matches['name'] . $new_attrs . '>';
};

/*
* Match all start tags that probably contain a binding attribute.
* @todo Warning: The following pattern is brittle since it truncates HTML tags that have attributes that contain ">" or "=>".
* For example, if there exists: `<body class="foo" [class]="bodyClasses.concat( isBar ? 'bar' : '' ).filter( className => '' != className )">`
* Then this results in the following being output as the first child fo the body: `'' != className )">`
*/
$converted = preg_replace_callback(
// Match all start tags that probably contain a binding attribute.
'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#',
$replace_callback,
$html
Expand Down
52 changes: 52 additions & 0 deletions tests/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,58 @@ public function test_font_data_url_handling() {
$this->assertContains( '.dashicons-format-chat:before', $actual_stylesheets[0] );
}

/**
* Test that auto-removal (tree shaking) does not remove rules for classes mentioned in class and [class] attributes.
*
* @covers AMP_Style_Sanitizer::get_used_class_names()
* @covers AMP_Style_Sanitizer::finalize_stylesheet_set()
*/
public function test_class_amp_bind_preservation() {
ob_start();
?>
<html amp>
<head>
<meta charset="utf-8">
<style>.sidebar1 { display:none }</style>
<style>.sidebar1.expanded { display:block }</style>
<style>.sidebar2{ visibility:hidden }</style>
<style>.sidebar2.visible { display:block }</style>
<style>.nothing { visibility:hidden; }</style>
</style>
</head>
<body>
<amp-state id="mySidebar">
<script type="application/json">
{
"expanded": false
}
</script>
</amp-state>
<aside class="sidebar1" [class]="! mySidebar.expanded ? '' : 'expanded'">...</aside>
<aside class="sidebar2" [class]='mySidebar.expanded ? "visible" : ""'>...</aside>
</body>
</html>
<?php
$dom = AMP_DOM_Utils::get_dom( ob_get_clean() );

$error_codes = array();
$sanitizer = new AMP_Style_Sanitizer( $dom, array(
'use_document_element' => true,
'remove_unused_rules' => 'always',
'validation_error_callback' => function( $error ) use ( &$error_codes ) {
$error_codes[] = $error['code'];
},
) );
$sanitizer->sanitize();
$this->assertEquals( array(), $error_codes );
$actual_stylesheets = array_values( $sanitizer->get_stylesheets() );
$this->assertEquals( '.sidebar1{display:none;}', $actual_stylesheets[0] );
$this->assertEquals( '.sidebar1.expanded{display:block;}', $actual_stylesheets[1] );
$this->assertEquals( '.sidebar2{visibility:hidden;}', $actual_stylesheets[2] );
$this->assertEquals( '.sidebar2.visible{display:block;}', $actual_stylesheets[3] );
$this->assertEmpty( $actual_stylesheets[4] );
}

/**
* Test that auto-removal is performed when remove_unused_rules=sometimes (the default), and that excessive CSS will be removed entirely.
*
Expand Down
5 changes: 3 additions & 2 deletions tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ public function test_prepare_response() {
$this->assertContains( '<meta charset="' . get_bloginfo( 'charset' ) . '">', $sanitized_html );
$this->assertContains( '<meta name="viewport" content="width=device-width,minimum-scale=1">', $sanitized_html );
$this->assertContains( '<style amp-boilerplate>', $sanitized_html );
$this->assertContains( '<style amp-custom>body{background:black;}', $sanitized_html );
$this->assertRegExp( '#<style amp-custom>.*?body{background:black;}.*?</style>#s', $sanitized_html );
$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0.js" async></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-list-latest.js" async custom-element="amp-list"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-mathml-latest.js" async custom-element="amp-mathml"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
Expand All @@ -1041,9 +1041,10 @@ public function test_prepare_response() {
$this->assertContains( '<script type=\'text/javascript\' src=\'https://cdn.ampproject.org/v0/amp-ad-latest.js\' async custom-element="amp-ad"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript

$this->assertContains( '<button>no-onclick</button>', $sanitized_html );
$this->assertCount( 4, $removed_nodes );
$this->assertCount( 3, $removed_nodes );
$this->assertInstanceOf( 'DOMElement', $removed_nodes['script'] );
$this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] );
$this->assertInstanceOf( 'DOMAttr', $removed_nodes['handle'] );
}

/**
Expand Down