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

Supply the extracted dimensions to images determined to need them #1117

Merged
merged 2 commits into from May 5, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented May 4, 2018

There was a regression introduced in #979 whereby the dimensions determined are no longer actually supplied to the images that need them. Instead, the fallback image sizes are always used.

This issue is most plainly visible in Gutenberg where image blocks do not get output with width or height.

Additionally, this PR ensures that when a width or height are missing that the missing dimension gets its counterpart supplied based on the ratio of the underlying extracted image dimensions. So given an image that is 1944⨉1191 and post_content that contains:

<img src="https://example.com/img.jpg">

<img src="https://example.com/img.jpg" width="50">

<img src="https://example.com/img.jpg" height="50">

This gets converted to:

<p><amp-img src="https://example.com/img.jpg" width="1944" height="1191" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

<p><amp-img src="https://example.com/img.jpg" width="50" height="30.632716049383" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

<p><amp-img src="https://example.com/img.jpg" height="50" width="81.612090680101" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

image

As opposed to:

<p><amp-img src="https://example.com/img.jpg" width="600" height="400" class="amp-wp-unknown-size amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

<p><amp-img src="https://example.com/img.jpg" width="50" height="400" class="amp-wp-unknown-size amp-wp-unknown-height amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

<p><amp-img src="https://example.com/img.jpg" height="50" width="600" class="amp-wp-unknown-size amp-wp-unknown-width amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>

image

Additionally, the amp-wp-unknown-* classes will only be provided if the dimensions are actually unknown (and fallbacks are used).

This PR also restores use of content_max_width if defined instead of FALLBACK_WIDTH.

@westonruter westonruter added this to the v0.7.1 milestone May 4, 2018

@westonruter westonruter requested a review from kienstra May 4, 2018

@westonruter westonruter changed the base branch from develop to 0.7 May 4, 2018

@@ -157,30 +157,29 @@ private function determine_dimensions( $need_dimensions ) {
if ( ! $node instanceof DOMElement ) {
continue;
}
$height = $dimensions && isset( $dimensions['height'] ) ? $dimensions['height'] : self::FALLBACK_HEIGHT;
$width = $dimensions && isset( $dimensions['width'] ) ? $dimensions['width'] : self::FALLBACK_WIDTH;

This comment has been minimized.

Copy link
@westonruter

westonruter May 4, 2018

Author Member

I think $this->args['content_max_width'] should be used here if available. It got removed in #979.

$node->setAttribute( 'width', $width );
$node->setAttribute( 'height', $height );
$class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size';
$node->setAttribute( 'class', $class );
$node->setAttribute( 'class', trim( $class . ' amp-wp-unknown-size' ) );

This comment has been minimized.

Copy link
@westonruter

westonruter May 4, 2018

Author Member

Is amp-wp-unknown-size even true here? Shouldn't it only be true if the FALLBACK_HEIGHT or FALLBACK_WIDTH are used? Otherwise, if we have $dimensions then we do know the size.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented May 4, 2018

I'm working on another commit for this.

@westonruter westonruter changed the title Supply the extracted dimensions to images determined to need them [WI] Supply the extracted dimensions to images determined to need them May 4, 2018

@westonruter westonruter changed the title [WI] Supply the extracted dimensions to images determined to need them [WIP] Supply the extracted dimensions to images determined to need them May 4, 2018

Let supplied image sizes respect ratio of underlying image dimensions
* Update image tests to test for normal success case of image dimensions being extracted.
* Add test case for when fallback dimensions are provided.
* When image has one dimension but lacks another, ensure missing dimensions gets same ratio as underlying image when supplied.
* Only add amp-wp-unknown-* classes if the dimensions are actually unknown (and fallbacks are used).
* Restore use of content_max_width instead of FALLBACK_WIDTH if defined.

@westonruter westonruter changed the title [WIP] Supply the extracted dimensions to images determined to need them Supply the extracted dimensions to images determined to need them May 5, 2018

@amedina

amedina approved these changes May 5, 2018

Copy link
Member

amedina left a comment

LGTM.

@westonruter westonruter merged commit 4120ef7 into 0.7 May 5, 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 fix/determine-dimensions branch May 5, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented May 5, 2018

Approved

Hi @westonruter,
Sorry for the delay. This pull request looks good. It's nice how it uses $this->args['content_max_width'] first if there's no width.

* Class AMP_Img_Sanitizer_Test
*
* @covers AMP_Style_Sanitizer
*/

This comment has been minimized.

Copy link
@kienstra

kienstra May 5, 2018

Collaborator

Nice addition of DocBlocks 😄

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.