Convert width attribute in col tags to equivalent CSS rule by amedina · Pull Request #1064 · ampproject/amp-wp

amedina · 2018-04-07T01:34:28Z

westonruter · 2018-04-07T15:39:21Z

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

+		foreach ( $this->dom->getElementsByTagName( 'col' ) as $col ) {
+			$width_attr = $col->getAttribute( 'width' );
+			if ( ! empty( $width_attr ) ) {
+				$col->setAttribute( 'style', $width_attr . 'px' );


I think this should also account for percentage widths. Also, if it has the * syntax then I think it should be skipped so that the whitelist sanitizer will then report the attribute as a violation.

Also, I see there is a CSS syntax error here where it needs to be width: {$width_attr}px. Because there is no width property name this is causing a CSS syntax error and resulting in the lack of a class name being added in the unit test below.

Also, this should account for whether an style attribute already exists, and if so, append to the existing one.

Lastly, I wonder how this is going to play nicely with the auto-conversion of width into max-width (introduced in v0.4). This is seen in 0.7 branch:

https://github.com/Automattic/amp-wp/blob/5ba9f5bbf21ea0dfdf44d08899940c3e7d228afa/includes/sanitizers/class-amp-style-sanitizer.php#L549

And here in develop branch:

https://github.com/Automattic/amp-wp/blob/40d1945778c9917ed258e74595d504ad31719d39/includes/sanitizers/class-amp-style-sanitizer.php#L792-L801

This was introduced in #494 via #495. Nevertheless, in #610 it was suggested to be removed:

Notably, the current AMP spec does not […] specify that width should be replaced by max-width, thus these rules are no longer enforced.

So we should investigate why that conversion was introduced in the first place, and potentially come up with a better fix.

westonruter · 2018-04-07T15:53:59Z

tests/test-amp-style-sanitizer.php

+
+			'col_with_width_attribute' => array(
+				'<table><colgroup><col width="253"/></colgroup></table>',
+				'<table><colgroup><col></colgroup></table>',


There is an unexpected lack of an class attribute here.

westonruter · 2018-04-07T15:54:15Z

tests/test-amp-style-sanitizer.php

+			'col_with_width_attribute' => array(
+				'<table><colgroup><col width="253"/></colgroup></table>',
+				'<table><colgroup><col></colgroup></table>',
+				array(),


The width should have been extracted to a stylesheet here.

westonruter · 2018-04-07T15:54:45Z

tests/test-amp-style-sanitizer.php

+				'<table><colgroup><col width="253"/></colgroup></table>',
+				'<table><colgroup><col></colgroup></table>',
+				array(),
+			),


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

westonruter · 2018-04-07T15:55:11Z

tests/test-amp-style-sanitizer.php

+				'<table><colgroup><col width="253"/></colgroup></table>',
+				'<table><colgroup><col></colgroup></table>',
+				array(),
+			),


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

amedina · 2018-04-08T02:22:39Z

@westonruter Addressed your review partially. Still work in progress with a couple of things pending.

westonruter · 2018-04-08T05:21:05Z

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

+		// 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 ) && ! strpos( $width_attr, '*' ) ) {


Minor point: strpos( $width_attr, '*' ) can return 0 when the needle is at the beginning of the haystack. So it is safer to do false === strpos( $width_attr, '*' ), though * really shouldn't be at the beginning of the string either.

westonruter · 2018-04-08T05:25:41Z

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

+			if ( ! empty( $width_attr ) && ! strpos( $width_attr, '*' ) ) {
+				strpos( $width_attr, '%' )
+					? $col->setAttribute( 'style', 'width: ' . $width_attr )
+					: $col->setAttribute( 'style', 'width: ' . $width_attr . 'px' );


Using a ternary expression as a control statement is usually discouraged. I recommend doing something like this:

$width_style = "width: $width_attr"; if ( is_numeric( $width_attr ) ) { $width_style .= 'px'; } $col->setAttribute( 'style', $width_style ); // @todo Still need to be wary of there being an existing style attr.

Nice. Will do.

westonruter · 2018-04-09T22:59:00Z

includes/class-amp-theme-support.php

+		add_filter( 'wp_audio_shortcode_library', function() {
+			return 'amp';
+		} );
+


This change is here because I haven't yet merged #106 into develop from 0.7.

Reverts PR #495 for Issue #494.

westonruter · 2018-04-10T00:27:44Z

@mehigh Could we have your insights on width vs max-width and whether automatically converting them (#495) it was the best solution to the problem reported in #494 (“Aligned images with inline width larger than max get displayed off-center”)? In #610 it was suggested to be removed:

Notably, the current AMP spec does not […] specify that width should be replaced by max-width, thus these rules are no longer enforced.

It feels like it fixes a symptom but isn't the right solution to the underlying problem. This PR includes the reversion of the width/max-width change, but it should include a fix to the underlying problem then.

westonruter · 2018-04-10T03:37:38Z

tests/test-amp-style-sanitizer.php

+				'<table><colgroup><col width="0*"/></colgroup></table>',
+				'<table><colgroup><col width="0*"></colgroup></table>',
+				array(),
+			),


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.

westonruter · 2018-04-17T04:15:01Z

tests/test-amp-style-sanitizer.php

+				'<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;}',


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.

mehigh · 2018-04-19T17:59:09Z

@westonruter sorry I didn't get to this in a decent amount of time.
Instead of replacing the width by max-width, a solution used almost everywhere on the internets is to introduce a max-width: 100%; on the images. That way they would 'stay put' regardless on whether the width attribute is larger than the container or not.
But there are edge cases, and nuances, as with any solutions.

amedina requested a review from westonruter April 7, 2018 01:43

westonruter requested changes Apr 7, 2018

View reviewed changes

amedina force-pushed the bugfix/1062 branch from 6c095f4 to 6057f97 Compare April 8, 2018 02:17

westonruter reviewed Apr 8, 2018

View reviewed changes

amedina changed the base branch from 0.7 to develop April 9, 2018 22:29

amedina force-pushed the bugfix/1062 branch from 6057f97 to 11696b1 Compare April 9, 2018 22:37

westonruter reviewed Apr 9, 2018

View reviewed changes

amedina force-pushed the bugfix/1062 branch from f50dc69 to 926b1b4 Compare April 9, 2018 23:03

amedina changed the title ~~[WIP] Convert width attribute in col tags to equivalent CSS rule~~ Convert width attribute in col tags to equivalent CSS rule Apr 9, 2018

amedina force-pushed the bugfix/1062 branch from 926b1b4 to 1d9f9b8 Compare April 9, 2018 23:47

amedina added 2 commits April 9, 2018 17:19

Convert width attribute of col tag to equivalent CSS rule

407c26c

Revert changes to convert width to max-width automatically

b09bb46

Reverts PR #495 for Issue #494.

amedina force-pushed the bugfix/1062 branch from 1d9f9b8 to b09bb46 Compare April 10, 2018 00:21

Update 'style' attribute if it exists, or create it if it doesn't

11293d4

westonruter reviewed Apr 10, 2018

View reviewed changes

westonruter modified the milestones: v0.7, v1.0 Apr 12, 2018

This was referenced Apr 15, 2018

Ensure layout attributes are only allowed on supporting elements #1075

Merged

Width attribute on table col elements causes validation errors #1062

Closed

amedina added 2 commits April 16, 2018 13:22

Merge branch 'develop' into bugfix/1062

45af2ed

Add test case with existing style attribute

a1eda20

westonruter reviewed Apr 17, 2018

View reviewed changes

westonruter approved these changes Apr 17, 2018

View reviewed changes

westonruter merged commit 286381c into develop Apr 17, 2018

westonruter deleted the bugfix/1062 branch April 17, 2018 04:20

westonruter mentioned this pull request Apr 19, 2018

Revisit aligned images with inline width larger than max getting displayed off-center #1086

Closed

2 tasks

amedina self-assigned this Apr 29, 2018

westonruter mentioned this pull request Jun 1, 2018

Disallowed attribute in HTML tag 'col' or Not Vaild AMP page #1191

Closed

Conversation

amedina commented Apr 7, 2018 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

westonruter Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

amedina commented Apr 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

westonruter commented Apr 10, 2018

Uh oh!

westonruter Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehigh commented Apr 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amedina commented Apr 7, 2018 •

edited by westonruter

Loading

westonruter Apr 7, 2018 •

edited

Loading

westonruter Apr 8, 2018 •

edited

Loading

westonruter Apr 10, 2018 •

edited

Loading

westonruter Apr 17, 2018 •

edited

Loading