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.

Sign up for GitHub

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

Photon: Do not remove width and height attributes when known #8196

Merged
merged 4 commits into from
Nov 18, 2017
Merged

Photon: Do not remove width and height attributes when known #8196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3436270
Photon: First pass at keeping with height when both known
ebinnion Nov 16, 2017
ab3789c
Photon: Use args array to provide support for filter; Add tests
ebinnion Nov 16, 2017
5ce6719
Photon: Improve comment for updating height and width attributes
ebinnion Nov 16, 2017
423a5f3
Photon: Add regex explanation for updating width/height attributes
ebinnion Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions class.photon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,36 @@ public static function filter_the_content( $content ) {
unset( $placeholder_src );
}

// Remove the width and height arguments from the tag to prevent distortion
$new_tag = preg_replace( '#(?<=\s)(width|height)=["|\']?[\d%]+["|\']?\s?#i', '', $new_tag );
// If we are not transforming the image with resize, fit, or letterbox (lb), then we should remove
// the width and height arguments from the image to prevent distortion. Even if $args['w'] and $args['h']
// are present, Photon does not crop to those dimensions. Instead, it appears to favor height.
Copy link
Contributor

Choose a reason for hiding this comment

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

If photon favours height, could we keep the height attribute and leave out the width attribute for this case? This might go some way towards alleviating the issues with pages reflowing over and over while infinite-scrolling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking into this some more, I don't think this will work how we want.

In testing, it seems to work when the image is treated as a block level element. For example, if a paragraph immediately comes after the image, then the proper height will be reserved. But, if the image or paragraph is floated, then reflowing will still occur.

//
// If we are transforming the image via one of those methods, let's update the width and height attributes.
if ( empty( $args['resize'] ) && empty( $args['fit'] ) && empty( $args['lb'] ) ) {
$new_tag = preg_replace( '#(?<=\s)(width|height)=["|\']?[\d%]+["|\']?\s?#i', '', $new_tag );
} else {
$resize_args = isset( $args['resize'] ) ? $args['resize'] : false;
if ( false == $resize_args ) {
$resize_args = ( ! $resize_args && isset( $args['fit'] ) )
? $args['fit']
: false;
}
if ( false == $resize_args ) {
$resize_args = ( ! $resize_args && isset( $args['lb'] ) )
? $args['lb']
: false;
}

$resize_args = array_map( 'trim', explode( ',', $resize_args ) );

// (?<=\s) - Ensure width or height attribute is preceded by a space
// (width=["|\']?) - Matches, and captures, width=, width=", or width='
// [\d%]+ - Matches 1 or more digits
// (["|\']?) - Matches, and captures, ", ', or empty string
// \s - Ensures there's a space after the attribute
$new_tag = preg_replace( '#(?<=\s)(width=["|\']?)[\d%]+(["|\']?)\s?#i', sprintf( '${1}%d${2} ', $resize_args[0] ), $new_tag );
$new_tag = preg_replace( '#(?<=\s)(height=["|\']?)[\d%]+(["|\']?)\s?#i', sprintf( '${1}%d${2} ', $resize_args[1] ), $new_tag );
}

// Tag an image for dimension checking
$new_tag = preg_replace( '#(\s?/)?>(\s*</a>)?$#i', ' data-recalc-dims="1"\1>\2', $new_tag );
Expand Down
115 changes: 115 additions & 0 deletions tests/php/test_class.jetpack_photon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,4 +755,119 @@ public function test_photon_return_custom_size_array_dimensions_no_meta() {
wp_delete_attachment( $test_image );
$this->_remove_image_sizes();
}

/**
* @author ebinnion
* @covers Jetpack_Photon::filter_the_content
* @since 5.6.0
*/
public function test_photon_filter_the_content_does_not_remove_width_height_when_both_known() {
list( $sample_html ) = $this->get_photon_sample_content( 'a-tags-without-images.html' );
$filtered_content = Jetpack_Photon::filter_the_content( $sample_html );
$first_line = strtok( $filtered_content, "\n" ); // Should contain an image tag on the first line
$attributes = wp_kses_hair( $first_line, wp_allowed_protocols() );

$this->assertArrayHasKey( 'width', $attributes );
$this->assertArrayHasKey( 'height', $attributes );

// These values obtained from first image in sample content
$this->assertEquals( 631, $attributes['width']['value'] );
$this->assertEquals( 376, $attributes['height']['value'] );
}

/**
* @author ebinnion
* @covers Jetpack_Photon::filter_the_content
* @since 5.6.0
*/
public function test_photon_filter_the_content_does_not_have_width_height_when_at_least_one_not_known() {
$sample_html = '<img class="aligncenter wp-image-6372" title="Tube Bomber salmon dry fly" alt="Tube Bomber salmon dry fly" src="http://www.fishmadman.com/pages/wp-content/uploads/2012/02/Rav-fra-2004-2009-11-1024x611.jpg" width="631" />';
$filtered_content = Jetpack_Photon::filter_the_content( $sample_html );
$attributes = wp_kses_hair( $filtered_content, wp_allowed_protocols() );

$this->assertArrayNotHasKey( 'width', $attributes );
$this->assertArrayNotHasKey( 'height', $attributes );
}

/**
* @author ebinnion
* @covers Jetpack_Photon::filter_the_content
* @dataProvider photon_attributes_when_filtered_data_provider
* @since 5.6.0
*/
public function test_photon_filter_the_content_width_height_attributes_when_image_args_filtered( $filter_callback, $has_attributes, $width, $height ) {
list( $sample_html ) = $this->get_photon_sample_content( 'a-tags-without-images.html' );

add_filter( 'jetpack_photon_post_image_args', array( $this, $filter_callback ) );
$filtered_content = Jetpack_Photon::filter_the_content( $sample_html );
remove_filter( 'jetpack_photon_post_image_args', array( $this, $filter_callback ) );

$first_line = strtok( $filtered_content, "\n" ); // Should contain an image tag on the first line
$attributes = wp_kses_hair( $first_line, wp_allowed_protocols() );

if ( $has_attributes ) {
$this->assertArrayHasKey( 'width', $attributes );
$this->assertArrayHasKey( 'height', $attributes );

// These values obtained from first image in sample content
$this->assertEquals( $width, $attributes['width']['value'] );
$this->assertEquals( $height, $attributes['height']['value'] );
} else {
$this->assertArrayNotHasKey( 'width', $attributes );
$this->assertArrayNotHasKey( 'height', $attributes );
}
}

public function photon_attributes_when_filtered_data_provider() {
return array(
'photon_post_image_args_force_resize' => array(
'photon_post_image_args_force_resize',
true,
300,
250
),
'photon_post_image_args_force_fit' => array(
'photon_post_image_args_force_fit',
true,
600,
600
),
'photon_post_image_args_force_lb' => array(
'photon_post_image_args_force_lb',
true,
800,
100
),
'photon_post_image_args_force_width_only' => array(
'photon_post_image_args_force_width_only',
false,
false,
false
),
);
}

public function photon_post_image_args_force_resize() {
return array(
'resize' => '300,250'
);
}

public function photon_post_image_args_force_fit() {
return array(
'fit' => '600,600'
);
}

public function photon_post_image_args_force_lb() {
return array(
'lb' => '800,100,000000'
);
}

public function photon_post_image_args_force_width_only() {
return array(
'w' => '104'
);
}
}