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

Introduce transform_important_qualifiers arg to AMP_Style_Sanitizer #6589

Merged
merged 1 commit into from
Sep 4, 2021
Merged

Introduce transform_important_qualifiers arg to AMP_Style_Sanitizer #6589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
102 changes: 61 additions & 41 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,16 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
* Array of flags used to control sanitization.
*
* @var array {
* @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered.
* @type bool $use_document_element Whether the root of the document should be used rather than the body.
* @type bool $require_https_src Require HTTPS URLs.
* @type callable $validation_error_callback Function to call when a validation error is encountered.
* @type bool $should_locate_sources Whether to locate the sources when reporting validation errors.
* @type string $parsed_cache_variant Additional value by which to vary parsed cache.
* @type string[] $focus_within_classes Class names in selectors that should be replaced with :focus-within pseudo classes.
* @type string[] $low_priority_plugins Plugin slugs of the plugins to deprioritize when hitting the CSS limit.
* @type bool $allow_transient_caching Whether to allow caching parsed CSS in transients. This may need to be disabled when there is highly-variable CSS content.
* @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered.
* @type bool $use_document_element Whether the root of the document should be used rather than the body.
* @type bool $require_https_src Require HTTPS URLs.
* @type callable $validation_error_callback Function to call when a validation error is encountered.
* @type bool $should_locate_sources Whether to locate the sources when reporting validation errors.
* @type string $parsed_cache_variant Additional value by which to vary parsed cache.
* @type string[] $focus_within_classes Class names in selectors that should be replaced with :focus-within pseudo classes.
* @type string[] $low_priority_plugins Plugin slugs of the plugins to deprioritize when hitting the CSS limit.
* @type bool $allow_transient_caching Whether to allow caching parsed CSS in transients. This may need to be disabled when there is highly-variable CSS content.
* @type bool $transform_important_qualifiers Whether !important rules should be transformed. This also necessarily transform inline style attributes.
* }
*/
protected $args;
Expand All @@ -141,19 +142,20 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
* @var array
*/
protected $DEFAULT_ARGS = [
'dynamic_element_selectors' => [
'dynamic_element_selectors' => [
'amp-list',
'amp-live-list',
'[submit-error]',
'[submit-success]',
'amp-script',
],
'should_locate_sources' => false,
'parsed_cache_variant' => null,
'focus_within_classes' => [ 'focus' ],
'low_priority_plugins' => [ 'query-monitor' ],
'allow_transient_caching' => true,
'skip_tree_shaking' => false,
'should_locate_sources' => false,
'parsed_cache_variant' => null,
'focus_within_classes' => [ 'focus' ],
'low_priority_plugins' => [ 'query-monitor' ],
'allow_transient_caching' => true,
'skip_tree_shaking' => false,
'transform_important_qualifiers' => true,
];

/**
Expand Down Expand Up @@ -955,12 +957,14 @@ public function sanitize() {
}
}

$elements = [];
foreach ( $this->dom->xpath->query( "//*[ @style $dev_mode_predicate ]" ) as $element ) {
$elements[] = $element;
}
foreach ( $elements as $element ) {
$this->collect_inline_styles( $element );
if ( $this->args['transform_important_qualifiers'] ) {
$elements = [];
foreach ( $this->dom->xpath->query( "//*[ @style $dev_mode_predicate ]" ) as $element ) {
$elements[] = $element;
}
foreach ( $elements as $element ) {
$this->collect_inline_styles( $element );
}
}

$this->finalize_styles();
Expand Down Expand Up @@ -1570,11 +1574,21 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) {
$cache_impacting_options = array_merge(
wp_array_slice_assoc(
$options,
[ 'property_allowlist', 'property_denylist', 'stylesheet_url', 'allowed_at_rules' ]
[
'property_allowlist',
'property_denylist',
'stylesheet_url',
'allowed_at_rules',
]
),
wp_array_slice_assoc(
$this->args,
[ 'should_locate_sources', 'parsed_cache_variant', 'dynamic_element_selectors' ]
[
'should_locate_sources',
'parsed_cache_variant',
'dynamic_element_selectors',
'transform_important_qualifiers',
]
),
[
'language' => get_bloginfo( 'language' ), // Used to tree-shake html[lang] selectors.
Expand Down Expand Up @@ -2382,10 +2396,12 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l
$this->process_font_face_at_rule( $ruleset, $options );
}

$results = array_merge(
$results,
$this->transform_important_qualifiers( $ruleset, $css_list, $options )
);
if ( $this->args['transform_important_qualifiers'] ) {
$results = array_merge(
$results,
$this->transform_important_qualifiers( $ruleset, $css_list, $options )
);
}

// Remove the ruleset if it is now empty.
if ( 0 === count( $ruleset->getRules() ) ) {
Expand Down Expand Up @@ -2605,10 +2621,12 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) {
continue;
}

$results = array_merge(
$results,
$this->transform_important_qualifiers( $rules, $css_list, $options )
);
if ( $this->args['transform_important_qualifiers'] ) {
$results = array_merge(
$results,
$this->transform_important_qualifiers( $rules, $css_list, $options )
);
}

$properties = $rules->getRules();
foreach ( $properties as $property ) {
Expand Down Expand Up @@ -3203,15 +3221,17 @@ static function ( $matches ) {
}

// Replace the somewhat-meta [style] attribute selectors with attribute selector using the data attribute the original styles are copied into.
$selector = preg_replace(
'/(?<=\[)style(?=([*$~]?=.*?)?])/is',
self::ORIGINAL_STYLE_ATTRIBUTE_NAME,
$selector,
-1,
$count
);
if ( $count > 0 ) {
$has_changed_selectors = true;
if ( $this->args['transform_important_qualifiers'] ) {
$selector = preg_replace(
'/(?<=\[)style(?=([*$~]?=.*?)?])/is',
self::ORIGINAL_STYLE_ATTRIBUTE_NAME,
$selector,
- 1,
$count
);
if ( $count > 0 ) {
$has_changed_selectors = true;
}
}

/*
Expand Down
74 changes: 74 additions & 0 deletions tests/php/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,80 @@ public function test_tree_shaking_disabled() {
$this->assertGreaterThan( 75000, strlen( implode( '', $actual_stylesheets ) ) );
}

/** @return array */
public function get_data_to_test_transform_important_qualifiers_arg() {
return [
'transform' => [
true,
[
':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .foo{color:red}',
':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .foo[data-amp-original-style*="blue"]{outline:solid 2px green}',
':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-9605c4d{background:blue}',
],
null,
],
'no_transform' => [
false,
[
'.foo{color:red !important}',
'.foo[style*="blue"]{outline:solid 2px green !important}',
],
'background: blue !important',
],
];
}

/**
* Test that transformation of !important qualifiers (and processing of style attributes) can be turned off.
*
* @dataProvider get_data_to_test_transform_important_qualifiers_arg
* @param bool $transform_important_qualifiers Sanitizer args.
* @param array $expected_stylesheets Expected stylesheets.
* @param string|null $expected_style_attr Inline style attribute value, or null if not expected.
*/
public function test_transform_important_qualifiers_arg( $transform_important_qualifiers, $expected_stylesheets, $expected_style_attr ) {
$dom = Document::fromHtml(
'
<html>
<head>
<style>
.foo { color: red !important; }
</style>
<style>
.foo[style*="blue"] { outline: solid 2px green !important; }
</style>
</head>
<body>
<div class="foo" style="background: blue !important"></div>
</body>
</html>
',
Options::DEFAULTS
);

$args = [
'use_document_element' => true,
];

$sanitizer = new AMP_Style_Sanitizer( $dom, array_merge( $args, compact( 'transform_important_qualifiers' ) ) );
$sanitizer->sanitize();

$validating_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args );
$validating_sanitizer->sanitize();

$this->assertEquals(
$expected_stylesheets,
array_values( array_filter( $sanitizer->get_stylesheets() ) )
);

$style_attr = $dom->xpath->query( '//*/@style' )->item( 0 );
if ( $style_attr instanceof DOMAttr ) {
$style_attr = $style_attr->nodeValue;
}

$this->assertEquals( $expected_style_attr, $style_attr );
}

/**
* Data for testing AMP selector conversion.
*
Expand Down