Skip to content

Commit

Permalink
Merge pull request #1117 from Automattic/fix/determine-dimensions
Browse files Browse the repository at this point in the history 
Supply the extracted dimensions to images determined to need them
  • Loading branch information
@westonruter
westonruter authored May 5, 2018
2 parents f7dcbfd + 32408e7 commit 4120ef7
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 44 deletions.
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-base-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ public function set_layout( $attributes ) {
*/
public function add_or_append_attribute( &$attributes, $key, $value, $separator = ' ' ) {
if ( isset( $attributes[ $key ] ) ) {
$attributes[ $key ] .= $separator . $value;
$attributes[ $key ] = trim( $attributes[ $key ] . $separator . $value );
} else {
$attributes[ $key ] = $value;
}
Expand Down
61 changes: 39 additions & 22 deletions includes/sanitizers/class-amp-img-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,31 +157,48 @@ private function determine_dimensions( $need_dimensions ) {
if ( ! $node instanceof DOMElement ) {
continue;
}
if (
! is_numeric( $node->getAttribute( 'width' ) ) &&
! is_numeric( $node->getAttribute( 'height' ) )
) {
$height = self::FALLBACK_HEIGHT;
$width = self::FALLBACK_WIDTH;
$class = $node->getAttribute( 'class' );
if ( ! $class ) {
$class = '';
}
if ( ! $dimensions ) {
$class .= ' amp-wp-unknown-size';
}

$width = isset( $this->args['content_max_width'] ) ? $this->args['content_max_width'] : self::FALLBACK_WIDTH;
$height = self::FALLBACK_HEIGHT;
if ( isset( $dimensions['width'] ) ) {
$width = $dimensions['width'];
}
if ( isset( $dimensions['height'] ) ) {
$height = $dimensions['height'];
}

if ( ! is_numeric( $node->getAttribute( 'width' ) ) ) {

// Let width have the right aspect ratio based on the height attribute.
if ( is_numeric( $node->getAttribute( 'height' ) ) && isset( $dimensions['height'] ) && isset( $dimensions['width'] ) ) {
$width = ( floatval( $node->getAttribute( 'height' ) ) * $dimensions['width'] ) / $dimensions['height'];
}

$node->setAttribute( 'width', $width );
if ( ! isset( $dimensions['width'] ) ) {
$class .= ' amp-wp-unknown-width';
}
}
if ( ! is_numeric( $node->getAttribute( 'height' ) ) ) {

// Let height have the right aspect ratio based on the width attribute.
if ( is_numeric( $node->getAttribute( 'width' ) ) && isset( $dimensions['width'] ) && isset( $dimensions['height'] ) ) {
$height = ( floatval( $node->getAttribute( 'width' ) ) * $dimensions['height'] ) / $dimensions['width'];
}

$node->setAttribute( 'height', $height );
$class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size';
$node->setAttribute( 'class', $class );
} elseif (
! is_numeric( $node->getAttribute( 'height' ) )
) {
$height = self::FALLBACK_HEIGHT;
$node->setAttribute( 'height', $height );
$class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-height' : 'amp-wp-unknown-size amp-wp-unknown-height';
$node->setAttribute( 'class', $class );
} elseif (
! is_numeric( $node->getAttribute( 'width' ) )
) {
$width = self::FALLBACK_WIDTH;
$node->setAttribute( 'width', $width );
$class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-width' : 'amp-wp-unknown-size amp-wp-unknown-width';
$node->setAttribute( 'class', $class );
if ( ! isset( $dimensions['height'] ) ) {
$class .= ' amp-wp-unknown-height';
}
}
$node->setAttribute( 'class', trim( $class ) );
}
}
}
Expand Down
82 changes: 61 additions & 21 deletions tests/test-amp-img-sanitizer.php
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,40 @@
<?php

/**
* Tests AMP_Style_Sanitizer.
*
* @package AMP
*/

/**
* Class AMP_Img_Sanitizer_Test
*
* @covers AMP_Style_Sanitizer
*/
class AMP_Img_Sanitizer_Test extends WP_UnitTestCase {
public static function force_remove_extraction_callbacks() {
remove_all_filters( 'amp_extract_image_dimensions_batch' );
}

/**
* Set up.
*/
public function setUp() {
parent::setUp();
add_action( 'amp_extract_image_dimensions_batch_callbacks_registered', array( __CLASS__, 'force_remove_extraction_callbacks' ) );
add_filter( 'amp_extract_image_dimensions_batch', function( $urls ) {
$dimensions = array();
foreach ( array_keys( $urls ) as $url ) {
if ( preg_match( '#/(?P<width>\d+)x(?P<height>\d+)$#', $url, $matches ) ) {
$dimensions[ $url ] = array_map( 'intval', wp_array_slice_assoc( $matches, array( 'width', 'height' ) ) );
} else {
$dimensions[ $url ] = false;
}
}
return $dimensions;
} );
}

/**
* Data for test_converter.
*
* @return array
*/
public function get_data() {
return array(
'no_images' => array(
Expand Down Expand Up @@ -38,18 +63,23 @@ public function get_data() {
),

'image_with_empty_width_and_height' => array(
'<p><img src="http://placehold.it/300x300" width="" height="" /></p>',
'<p><amp-img src="http://placehold.it/300x300" width="600" height="400" class="amp-wp-unknown-size amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
'<p><img src="http://placehold.it/200x300" width="" height="" /></p>',
'<p><amp-img src="http://placehold.it/200x300" width="200" height="300" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
),

'image_with_undefined_width_and_height' => array(
'<p><img src="http://placehold.it/200x300" /></p>',
'<p><amp-img src="http://placehold.it/200x300" width="200" height="300" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
),

'image_with_empty_width' => array(
'<p><img src="http://placehold.it/300x300" width="" height="300" /></p>',
'<p><amp-img src="http://placehold.it/300x300" width="600" height="300" class="amp-wp-unknown-size amp-wp-unknown-width amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
'<p><img src="http://placehold.it/500x1000" width="" height="300" /></p>',
'<p><amp-img src="http://placehold.it/500x1000" width="150" height="300" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
),

'image_with_empty_height' => array(
'<p><img src="http://placehold.it/300x300" width="300" height="" /></p>',
'<p><amp-img src="http://placehold.it/300x300" width="300" height="400" class="amp-wp-unknown-size amp-wp-unknown-height amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
'<p><img src="http://placehold.it/500x1000" width="300" height="" /></p>',
'<p><amp-img src="http://placehold.it/500x1000" width="300" height="600" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img></p>',
),

'image_with_zero_width' => array(
Expand Down Expand Up @@ -92,14 +122,14 @@ public function get_data() {
'<amp-img src="http://placehold.it/350x150" width="350" height="150" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img>',
),

'image_with_no_dimensions_is_forced_dimensions' => array(
'image_with_no_dimensions_is_forced' => array(
'<img src="http://placehold.it/350x150" />',
'<amp-img src="http://placehold.it/350x150" width="600" height="400" class="amp-wp-unknown-size amp-wp-enforced-sizes" layout="intrinsic"></amp-img>',
'<amp-img src="http://placehold.it/350x150" width="350" height="150" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img>',
),

'image_with_sizes_attribute_is_overridden' => array(
'<img src="http://placehold.it/350x150" width="350" height="150" />',
'<amp-img src="http://placehold.it/350x150" width="350" height="150" class="amp-wp-enforced-sizes" layout="intrinsic"></amp-img>',
'image_with_bad_src_url_get_fallback_dims' => array(
'<img src="http://example.com/404.png" />',
'<amp-img src="http://example.com/404.png" width="' . AMP_Img_Sanitizer::FALLBACK_WIDTH . '" height="' . AMP_Img_Sanitizer::FALLBACK_HEIGHT . '" class="amp-wp-unknown-size amp-wp-unknown-width amp-wp-unknown-height amp-wp-enforced-sizes" layout="intrinsic"></amp-img>',
),

'gif_image_conversion' => array(
Expand Down Expand Up @@ -147,21 +177,28 @@ public function get_data() {
}

/**
* Test converter.
*
* @param string $source Source.
* @param string $expected Expected.
* @dataProvider get_data
*/
public function test_converter( $source, $expected ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer->sanitize();
$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$this->assertEquals( $expected, $content );
}

/**
* Test that amp-anim does not get included for a PNG.
*/
public function test_no_gif_no_image_scripts() {
$source = '<img src="http://placehold.it/350x150.png" width="350" height="150" alt="Placeholder!" />';
$source = '<img src="http://placehold.it/350x150.png" width="350" height="150" alt="Placeholder!" />';
$expected = array();

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer->sanitize();

Expand All @@ -175,11 +212,14 @@ public function test_no_gif_no_image_scripts() {
$this->assertEquals( $expected, $scripts );
}

/**
* Test that amp-anim does get included for a GIF.
*/
public function test_no_gif_image_scripts() {
$source = '<img src="http://placehold.it/350x150.gif" width="350" height="150" alt="Placeholder!" />';
$source = '<img src="http://placehold.it/350x150.gif" width="350" height="150" alt="Placeholder!" />';
$expected = array( 'amp-anim' => true );

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer->sanitize();

Expand Down

0 comments on commit 4120ef7

Please sign in to comment.