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

Definitively fix issues with images in AMP not visually matching non-AMP version #2036

Merged
merged 12 commits into from Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions assets/css/amp-default.css
@@ -1,6 +1,11 @@
.amp-wp-unknown-size [src] {
/** Worst case scenario when we can't figure out dimensions for an image. **/
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/

/*
* Prevent cases of amp-img converted from img to appear with stretching by using object-fit to scale.
* See <https://github.com/ampproject/amphtml/issues/21371#issuecomment-475443219>.
* Also use object-fit:contain in worst case scenario when we can't figure out dimensions for an image.
*/
amp-img.amp-wp-enforced-sizes[layout=intrinsic] > img,
.amp-wp-unknown-size > img {
object-fit: contain;
}

Expand Down
4 changes: 3 additions & 1 deletion includes/amp-helper-functions.php
Expand Up @@ -783,7 +783,9 @@ function amp_get_content_sanitizers( $post = null ) {
'template' => get_template(),
'stylesheet' => get_stylesheet(),
),
'AMP_Img_Sanitizer' => array(),
'AMP_Img_Sanitizer' => array(
'align_wide_support' => current_theme_supports( 'align-wide' ),
),
'AMP_Form_Sanitizer' => array(),
'AMP_Comments_Sanitizer' => array(
'comments_live_list' => ! empty( $theme_support_args['comments_live_list'] ),
Expand Down
2 changes: 1 addition & 1 deletion includes/class-amp-theme-support.php
Expand Up @@ -913,7 +913,7 @@ function() {
);

add_action( 'wp_head', 'amp_add_generator_metadata', 20 );
add_action( 'wp_enqueue_scripts', array( __CLASS__, 'enqueue_assets' ) );
add_action( 'wp_enqueue_scripts', array( __CLASS__, 'enqueue_assets' ), 0 ); // Enqueue before theme's styles.
add_action( 'wp_enqueue_scripts', array( __CLASS__, 'dequeue_customize_preview_scripts' ), 1000 );
add_filter( 'customize_partial_render', array( __CLASS__, 'filter_customize_partial_render' ) );

Expand Down
100 changes: 17 additions & 83 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
Expand Up @@ -53,20 +53,18 @@ class AMP_Core_Theme_Sanitizer extends AMP_Base_Sanitizer {
protected static $theme_features = array(
// Twenty Nineteen.
'twentynineteen' => array(
'dequeue_scripts' => array(
'dequeue_scripts' => array(
'twentynineteen-skip-link-focus-fix', // This is part of AMP. See <https://github.com/ampproject/amphtml/issues/18671>.
'twentynineteen-priority-menu',
'twentynineteen-touch-navigation', // @todo There could be an AMP implementation of this, similar to what is implemented on ampproject.org.
),
'remove_actions' => array(
'remove_actions' => array(
'wp_print_footer_scripts' => array(
'twentynineteen_skip_link_focus_fix', // See <https://github.com/WordPress/twentynineteen/pull/47>.
),
),
'add_twentynineteen_masthead_styles' => array(),
'add_twentynineteen_image_styles' => array(),
'remove_twentynineteen_thumbnail_image_sizes' => array(),

'add_twentynineteen_masthead_styles' => array(),
'adjust_twentynineteen_images' => array(),
),

// Twenty Seventeen.
Expand Down Expand Up @@ -402,30 +400,6 @@ function ( $content ) {
);
}

/**
* Remove the sizes attribute from thumbnail images in Twenty Nineteen.
*
* The AMP runtime sets an inline style on an <amp-img> based on the sizes attribute if it's present.
* For example, <amp-img style="width:calc(50vw)">.
* Removing the 'sizes' attribute isn't ideal, but it looks like it's not possible to override that inline style.
*
* @todo: remove when this is resolved: https://github.com/ampproject/amphtml/issues/17053
* @since 1.0
*/
public static function remove_twentynineteen_thumbnail_image_sizes() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great to be able to remove these workarounds!

add_filter(
'wp_get_attachment_image_attributes',
function( $attr ) {
if ( isset( $attr['class'] ) && false !== strpos( $attr['class'], 'attachment-post-thumbnail' ) ) {
unset( $attr['sizes'] );
}

return $attr;
},
11
);
}

/**
* Add filter to adjust the attachment image attributes to ensure attachment pages have a consistent <amp-img> rendering.
*
Expand All @@ -435,38 +409,6 @@ function( $attr ) {
* @link https://github.com/WordPress/wordpress-develop/blob/ddc8f803c6e99118998191fd2ea24124feb53659/src/wp-content/themes/twentyseventeen/functions.php#L545:L554
*/
public static function add_twentyseventeen_attachment_image_attributes() {
add_filter(
'wp_get_attachment_image_attributes',
function ( $attr, $attachment, $size ) {
if (
isset( $attr['class'] )
&&
(
'custom-logo' === $attr['class']
||
false !== strpos( $attr['class'], 'attachment-twentyseventeen-featured-image' )
)
) {
/*
* The AMP runtime sets an inline style on an <amp-img> based on the sizes attribute if it's present.
* For example, <amp-img style="width:100%">.
* Removing the 'sizes' attribute is only a workaround, as it looks like it's not possible to override that inline style.
*
* @todo: remove when this is resolved: https://github.com/ampproject/amphtml/issues/17053
*/
unset( $attr['sizes'] );
} elseif ( is_attachment() ) {
$sizes = wp_get_attachment_image_sizes( $attachment->ID, $size );
if ( false !== $sizes ) {
$attr['sizes'] = $sizes;
}
}
return $attr;
},
11,
3
);

/*
* The max-height of the `.custom-logo-link img` is defined as being 80px, unless
* there is header media in which case it is 200px. Issues related to vertically-squashed
Expand Down Expand Up @@ -1082,29 +1024,21 @@ function() use ( $args ) {
}

/**
* Output image styles for twentynineteen.
*
* When <img> tags have an 'aligncenter' class, AMP_Img_Sanitizer::handle_centering() wraps theme in <figure class="aligncenter">.
* This ensures that the image inside it is centered.
* Adjust images in twentynineteen.
*
* @since 1.0
* @since 1.1
*/
public static function add_twentynineteen_image_styles() {
add_action(
'wp_enqueue_scripts',
function() {
ob_start();
?>
<style>
figure.aligncenter {
text-align: center
}
</style>
<?php
$styles = str_replace( array( '<style>', '</style>' ), '', ob_get_clean() );
wp_add_inline_style( get_template() . '-style', $styles );
},
11
public static function adjust_twentynineteen_images() {

// Make sure the featured image gets responsive layout.
add_filter(
'wp_get_attachment_image_attributes',
function( $attributes ) {
if ( preg_match( '/(^|\s)(attachment-post-thumbnail)(\s|$)/', $attributes['class'] ) ) {
$attributes['data-amp-layout'] = 'responsive';
}
return $attributes;
}
);
}
}
102 changes: 25 additions & 77 deletions includes/sanitizers/class-amp-img-sanitizer.php
Expand Up @@ -107,6 +107,21 @@ public function sanitize() {

$this->determine_dimensions( $need_dimensions );
$this->adjust_and_replace_nodes_in_array_map( $need_dimensions );

/*
* Opt-in to amp-img-auto-sizes experiment.
* This is needed because the sizes attribute is removed from all img elements converted to amp-img
* in order to prevent the undesirable setting of the width. This $meta tag can be removed once the
* experiment ends (and the feature has been fully launched).
* See <https://github.com/ampproject/amphtml/issues/21371> and <https://github.com/ampproject/amp-wp/pull/2036>.
*/
$head = $this->dom->getElementsByTagName( 'head' )->item( 0 );
if ( $head ) {
$meta = $this->dom->createElement( 'meta' );
$meta->setAttribute( 'name', 'amp-experiments-opt-in' );
$meta->setAttribute( 'content', 'amp-img-auto-sizes' );
$head->insertBefore( $meta, $head->firstChild );
}
}

/**
Expand Down Expand Up @@ -249,9 +264,17 @@ private function adjust_and_replace_node( $node ) {

$this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' );
if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['height'] ) && ! empty( $new_attributes['width'] ) ) {
$new_attributes['layout'] = 'intrinsic';
// Use responsive images when a theme supports wide and full-bleed images.
if ( ! empty( $this->args['align_wide_support'] ) && $node->parentNode && 'figure' === $node->parentNode->nodeName && preg_match( '/(^|\s)(alignwide|alignfull)(\s|$)/', $node->parentNode->getAttribute( 'class' ) ) ) {
$new_attributes['layout'] = 'responsive';
} else {
$new_attributes['layout'] = 'intrinsic';
}
}

// Remove sizes attribute since it causes headaches in AMP and because AMP will generate it for us. See <https://github.com/ampproject/amphtml/issues/21371>.
unset( $new_attributes['sizes'] );

if ( $this->is_gif_url( $new_attributes['src'] ) ) {
$this->did_convert_elements = true;

Expand All @@ -261,15 +284,12 @@ private function adjust_and_replace_node( $node ) {
}

$img_node = AMP_DOM_Utils::create_node( $this->dom, $new_tag, $new_attributes );
$new_node = $this->handle_centering( $img_node );
$node->parentNode->replaceChild( $new_node, $node );
$node->parentNode->replaceChild( $img_node, $node );

// Preserve original node in noscript for no-JS environments.
$noscript = $this->dom->createElement( 'noscript' );
$noscript->appendChild( $node );
$img_node->appendChild( $noscript );

$this->add_auto_width_to_figure( $new_node );
}

/**
Expand Down Expand Up @@ -313,76 +333,4 @@ private function is_gif_url( $url ) {
$path = wp_parse_url( $url, PHP_URL_PATH );
return substr( $path, -strlen( $ext ) ) === $ext;
}

/**
* Handles an issue with the aligncenter class.
*
* If the <amp-img> has the class aligncenter, this strips the class and wraps it in a <figure> to center the image.
* There was an issue where the aligncenter class overrode the "display: inline-block" rule of AMP's layout="intrinsic" attribute.
* So this strips that class, and instead wraps the image in a <figure> to center it.
*
* @since 0.7
* @see https://github.com/ampproject/amp-wp/issues/1104
*
* @param DOMElement $node The <amp-img> node.
* @return DOMElement $node The <amp-img> node, possibly wrapped in a <figure>.
*/
public function handle_centering( $node ) {
$align_class = 'aligncenter';
$classes = $node->getAttribute( 'class' );
$width = $node->getAttribute( 'width' );

// If this doesn't have a width attribute, centering it in the <figure> wrapper won't work.
if ( empty( $width ) || ! in_array( $align_class, preg_split( '/\s+/', trim( $classes ) ), true ) ) {
return $node;
}

// Strip the class, and wrap the <amp-img> in a <figure>.
$classes = trim( str_replace( $align_class, '', $classes ) );
$node->setAttribute( 'class', $classes );
$figure = AMP_DOM_Utils::create_node(
$this->dom,
'figure',
array(
'class' => $align_class,
'style' => "max-width: {$width}px;",
)
);
$figure->appendChild( $node );

return $figure;
}

/**
* Add an inline style to set the `<figure>` element's width to `auto` instead of `fit-content`.
*
* @since 1.0
* @see https://github.com/ampproject/amp-wp/issues/1086
*
* @param DOMElement $node The DOMNode to adjust and replace.
*/
protected function add_auto_width_to_figure( $node ) {
$figure = $node->parentNode;
if ( ! ( $figure instanceof DOMElement ) || 'figure' !== $figure->tagName ) {
return;
}

$class = $figure->getAttribute( 'class' );
// Target only the <figure> with a 'wp-block-image' class attribute.
if ( false === strpos( $class, 'wp-block-image' ) ) {
return;
}

// Target only <figure> without a 'is-resized' class attribute.
if ( false !== strpos( $class, 'is-resized' ) ) {
return;
}

$new_style = 'width: auto;';
if ( $figure->hasAttribute( 'style' ) ) {
$figure->setAttribute( 'style', $new_style . $figure->getAttribute( 'style' ) );
} else {
$figure->setAttribute( 'style', $new_style );
}
}
}
12 changes: 7 additions & 5 deletions templates/style.php
Expand Up @@ -34,6 +34,7 @@

.aligncenter {
display: block;
text-align: center;
margin-left: auto;
margin-right: auto;
}
Expand All @@ -44,11 +45,7 @@
margin: 0 auto;
}

.amp-wp-unknown-size img {
/** Worst case scenario when we can't figure out dimensions for an image. **/
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
}
<?php echo file_get_contents( AMP__DIR__ . '/assets/css/amp-default.css' ); // phpcs:ignore WordPress.WP.AlternativeFunctions ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to echo amp-default.css here.


/* Template Styles */

Expand Down Expand Up @@ -284,6 +281,11 @@

/* AMP Media */

.alignwide,
.alignfull {
clear: both;
}

amp-carousel {
background: <?php echo sanitize_hex_color( $border_color ); ?>;
margin: 0 -16px 1.5em;
Expand Down