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

Fix conversion of video to amp-video #1477

Merged
merged 3 commits into from Sep 28, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Member

westonruter commented Sep 28, 2018

This fixes a regression in #1474.

Given a video widget generated from the non-AMP version as follows:

<section id="media_video-2" class="widget widget_media_video">
	<h2 class="widget-title">The Video</h2>
	<div style="width:100%;" class="wp-video">
		<!--[if lt IE 9]><script>document.createElement( 'video' );</script><![endif]-->
		<video class="wp-video-shortcode" id="video-7-1" preload="metadata" controls="controls">
			<source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1"/>
			<source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1"/>
			<track srclang="en" label="English" kind="subtitles" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/test-en.vtt" />
			<a href="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4">https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4</a>
		</video>
	</div>
</section>

I was seeing this in the error log in AMP:

PHP Notice: Undefined index: src in /srv/www/wordpress-develop/public_html/src/wp-content/plugins/amp/includes/sanitizers/class-amp-video-sanitizer.php on line 136
PHP Notice: Undefined index: src in /srv/www/wordpress-develop/public_html/src/wp-content/plugins/amp/includes/sanitizers/class-amp-video-sanitizer.php on line 137

If I fixed this problem (dd21353) then I get another regression in that the video gets the default height of 400 (with the aspect ratio no longer being landscape):

<amp-video class="wp-video-shortcode" controls="" height="400" layout="fixed-height"><source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1">
	<source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1">
</amp-video>

So then I went deeper with additional fixes in cf6d2a2 and now I get what is expected:

<amp-video class="wp-video-shortcode" id="video-7-1" preload="metadata" controls="" width="1152" height="864" layout="responsive">
    <source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1">
    <source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2018/09/Clip-May-6-2018-at-802-PM.mp4?_=1">
</amp-video>

This PR also ensures that track children of video are not removed, and that the first non-source/track element child gets a fallback attribute added.

westonruter added some commits Sep 27, 2018

Preserve extra video attributes and non-source children in amp-video …
…conversion

There is no need to remove unrecognized attributes because these will be handled by the whitelist sanitizer.

Also preserve all original child nodes of video so that track elements will be preserved.

@westonruter westonruter added this to the v1.0 milestone Sep 28, 2018

@westonruter westonruter requested a review from kienstra Sep 28, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 28, 2018

Approved, Thanks For Fixing

HI @westonruter,
Thanks, this looks good.

The videos still appear as expected, and there's no ‘Undefined index: src’ warning, like before.

videos-fix-applied

I should have caught that earlier in #1474. Did you tail the error log from within VVV?

tail -f /srv/www/example-site/log/error.log
}
$sources_count++;
$child_node->setAttribute( 'src', $src );
$new_attributes = $this->filter_video_dimensions( $new_attributes, $src );

This comment has been minimized.

@kienstra

kienstra Sep 28, 2018

Collaborator

Good idea to pass $src to $this->filter_video_dimensions().

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 28, 2018

Yes, I found the issue by tailing the error log.

@westonruter westonruter merged commit c19dad5 into develop Sep 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/undefined-index-video-src branch Sep 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment