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.

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

Img height or width = auto results in AMP validation error #781

Closed
mmills0 opened this issue Sep 25, 2017 · 3 comments
Closed

Img height or width = auto results in AMP validation error #781

mmills0 opened this issue Sep 25, 2017 · 3 comments

Comments

@mmills0
Copy link

mmills0 commented Sep 25, 2017

Force run of determine_dimensions() on images set with height/width=auto and eliminate the AMP error due to missing height/width.

https://github.com/Automattic/amp-wp/blob/master/includes/sanitizers/class-amp-img-sanitizer.php#L40

Update to:

if ( '' === $node->getAttribute( 'width' ) || '' === $node->getAttribute( 'height' ) || 'auto' == $node->getAttribute( 'width' ) || 'auto' == $node->getAttribute( 'height' ) ) {

Or check is_numeric() instead of specifically for 'auto' ? Thoughts?

@kienstra
Copy link
Contributor

kienstra commented Dec 11, 2017

Question About Whether Issue Still Exists

Hi @mmills0,
Thanks for opening this issue. Does this issue still exist for you? It looks like the conditional checks for the attribute of 'auto' have since been deleted.

The snippet from this line in your comment above no longer has these expressions:

'auto' == $node->getAttribute( 'width' ) || 'auto' == $node->getAttribute( 'height' )

@cameronterry
Copy link

Hi @kienstra,

I'm not the creator of this issue but I can add that it is still present in the latest version of AMP (0.6.2 at the time of writing this) for me.

My specific use case is the inclusion of a third party image tag, i.e. Statista, which makes use of the "auto" value for height and a width of 100%. One example taken from here;

<a href="https://www.statista.com/chart/1129/wordpress-turns-10/" title="Infographic: WordPress Turns 10 | Statista">
   <img src="https://infographic.statista.com/normal/chartoftheday_1129_WordPress_Turns_10_b.jpg" alt="Infographic: WordPress Turns 10 | Statista" width="100%" height="auto" style="width: 100%; height: auto !important; max-width:960px;-ms-interpolation-mode: bicubic;"/>
</a>
You will find more infographics at <a href="https://www.statista.com/chartoftheday/">Statista</a>

The "height" attribute is being set to blank, due to the call to $this->filter_attributes() inside adjust_and_replace_node() method.

<amp-img src="https://infographic.statista.com/normal/chartoftheday_1129_WordPress_Turns_10_b.jpg" alt="Infographic: WordPress Turns 10 | Statista" width="600" height="" class="amp-wp-inline-4c4331ff41b44c926ba5c8b7db7b4915 amp-wp-enforced-sizes i-amphtml-element" sizes="(min-width: 600px) 600px, 100vw" style="width: 600px;"></amp-img>

I'm tempted to say that the "height" and "width" retrieved through getAttributes() should be checked in manner similar to AMP_Base_Sanitizer->sanitize_dimensions(), so that anything which is not numeric (100), pixels (100px), or percentage (100%) should be set to blank. This would then add it to the $need_dimensions array.

@westonruter
Copy link
Member

@cameronterry Is this still an issue in 0.7.x?

@kienstra kienstra closed this as completed Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants