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
Merged

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

merged 9 commits into from
Feb 27, 2018

Conversation

davisshaver
Copy link
Contributor

@davisshaver davisshaver commented Feb 26, 2018

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

Fixes #974

@westonruter
Copy link
Member

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

@davisshaver
Copy link
Contributor Author

davisshaver commented Feb 26, 2018

opps! fixing..

@davisshaver
Copy link
Contributor Author

@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 ) ) {

/**
Copy link
Member

Choose a reason for hiding this comment

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

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 );
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be floor( $value )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ) );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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>',
),
Copy link
Member

Choose a reason for hiding this comment

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

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">

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✔️

@davisshaver
Copy link
Contributor Author

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

@davisshaver
Copy link
Contributor Author

@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
@sach-patel
Copy link

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
Copy link
Member

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

@westonruter
Copy link
Member

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

@sach-patel
Copy link

@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
Copy link
Member

@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'] );
Copy link
Member

Choose a reason for hiding this comment

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants