Prevent video block from causing AMP validation errors in stories

assets/src/components/higher-order/with-amp-story-settings.js

@@ -1,3 +1,8 @@
+/**
+ * External dependencies
+ */
+import { get } from 'lodash';
+
 /**
  * WordPress dependencies
  */
@@ -40,13 +45,24 @@ const applyFallbackStyles = withFallbackStyles( ( node, ownProps ) => {
 const applyWithSelect = withSelect( ( select, props ) => {
 	const { getSelectedBlockClientId, getBlockRootClientId, getBlock } = select( 'core/block-editor' );
 	const { getAnimatedBlocks, isValidAnimationPredecessor } = select( 'amp/story' );
+	const { getMedia } = select( 'core' );
 
 	const currentBlock = getSelectedBlockClientId();
 	const page = getBlockRootClientId( currentBlock );
 
 	const animatedBlocks = getAnimatedBlocks()[ page ] || [];
 	const animationOrderEntry = animatedBlocks.find( ( { id } ) => id === props.clientId );
 
+	const isVideoBlock = 'core/video' === props.name;
+	let videoFeaturedImage;
+
+	// If we have a video set from an attachment but there is no poster, use the featured image of the video if available.
+	if ( isVideoBlock && props.attributes.id && ! props.attributes.poster ) {
+		const media = getMedia( props.attributes.id );
+		const featuredImage = media && get( media, [ '_links', 'wp:featuredmedia', 0, 'href' ], null );
+		videoFeaturedImage = featuredImage && getMedia( Number( featuredImage.split( '/' ).pop() ) );
+	}
+
 	return {
 		parentBlock: getBlock( getBlockRootClientId( props.clientId ) ),
 		// Use parent's clientId instead of anchor attribute.
@@ -70,6 +86,7 @@ const applyWithSelect = withSelect( ( select, props ) => {
 					};
 				} );
 		},
+		videoFeaturedImage,
 	};
 } );
 
@@ -141,6 +158,7 @@ export default createHigherOrderComponent(
 				onAnimationDelayChange,
 				getAnimatedBlocks,
 				animationAfter,
+				videoFeaturedImage,
 				startBlockRotation,
 				stopBlockRotation,
 			} = props;
@@ -152,6 +170,7 @@ export default createHigherOrderComponent(
 			}
 
 			const isImageBlock = 'core/image' === name;
+			const isVideoBlock = 'core/video' === name;
 			const isTextBlock = 'amp/amp-story-text' === name;
 			const needsTextSettings = BLOCKS_WITH_TEXT_SETTINGS.includes( name );
 			const isMovableBlock = ALLOWED_MOVABLE_BLOCKS.includes( name );
@@ -170,6 +189,11 @@ export default createHigherOrderComponent(
 				rotationAngle,
 			} = attributes;
 
+			// If we have a video set from an attachment but there is no poster, use the featured image of the video if available.
+			if ( isVideoBlock && videoFeaturedImage ) {
+				setAttributes( { poster: videoFeaturedImage.source_url } );
+			}
+
 			const minTextHeight = 20;
 			const minTextWidth = 30;
 

assets/src/components/higher-order/with-video-poster-image-notice.js

@@ -0,0 +1,57 @@
+
+/**
+ * WordPress dependencies
+ */
+import { __ } from '@wordpress/i18n';
+import { select } from '@wordpress/data';
+import { Fragment } from '@wordpress/element';
+import { Notice } from '@wordpress/components';
+
+/**
+ * Internal dependencies
+ */
+import { createHigherOrderComponent } from '@wordpress/compose';
+
+/**
+ * Higher-order component that is used for adding an error notice for the poster image control for video blocks in AMP stories.
+ *
+ * Note that if a user has selected a poster image and then removed it, the poster attribute will remain but be empty.
+ * Because of this, an AMP validation error will not not always ensue if no poster image is supplied, as the plugin's sanitizer
+ * will supply an empty poster attribute to try to prevent a validation error from happening. The  "amp-story >> amp-video" spec
+ * has a loose definition for amp-video[poster] in that only specifies that it is mandatory, but it leaves its value as being undefined.
+ * So this is why an empty poster attribute actually circumvents a validation error from happening. In the future if the value_url type
+ * is specified, then the sanitizer will not be able to supply an empty value to circumvent the AMP validation error.
+ *
+ * @link https://github.com/ampproject/amphtml/blob/b814e5d74cadf554c5caa1233d71e8e840788ff5/extensions/amp-video/validator-amp-video.protoascii#L147-L150
+ *
+ * @return {Function} Higher-order component.
+ */
+export default createHigherOrderComponent(
+	( MediaUpload ) => {
+		return ( props ) => {
+			const isAmpStory = 'amp_story' === select( 'core/editor' ).getCurrentPostType();
+			const selectedBlock = select( 'core/block-editor' ).getSelectedBlock();
+
+			// The `! selectedBlock.attributes.src` check ensures that the notice is not added to the block placeholder.
+			// Otherwise, if a video has been selected, then it is presumed that the MediaUpload here is for the poster image.
+			// There is apparently no way to know for sure that this is for the poster, other than by checking if
+			// props.title === "Select Poster Image", but this would be quite brittle.
+			if ( ! isAmpStory || ! selectedBlock || 'core/video' !== selectedBlock.name || ! selectedBlock.attributes.src || selectedBlock.attributes.poster ) {
+				return <MediaUpload { ...props } />;
+			}
+
+			return (
+				<Fragment>
+					<Notice
+						status="error"
+						isDismissible={ false }
+					>
+						{ __( 'A poster is required for videos in stories.', 'amp' ) }
+					</Notice>
+					<MediaUpload { ...props } />
+				</Fragment>
+			);
+		};
+	},
+	'withVideoPosterImageNotice'
+);

assets/src/components/index.js

@@ -32,3 +32,4 @@ export { default as withCroppedFeaturedImage } from './with-cropped-featured-ima
 export { default as withActivePageState } from './with-active-page-state';
 export { default as withStoryBlockDropZone } from './with-story-block-drop-zone';
 export { default as withCallToActionValidation } from './higher-order/with-call-to-action-validation';
+export { default as withVideoPosterImageNotice } from './higher-order/with-video-poster-image-notice';

assets/src/stories-editor/helpers/index.js

@@ -174,6 +174,7 @@ export const addAMPAttributes = ( settings, name ) => {
 	}
 
 	const isImageBlock = 'core/image' === name;
+	const isVideoBlock = 'core/video' === name;
 	const isMovableBlock = ALLOWED_MOVABLE_BLOCKS.includes( name );
 
 	const addedAttributes = {
@@ -256,6 +257,28 @@ export const addAMPAttributes = ( settings, name ) => {
 		};
 	}
 
+	if ( isVideoBlock ) {
+		// Required defaults for AMP validity.
+		addedAttributes.autoplay = {
+			...settings.attributes.autoplay,
+			default: true,
+		};
+		addedAttributes.playsInline = {
+			...settings.attributes.playsInline,
+			default: false,
+		};
+
+		// Optional defaults.
+		addedAttributes.loop = {
+			...settings.attributes.loop,
+			default: true,
+		};
+		addedAttributes.controls = {
+			...settings.attributes.controls,
+			default: false,
+		};
+	}
+
 	// Define selector according to mappings.
 	if ( BLOCK_TAG_MAPPING[ name ] ) {
 		addedAttributes.ampAnimationType = {

assets/src/stories-editor/index.js

@@ -29,6 +29,7 @@ import {
 	withActivePageState,
 	withStoryBlockDropZone,
 	withCallToActionValidation,
+	withVideoPosterImageNotice,
 } from '../components';
 import {
 	maybeEnqueueFontStyle,
@@ -277,6 +278,7 @@ addFilter( 'editor.PostFeaturedImage', 'ampStoryEditorBlocks/addFeaturedImageNot
 addFilter( 'editor.BlockListBlock', 'ampStoryEditorBlocks/withActivePageState', withActivePageState );
 addFilter( 'editor.BlockListBlock', 'ampStoryEditorBlocks/addWrapperProps', withWrapperProps );
 addFilter( 'editor.MediaUpload', 'ampStoryEditorBlocks/addCroppedFeaturedImage', ( InitialMediaUpload ) => withCroppedFeaturedImage( InitialMediaUpload, getMinimumStoryPosterDimensions() ) );
+addFilter( 'editor.MediaUpload', 'ampStoryEditorBlocks/addPosterImageNotice', withVideoPosterImageNotice );
 addFilter( 'blocks.getSaveContent.extraProps', 'ampStoryEditorBlocks/addExtraAttributes', addAMPExtraProps );
 addFilter( 'blocks.getSaveElement', 'ampStoryEditorBlocks/wrapBlocksInGridLayer', wrapBlocksInGridLayer );
 addFilter( 'editor.BlockDropZone', 'ampStoryEditorBlocks/withStoryBlockDropZone', withStoryBlockDropZone );

bin/amphtml-update.py

@@ -393,14 +393,17 @@ def GetTagSpec(tag_spec, attr_lists):
 	tag_dict = GetTagRules(tag_spec)
 	if tag_dict is None:
 		return None
-	attr_dict = GetAttrs(tag_spec.attrs)
+	attr_dict = {}
 
-	# Now add attributes from any attribute lists to this tag.
+	# First add attributes from any attribute lists to this tag.
 	for (tag_field_desc, tag_field_val) in tag_spec.ListFields():
 		if 'attr_lists' == tag_field_desc.name:
 			for attr_list in tag_field_val:
 				attr_dict.update(attr_lists[UnicodeEscape(attr_list)])
 
+	# Then merge the spec-specific attributes on top to override any list definitions.
+	attr_dict.update(GetAttrs(tag_spec.attrs))
+
 	logging.info('... done')
 	tag_spec_dict = {'tag_spec':tag_dict, 'attr_spec_list':attr_dict}
 	if tag_spec.HasField('cdata'):

includes/sanitizers/class-amp-allowed-tags-generated.php

@@ -5306,6 +5306,7 @@ class AMP_Allowed_Tags_Generated {
 					'artwork' => array(),
 					'attribution' => array(),
 					'autoplay' => array(
+						'mandatory' => true,
 						'value' => array(
 							'',
 						),
@@ -16112,7 +16113,6 @@ class AMP_Allowed_Tags_Generated {
 				'',
 			),
 		),
-		'grid-area' => array(),
 		'hidden' => array(
 			'value' => array(
 				'',