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
Changes from all commits
27c38e9
722d173
ba2aa93
94424d6
6bdc18c
b1da905
88d9d7a
6926f9b
929698e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,7 @@ public function sanitize() { | |
} | ||
|
||
// Determine which images need their dimensions determined/extracted. | ||
if ( '' === $node->getAttribute( 'width' ) || '' === $node->getAttribute( 'height' ) ) { | ||
if ( ! is_numeric( $node->getAttribute( 'width' ) ) || ! is_numeric( $node->getAttribute( 'height' ) ) ) { | ||
$need_dimensions[ $node->getAttribute( 'src' ) ][] = $node; | ||
} else { | ||
$this->adjust_and_replace_node( $node ); | ||
|
@@ -154,18 +154,30 @@ private function determine_dimensions( $need_dimensions ) { | |
if ( ! $node instanceof DOMElement ) { | ||
continue; | ||
} | ||
|
||
// Provide default dimensions for images whose dimensions we couldn't fetch. | ||
if ( false !== $dimensions ) { | ||
$node->setAttribute( 'width', $dimensions['width'] ); | ||
$node->setAttribute( 'height', $dimensions['height'] ); | ||
} else { | ||
$width = isset( $this->args['content_max_width'] ) ? $this->args['content_max_width'] : self::FALLBACK_WIDTH; | ||
if ( | ||
! is_numeric( $node->getAttribute( 'width' ) ) && | ||
! is_numeric( $node->getAttribute( 'height' ) ) | ||
) { | ||
$height = self::FALLBACK_HEIGHT; | ||
$width = self::FALLBACK_WIDTH; | ||
$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 ); | ||
} 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' ) ) | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation of this |
||
$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 ); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,42 +12,72 @@ public function setUp() { | |
|
||
public function get_data() { | ||
return array( | ||
'no_images' => array( | ||
'no_images' => array( | ||
'<p>Lorem Ipsum Demet Delorit.</p>', | ||
'<p>Lorem Ipsum Demet Delorit.</p>', | ||
), | ||
|
||
'image_without_src' => array( | ||
'image_without_src' => array( | ||
'<p><img width="300" height="300" /></p>', | ||
'<p></p>', | ||
), | ||
|
||
'image_with_empty_src' => array( | ||
'image_with_empty_src' => array( | ||
'<p><img src="" width="300" height="300" /></p>', | ||
'<p></p>', | ||
), | ||
|
||
'image_with_self_closing_tag' => array( | ||
'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" sizes="(min-width: 600px) 600px, 100vw"></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" sizes="(min-width: 600px) 600px, 100vw"></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" sizes="(min-width: 300px) 300px, 100vw"></amp-img></p>', | ||
), | ||
|
||
'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>', | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done ✔️ |
||
|
||
'image_with_zero_width_and_height' => array( | ||
'<p><img src="http://placehold.it/300x300" width="0" height="0" /></p>', | ||
'<p><amp-img src="http://placehold.it/300x300" width="0" height="0" sizes="(min-width: 0px) 0px, 100vw" class="amp-wp-enforced-sizes"></amp-img></p>', | ||
), | ||
|
||
'image_with_decimal_width' => array( | ||
'<p><img src="http://placehold.it/300x300" width="299.5" height="300" /></p>', | ||
'<p><amp-img src="http://placehold.it/300x300" width="299.5" height="300" sizes="(min-width: 299.5px) 299.5px, 100vw" class="amp-wp-enforced-sizes"></amp-img></p>', | ||
), | ||
|
||
'image_with_self_closing_tag' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!" />', | ||
'<amp-img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'image_with_no_end_tag' => array( | ||
'image_with_no_end_tag' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!">', | ||
'<amp-img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'image_with_end_tag' => array( | ||
'image_with_end_tag' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!"></img>', | ||
'<amp-img src="http://placehold.it/350x150" width="350" height="150" alt="Placeholder!" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'image_with_on_attribute' => array( | ||
'image_with_on_attribute' => array( | ||
'<img src="http://placehold.it/350x150" on="tap:my-lightbox" width="350" height="150" />', | ||
'<amp-img src="http://placehold.it/350x150" on="tap:my-lightbox" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'image_with_blacklisted_attribute' => array( | ||
'image_with_blacklisted_attribute' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" style="border: 1px solid red;" />', | ||
'<amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
@@ -62,17 +92,17 @@ public function get_data() { | |
'<amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'gif_image_conversion' => array( | ||
'gif_image_conversion' => array( | ||
'<img src="http://placehold.it/350x150.gif" width="350" height="150" alt="Placeholder!" />', | ||
'<amp-anim src="http://placehold.it/350x150.gif" width="350" height="150" alt="Placeholder!" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-anim>', | ||
), | ||
|
||
'gif_image_url_with_querystring' => array( | ||
'gif_image_url_with_querystring' => array( | ||
'<img src="http://placehold.it/350x150.gif?foo=bar" width="350" height="150" alt="Placeholder!" />', | ||
'<amp-anim src="http://placehold.it/350x150.gif?foo=bar" width="350" height="150" alt="Placeholder!" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-anim>', | ||
), | ||
|
||
'multiple_same_image' => array( | ||
'multiple_same_image' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" /> | ||
<img src="http://placehold.it/350x150" width="350" height="150" /> | ||
<img src="http://placehold.it/350x150" width="350" height="150" /> | ||
|
@@ -81,7 +111,7 @@ public function get_data() { | |
'<amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img><amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img><amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img><amp-img src="http://placehold.it/350x150" width="350" height="150" sizes="(min-width: 350px) 350px, 100vw" class="amp-wp-enforced-sizes"></amp-img>', | ||
), | ||
|
||
'multiple_different_images' => array( | ||
'multiple_different_images' => array( | ||
'<img src="http://placehold.it/350x150" width="350" height="150" /> | ||
<img src="http://placehold.it/360x160" width="360" height="160" /> | ||
<img src="http://placehold.it/370x170" width="370" height="170" /> | ||
|
There was a problem hiding this comment.
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.