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 3 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', $col->getAttribute( 'style' ) . ';' . $width_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
30 changes: 22 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,28 @@ 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

Choose a reason for hiding this comment

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

The width should have been extracted to a stylesheet here.

),
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for when a col has an existing style attribute to make sure its properties don't get clobbered.

Copy link
Member

Choose a reason for hiding this comment

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

Also add a test for percentage-based width and for width with * syntax.

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.

);
}

Expand Down