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.

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

Conversation

Projects
None yet
3 participants
@amedina
Copy link
Member

amedina commented Apr 7, 2018

Fixes #1062

@amedina amedina requested a review from westonruter Apr 7, 2018

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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.

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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.

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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.

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

There is an unexpected lack of an class attribute here.

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

The width should have been extracted to a stylesheet here.

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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

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

This comment has been minimized.

@westonruter

westonruter Apr 7, 2018

Member

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

@amedina amedina force-pushed the bugfix/1062 branch from 6c095f4 to 6057f97 Apr 8, 2018

@amedina

This comment has been minimized.

Copy link
Member

amedina commented Apr 8, 2018

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

// 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, '*' ) ) {

This comment has been minimized.

@westonruter

westonruter Apr 8, 2018

Member

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.

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

This comment has been minimized.

@westonruter

westonruter Apr 8, 2018

Member

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.

This comment has been minimized.

@amedina

amedina Apr 8, 2018

Member

Nice. Will do.

@amedina amedina changed the base branch from 0.7 to develop Apr 9, 2018

@amedina amedina force-pushed the bugfix/1062 branch from 6057f97 to 11696b1 Apr 9, 2018

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

This comment has been minimized.

@westonruter

westonruter Apr 9, 2018

Member

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

@amedina amedina force-pushed the bugfix/1062 branch from f50dc69 to 926b1b4 Apr 9, 2018

@amedina 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 amedina force-pushed the bugfix/1062 branch from 926b1b4 to 1d9f9b8 Apr 9, 2018

@amedina amedina force-pushed the bugfix/1062 branch from 1d9f9b8 to b09bb46 Apr 10, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Apr 10, 2018

@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.

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

This comment has been minimized.

@westonruter

westonruter Apr 10, 2018

Member

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.

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

This comment has been minimized.

@westonruter

westonruter Apr 17, 2018

Member

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.

@westonruter westonruter merged commit 286381c into develop Apr 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the bugfix/1062 branch Apr 17, 2018

@mehigh

This comment has been minimized.

Copy link
Collaborator

mehigh commented Apr 19, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment