Merge pull request #5955 from ampproject/add/object-fit-position-inheritance

Inherit object-fit and object-position styles so more conversions work out of the box

Author: westonruter

assets/css/src/amp-default.css

@@ -1,13 +1,10 @@
-
 /*
- * Prevent cases of amp-img converted from img to appear with stretching by using object-fit to scale.
+ * Prevent cases of amp-img converted from img to appear with stretching by using object-fit to scale. Only do this for
+ * amp-img/amp-anim elements that we converted from <img> via AMP_Img_Sanitizer. This is key for images that get 100%
+ * width such as in the post content so that the contents do not get stretched or cropped.
  * 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.
- * Additionally, in side of \AMP_Img_Sanitizer::determine_dimensions() it could $amp_img->setAttribute( 'object-fit', 'contain' )
- * so that the following rules wouldn't be needed.
  */
-amp-img.amp-wp-enforced-sizes[layout="intrinsic"] > img,
-amp-anim.amp-wp-enforced-sizes[layout="intrinsic"] > img {
+.amp-wp-enforced-sizes {
 	object-fit: contain;
 }
 
@@ -89,3 +86,20 @@ amp-carousel .amp-wp-gallery-caption {
 button[overflow] {
 	bottom: 0;
 }
+
+/*
+ * Ensure relevant properties for "replaced elements" which are set on sanitizer-converted custom elements will be
+ * inherited to the actual replaced element which is the shadow element and also to the noscript fallback element.
+ */
+amp-anim img,
+amp-anim noscript,
+amp-iframe iframe,
+amp-iframe noscript,
+amp-img img,
+amp-img noscript,
+amp-video video,
+amp-video noscript {
+	image-rendering: inherit;
+	object-fit: inherit;
+	object-position: inherit;
+}

includes/embeds/class-amp-core-block-handler.php

@@ -50,7 +50,6 @@ class AMP_Core_Block_Handler extends AMP_Base_Embed_Handler {
 		'core/categories' => 'ampify_categories_block',
 		'core/archives'   => 'ampify_archives_block',
 		'core/video'      => 'ampify_video_block',
-		'core/cover'      => 'ampify_cover_block',
 	];
 
 	/**
@@ -212,75 +211,6 @@ public function ampify_video_block( $block_content, $block ) {
 		return $block_content;
 	}
 
-	/**
-	 * Ampify cover block.
-	 *
-	 * This ensures that the background img/video in a cover block has object-fit=cover and the appropriate object-position
-	 * attribute so that they will be carried over to to the amp-img/amp-video and propagated to the img/video in the
-	 * light shadow DOM.
-	 *
-	 * @see \AMP_Video_Sanitizer::filter_video_dimensions()
-	 *
-	 * @param string $block_content The block content about to be appended.
-	 * @param array  $block         The full block, including name and attributes.
-	 * @return string Filtered block content.
-	 */
-	public function ampify_cover_block( $block_content, $block ) {
-		$is_video_background = (
-			isset( $block['attrs']['backgroundType'] )
-			&&
-			'video' === $block['attrs']['backgroundType']
-		);
-		$is_image_element    = ! (
-			! empty( $block['attrs']['hasParallax'] )
-			||
-			! empty( $block['attrs']['isRepeated'] )
-		);
-
-		// Object fit/position is not relevant when background image has fixed positioning or is repeated.
-		// In other words, it is not relevant when a <video> or a <img> is not going to be used.
-		// See <https://github.com/WordPress/gutenberg/blob/54c9066d4/packages/block-library/src/cover/save.js#L54-L72>.
-		if ( ! ( $is_video_background || $is_image_element ) ) {
-			return $block_content;
-		}
-
-		$pattern = sprintf(
-			'#<%s(?= )[^>]*? class="(?:[^"]*? )?wp-block-cover__%s-background(?: [^"]*?)?"#',
-			$is_video_background ? 'video' : 'img',
-			$is_video_background ? 'video' : 'image'
-		);
-		return preg_replace_callback(
-			$pattern,
-			static function ( $matches ) use ( $block ) {
-				$replacement = $matches[0];
-
-				// The background image/video for the cover block by definition needs object-fit="cover" on the resulting amp-ing/amp-video.
-				$replacement .= ' object-fit="cover"';
-
-				// Add the fill layout to skip needlessly obtaining the dimensions.
-				$replacement .= ' layout="fill"';
-
-				// Add object-position from the block's attributes to add to the img/video to be copied onto the amp-img/amp-video.
-				// The AMP runtime copies object-position attribute onto the underlying img/video for a given amp-img/amp-video.
-				// This is needed since the object-position property directly on an amp-img/amp-video will have no effect since
-				// since it is merely a wrapper for the underlying img/video element which actually supports the CSS property.
-				if ( isset( $block['attrs']['focalPoint']['x'], $block['attrs']['focalPoint']['y'] ) ) {
-					// See logic in Gutenberg for writing focal point to object-position attr:
-					// <https://github.com/WordPress/gutenberg/blob/54c9066/packages/block-library/src/cover/save.js#L71>.
-					$replacement .= sprintf(
-						' object-position="%d%% %d%%"',
-						round( (float) $block['attrs']['focalPoint']['x'] * 100 ),
-						round( (float) $block['attrs']['focalPoint']['y'] * 100 )
-					);
-				}
-
-				return $replacement;
-			},
-			$block_content,
-			1
-		);
-	}
-
 	/**
 	 * Sanitize widgets that are not added via Gutenberg.
 	 *

includes/sanitizers/class-amp-core-theme-sanitizer.php

@@ -158,18 +158,17 @@ protected static function get_theme_features_config( $theme_slug ) {
 			// Twenty Nineteen.
 			case 'twentynineteen':
 				return [
-					'dequeue_scripts'                    => [
+					'dequeue_scripts'              => [
 						'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'                     => [
+					'remove_actions'               => [
 						'wp_print_footer_scripts' => [
 							'twentynineteen_skip_link_focus_fix', // See <https://github.com/WordPress/twentynineteen/pull/47>.
 						],
 					],
-					'add_twentynineteen_masthead_styles' => [],
-					'adjust_twentynineteen_images'       => [],
+					'adjust_twentynineteen_images' => [],
 					'enable_determine_hero_images_transformer' => [],
 				];
 
@@ -938,121 +937,20 @@ static function() use ( $method ) {
 	}
 
 	/**
-	 * Add required styles for featured image header in Twenty Nineteen.
-	 *
-	 * The following is necessary because the styles in the theme apply to the featured img,
-	 * and the CSS parser will then convert the selectors to amp-img. Nevertheless, object-fit
-	 * does not apply on amp-img and it needs to apply on an actual img.
-	 *
-	 * @link https://github.com/WordPress/wordpress-develop/blob/5.0/src/wp-content/themes/twentynineteen/style.css#L2276-L2299
-	 * @since 1.0
-	 */
-	public static function add_twentynineteen_masthead_styles() {
-		add_action(
-			'wp_enqueue_scripts',
-			static function() {
-				ob_start();
-				?>
-				<style>
-				.site-header.featured-image .site-featured-image .post-thumbnail amp-img > img {
-					height: auto;
-					left: 50%;
-					max-width: 1000%;
-					min-height: 100%;
-					min-width: 100vw;
-					position: absolute;
-					top: 50%;
-					transform: translateX(-50%) translateY(-50%);
-					width: auto;
-					z-index: 1;
-					/* When image filters are active, make it grayscale to colorize it blue. */
-				}
-
-				@supports (object-fit: cover) {
-					.site-header.featured-image .site-featured-image .post-thumbnail amp-img > img {
-						height: 100%;
-						left: 0;
-						object-fit: cover;
-						top: 0;
-						transform: none;
-						width: 100%;
-					}
-				}
-				</style>
-				<?php
-				$styles = str_replace( [ '<style>', '</style>' ], '', ob_get_clean() );
-				wp_add_inline_style( get_template() . '-style', $styles );
-			},
-			11
-		);
-	}
-
-	/**
-	 * Add required styles for video and image headers.
+	 * Add required styles for Twenty Seventeen header.
 	 *
-	 * This is currently used exclusively for Twenty Seventeen.
+	 * This is required since JS is not applying the required styles at runtime.
 	 *
 	 * @since 1.0
-	 * @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/style.css#L1687
-	 * @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/style.css#L1743
 	 */
 	public static function add_twentyseventeen_masthead_styles() {
-		/*
-		 * The following is necessary because the styles in the theme apply to img and video,
-		 * and the CSS parser will then convert the selectors to amp-img and amp-video respectively.
-		 * Nevertheless, object-fit does not apply on amp-img and it needs to apply on an actual img.
-		 */
 		add_action(
 			'wp_enqueue_scripts',
 			static function() {
 				$is_front_page_layout = ( is_front_page() && 'posts' !== get_option( 'show_on_front' ) ) || ( is_home() && is_front_page() );
 				ob_start();
 				?>
 				<style>
-				.has-header-image .custom-header-media amp-img > img,
-				.has-header-video .custom-header-media amp-video > video{
-					position: fixed;
-					height: auto;
-					left: 50%;
-					max-width: 1000%;
-					min-height: 100%;
-					min-width: 100%;
-					min-width: 100vw; /* vw prevents 1px gap on left that 100% has */
-					width: auto;
-					top: 50%;
-					padding-bottom: 1px; /* Prevent header from extending beyond the footer */
-					-ms-transform: translateX(-50%) translateY(-50%);
-					-moz-transform: translateX(-50%) translateY(-50%);
-					-webkit-transform: translateX(-50%) translateY(-50%);
-					transform: translateX(-50%) translateY(-50%);
-				}
-				.has-header-image:not(.twentyseventeen-front-page):not(.home) .custom-header-media amp-img > img {
-					bottom: 0;
-					position: absolute;
-					top: auto;
-					-ms-transform: translateX(-50%) translateY(0);
-					-moz-transform: translateX(-50%) translateY(0);
-					-webkit-transform: translateX(-50%) translateY(0);
-					transform: translateX(-50%) translateY(0);
-				}
-				/* For browsers that support object-fit */
-				@supports ( object-fit: cover ) {
-					.has-header-image .custom-header-media amp-img > img,
-					.has-header-video .custom-header-media amp-video > video,
-					.has-header-image:not(.twentyseventeen-front-page):not(.home) .custom-header-media amp-img > img {
-						height: 100%;
-						left: 0;
-						-o-object-fit: cover;
-						object-fit: cover;
-						top: 0;
-						-ms-transform: none;
-						-moz-transform: none;
-						-webkit-transform: none;
-						transform: none;
-						width: 100%;
-					}
-				}
-
 				.navigation-top.site-navigation-fixed {
 					display: none;
 				}
@@ -1555,7 +1453,7 @@ static function() {
 							font-size: 32px;
 							line-height: 46px;
 						}
-						.featured-content .post-thumbnail amp-img > img {
+						.featured-content .post-thumbnail amp-img {
 							object-fit: cover;
 							object-position: top;
 						}

includes/sanitizers/trait-amp-noscript-fallback.php

@@ -6,6 +6,7 @@
  */
 
 use AmpProject\Dom\Document;
+use AmpProject\Attribute;
 
 /**
  * Trait AMP_Noscript_Fallback
@@ -52,6 +53,13 @@ protected function initialize_noscript_allowed_attributes( $tag ) {
 				}
 			}
 		}
+
+		// Remove attributes which are likely to cause styling conflicts, as the noscript fallback should get treated like it has fill layout.
+		unset(
+			$this->noscript_fallback_allowed_attributes[ Attribute::ID ],
+			$this->noscript_fallback_allowed_attributes[ Attribute::CLASS_ ],
+			$this->noscript_fallback_allowed_attributes[ Attribute::STYLE ]
+		);
 	}
 
 	/**
@@ -81,13 +89,12 @@ protected function append_old_node_noscript( DOMElement $new_element, DOMElement
 		$noscript->appendChild( $old_element );
 		$new_element->appendChild( $noscript );
 
-		// Remove all non-allowed attributes preemptively to prevent doubled validation errors.
+		// Remove all non-allowed attributes preemptively to prevent doubled validation errors, only leaving the attributes required.
 		for ( $i = $old_element->attributes->length - 1; $i >= 0; $i-- ) {
 			$attribute = $old_element->attributes->item( $i );
-			if ( isset( $this->noscript_fallback_allowed_attributes[ $attribute->nodeName ] ) ) {
-				continue;
+			if ( ! isset( $this->noscript_fallback_allowed_attributes[ $attribute->nodeName ] ) ) {
+				$old_element->removeAttribute( $attribute->nodeName );
 			}
-			$old_element->removeAttribute( $attribute->nodeName );
 		}
 	}
 }

src/Transformer/DetermineHeroImages.php

@@ -37,21 +37,21 @@ final class DetermineHeroImages implements Transformer {
 	 *
 	 * @var string
 	 */
-	const CUSTOM_HEADER_XPATH_QUERY = ".//*[ @id = 'wp-custom-header' or @id = 'masthead' or @id = 'site-header' or contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-custom-header ' ) ]//*[ ( self::img or self::amp-img ) and not( @data-hero ) and not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) ]";
+	const CUSTOM_HEADER_XPATH_QUERY = ".//*[ @id = 'wp-custom-header' or @id = 'masthead' or @id = 'site-header' or contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-custom-header ' ) ]//amp-img[ not( @data-hero ) and not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) ]";
 
 	/**
 	 * XPath query to find the custom logo.
 	 *
 	 * @var string
 	 */
-	const CUSTOM_LOGO_XPATH_QUERY = ".//a[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo-link ' ) ]//*[ ( self::img or self::amp-img ) and contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ][ not( @data-hero ) ]";
+	const CUSTOM_LOGO_XPATH_QUERY = ".//a[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo-link ' ) ]//amp-img[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ][ not( @data-hero ) ]";
 
 	/**
 	 * XPath query to find the featured image.
 	 *
 	 * @var string
 	 */
-	const FEATURED_IMAGE_XPATH_QUERY = ".//*[ ( self::img or self::amp-img ) and contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-post-image ' ) ][ not( @data-hero ) ]";
+	const FEATURED_IMAGE_XPATH_QUERY = ".//amp-img[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-post-image ' ) ][ not( @data-hero ) ]";
 
 	/**
 	 * XPath query to find the first entry-content.
@@ -65,14 +65,14 @@ final class DetermineHeroImages implements Transformer {
 	 *
 	 * @var string
 	 */
-	const INITIAL_COVER_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::div[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover ' ) ]/*[ ( self::img or self::amp-img ) and contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover__image-background ' ) ][ not( @data-hero ) ]";
+	const INITIAL_COVER_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::div[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover ' ) ]/amp-img[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover__image-background ' ) ][ not( @data-hero ) ]";
 
 	/**
 	 * XPath query to find Image Block at the beginning of post content (including nested inside of another block).
 	 *
 	 * @var string
 	 */
-	const INITIAL_IMAGE_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::figure[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-image ' ) ]/*[ ( self::img or self::amp-img ) ][ not( @data-hero ) ]";
+	const INITIAL_IMAGE_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::figure[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-image ' ) ]/amp-img[ not( @data-hero ) ]";
 
 	/**
 	 * Apply transformations to the provided DOM document.