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

Convert width attribute in col tags to equivalent CSS rule #1064

Merged
merged 5 commits into from
Apr 17, 2018
Merged

Convert width attribute in col tags to equivalent CSS rule #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
407c26c
Convert width attribute of col tag to equivalent CSS rule
amedina Apr 7, 2018
b09bb46
Revert changes to convert width to max-width automatically
amedina Apr 10, 2018
11293d4
Update 'style' attribute if it exists, or create it if it doesn't
amedina Apr 10, 2018
45af2ed
Merge branch 'develop' into bugfix/1062
amedina Apr 16, 2018
a1eda20
Add test case with existing style attribute
amedina Apr 17, 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
49 changes: 26 additions & 23 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 @@ -258,6 +258,23 @@ public function sanitize() {
$elements[] = $element;
}

// If 'width' attribute is present for 'col' tag, convert to proper CSS rule.
foreach ( $this->dom->getElementsByTagName( 'col' ) as $col ) {
$width_attr = $col->getAttribute( 'width' );
if ( ! empty( $width_attr ) && ( false === strpos( $width_attr, '*' ) ) ) {
$width_style = 'width: ' . $width_attr;
if ( is_numeric( $width_attr ) ) {
$width_style .= 'px';
}
if ( $col->hasAttribute( 'style' ) ) {
$col->setAttribute( 'style', $width_style . ';' . $col->getAttribute( 'style' ) );
} else {
$col->setAttribute( 'style', $width_style );
}
$col->removeAttribute( 'width' );
}
}

/**
* Element.
*
Expand Down Expand Up @@ -459,7 +476,6 @@ private function process_link_element( DOMElement $element ) {
* @type bool $class_selector_tree_shaking Whether to perform tree shaking to delete rules that reference class names not extant in the current document.
* @type string[] $property_whitelist Exclusively-allowed properties.
* @type string[] $property_blacklist Disallowed properties.
* @type bool $convert_width_to_max_width Convert width to max-width.
* @type string $stylesheet_url Original URL for stylesheet when originating via link (or @import?).
* @type string $stylesheet_path Original filesystem path for stylesheet when originating via link (or @import?).
* @type array $allowed_at_rules Allowed @-rules.
Expand All @@ -470,7 +486,7 @@ private function process_link_element( DOMElement $element ) {
private function process_stylesheet( $stylesheet, $node, $options = array() ) {
$cache_impacting_options = wp_array_slice_assoc(
$options,
array( 'property_whitelist', 'property_blacklist', 'convert_width_to_max_width', 'stylesheet_url', 'allowed_at_rules' )
array( 'property_whitelist', 'property_blacklist', 'stylesheet_url', 'allowed_at_rules' )
);

$cache_key = md5( $stylesheet . serialize( $cache_impacting_options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize
Expand Down Expand Up @@ -519,17 +535,16 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) {

$options = array_merge(
array(
'allowed_at_rules' => array(),
'convert_width_to_max_width' => false,
'property_blacklist' => array(
'allowed_at_rules' => array(),
'property_blacklist' => array(
// See <https://www.ampproject.org/docs/design/responsive/style_pages#disallowed-styles>.
'behavior',
'-moz-binding',
),
'property_whitelist' => array(),
'validate_keyframes' => false,
'stylesheet_url' => null,
'stylesheet_path' => null,
'property_whitelist' => array(),
'validate_keyframes' => false,
'stylesheet_url' => null,
'stylesheet_path' => null,
),
$options
);
Expand Down Expand Up @@ -789,17 +804,6 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l
$this->transform_important_qualifiers( $ruleset, $css_list )
);

// Convert width to max-width when requested. See <https://github.com/Automattic/amp-wp/issues/494>.
if ( $options['convert_width_to_max_width'] ) {
$properties = $ruleset->getRules( 'width' );
foreach ( $properties as $property ) {
$width_property = new Rule( 'max-width' );
$width_property->setValue( $property->getValue() );
$ruleset->removeRule( $property );
$ruleset->addRule( $width_property, $property );
}
}

// Remove the ruleset if it is now empty.
if ( 0 === count( $ruleset->getRules() ) ) {
$css_list->remove( $ruleset );
Expand Down Expand Up @@ -1072,9 +1076,8 @@ private function collect_inline_styles( $element ) {
$rule = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue );

$stylesheet = $this->process_stylesheet( $rule, $style_attribute, array(
'convert_width_to_max_width' => true,
'allowed_at_rules' => array(),
'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'],
'allowed_at_rules' => array(),
'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'],
) );

if ( empty( $stylesheet ) ) {
Expand Down
38 changes: 30 additions & 8 deletions tests/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@ public function get_body_style_attribute_data() {
),
),

'width_to_max-width' => array(
'<figure style="width: 300px"></figure>',
'<figure class="amp-wp-343bce0"></figure>',
array(
':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-343bce0{max-width:300px;}',
),
),

'span_display_none' => array(
'<span style="display: none;">Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.</span>',
'<span class="amp-wp-224b51a">Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.</span>',
Expand Down Expand Up @@ -156,6 +148,36 @@ public function get_body_style_attribute_data() {
':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-ab79d9e{color:purple;}',
),
),

'col_with_width_attribute' => array(
'<table><colgroup><col width="253"/></colgroup></table>',
'<table><colgroup><col class="amp-wp-cbcb5c2"></colgroup></table>',
array(
':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cbcb5c2{width:253px;}',
),
),

'col_with_percent_width_attribute' => array(
'<table><colgroup><col width="50%"/></colgroup></table>',
'<table><colgroup><col class="amp-wp-cd7753e"></colgroup></table>',
array(
':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cd7753e{width:50%;}',
),
),

'col_with_star_width_attribute' => array(
'<table><colgroup><col width="0*"/></colgroup></table>',
'<table><colgroup><col width="0*"></colgroup></table>',
array(),
),
Copy link
Member

@westonruter westonruter Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add one more test with a col that has a width=50 style="background-color: red; width: 60px;". This will ensure that (1) the inline style overrides any width attribute, and (2) the new width style is correctly merged with the existing styles. In particular, this shoes that width:50px; should be prepended and not appended because here we'd need the width:60px to “win” over width:50px.


'col_with_width_attribute_and_existing_style' => array(
'<table><colgroup><col width="50" style="background-color: red; width: 60px"/></colgroup></table>',
'<table><colgroup><col class="amp-wp-c8aa9e9"></colgroup></table>',
array(
':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-c8aa9e9{width:50px;width:60px;background-color:red;}',
Copy link
Member

@westonruter westonruter Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting the behavior of the CSS parser here. It groups properties with the same name when serializing, but it preserves the order. Given a source stylesheet:

#bard {
	width: 1%;
	color: red;
	width: 123px;
	color: green;
	width: 456%;
	color: blue;
	width: 789em;
}

This gets serialized out as:

#bard{width:1%;width:123px;width:456%;width:789em;color:red;color:green;color:blue;}

So everything looks to be in order here.

),
),
);
}

Expand Down