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

Refines handling of 0 and empty height/width attributes #979

Merged
merged 9 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@davisshaver
Copy link
Contributor

davisshaver commented Feb 26, 2018

Alternate branch of #978 (comment) w/ spacing update.

Fixes #974

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 26, 2018

@davisshaver in #974 you call out numbers with decimals specifically but I don't see any tests that have float dimensions.

@davisshaver

This comment has been minimized.

Copy link
Contributor Author

davisshaver commented Feb 26, 2018

opps! fixing..

@davisshaver

This comment has been minimized.

Copy link
Contributor Author

davisshaver commented Feb 26, 2018

@westonruter Updated, sry about that.

@@ -174,14 +174,25 @@ protected function get_body_node() {
* @return float|int|string Returns a numeric dimension value, or an empty string.
*/
public function sanitize_dimension( $value, $dimension ) {
if ( empty( $value ) ) {
/**

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 26, 2018

Member

Minor thing, but this should be just:

// Allows 0 as valid dimension.

Same for the comment below.

The /** comments are for phpdoc specifically, and there is no need for multi-line here.

* Allows floats but parses them to integers.
*/
if ( false !== filter_var( $value, FILTER_VALIDATE_FLOAT ) ) {
return absint( $value );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 26, 2018

Member

Should this not be floor( $value )?

This comment has been minimized.

Copy link
@davisshaver

davisshaver Feb 27, 2018

Author Contributor

@westonruter Maybe? I am looking at the docs for floor() and round(). Both return floats. What about return absint( round( $value, 0 ) )?

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2018

Member

absint( -1.5 ) is 1 but floor( -1.5 ) is -1. In other words, we don't want to lose the sign, do we?

Somewhat strange in the docs there for floor():

Returns the next lowest integer value (as float)

So I suppose then this is what should be done:

return (int) floor( $value );

The @return phpdoc should be updated to remove float?

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2018

Member

Correction: floor( -1.5 ) is 2, as is floor( -1.1 ). Maybe round() is better then:

return (int) round( $value, 0 );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2018

Member

@davisshaver Hold on. I don't see how decimals are invalid AMP:

image

This comment has been minimized.

Copy link
@davisshaver

davisshaver Feb 27, 2018

Author Contributor

Agree about updating the @return statement.

I'd lean towards absint() to match the behavior on values that are already int, otherwise we may pass a negative height/width.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2018

Member

True, negative numbers are illegal. But decimal-point values are allowed. So then I think it should be the following with the two conditionals being combined:

if ( is_numeric( $value ) ) {
    return max( 0, floatval( $value ) );
}

This comment has been minimized.

Copy link
@davisshaver

davisshaver Feb 27, 2018

Author Contributor

Thanks for the discussion. I will update the PR tomorrow for another consideration

$node->setAttribute( 'class', $class );
} elseif (
! is_numeric( $node->getAttribute( 'width' ) )
) {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 26, 2018

Member

Indentation of this elseif line is inconsistent with the previous multi-line elseif.

'image_with_zero_width' => array(
'<p><img src="http://placehold.it/300x300" width="0" height="300" /></p>',
'<p><amp-img src="http://placehold.it/300x300" width="0" height="300" sizes="(min-width: 0px) 0px, 100vw" class="amp-wp-enforced-sizes"></amp-img></p>',
),

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 26, 2018

Member

Please also include a test with a zero width and height so we can ensure that the support topic is covered:

<img src="https://example.com/img.jpg" width="0" height="0" border="0">

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2018

Member

Could you add a test for this?

This comment has been minimized.

Copy link
@davisshaver

davisshaver Feb 27, 2018

Author Contributor

Done ✔️

@davisshaver

This comment has been minimized.

Copy link
Contributor Author

davisshaver commented Feb 26, 2018

Thanks for the review, will clean this up tomorrow AM!

@davisshaver

This comment has been minimized.

Copy link
Contributor Author

davisshaver commented Feb 27, 2018

@westonruter all set for another 👀 ! Much appreciated

@westonruter westonruter changed the title Refines handling of 0 and unset height/width attrs [With spacing update] Refines handling of 0 and empty height/width attributes Feb 27, 2018

@westonruter westonruter merged commit 23eced6 into ampproject:develop Feb 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sach-patel

This comment has been minimized.

Copy link

sach-patel commented Mar 15, 2018

Found this while searching for a solution where images have fractional value for the height property. It is weird but it happened during migration from Drupal to WP. Is there any way to tackle such images? Thanks.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 15, 2018

@sach-patel I wasn't able to see floats causing any validation errors.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 20, 2018

@davisshaver I have a related bug if you're interested: #1030

@sach-patel

This comment has been minimized.

Copy link

sach-patel commented Mar 28, 2018

@westonruter
Below is an example;
Post: https://thebrag.com/tldr-week-cinema-bad-cop-bad-cop/
AMP version: https://thebrag.com/tldr-week-cinema-bad-cop-bad-cop/amp/

img src="https://i.imgur.com/d1NBb20.gif" alt="" width="425" height="238.85" tag has been converted to amp-anim src="https://i.imgur.com/d1NBb20.gif" alt="" width="425" height="" sizes="(min-width: 425px) 425px, 100vw" class="amp-wp-enforced-sizes"

Google SC is reporting this as "Invalid value for attribute in layout 'height' in tag 'amp-anim'". Is there any quick solution or patch?

Thanks.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Apr 2, 2018

@sach-patel the fix is coming in the 0.7 version of the plugin. I suggest testing 0.7-beta2 which should be out this week.

// Provide default dimensions for images whose dimensions we couldn't fetch.
if ( false !== $dimensions ) {
$node->setAttribute( 'width', $dimensions['width'] );
$node->setAttribute( 'height', $dimensions['height'] );

This comment has been minimized.

Copy link
@westonruter

westonruter May 4, 2018

Member

These should not have been removed. See #1117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.