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

MP4 Video shortcode creates invalid AMP page #976

Closed
erralb opened this Issue Feb 26, 2018 · 3 comments

Comments

5 participants
@erralb
Copy link

commented Feb 26, 2018

As a user, I expect video shortcodes to work inside my WordPress posts when they are rendered as AMP.

  • AC1: The sanitizer should convert the http request to https as described below.

Hi,

You can see this problem directly within the AMP Google search console :
https://search.google.com/test/amp?skip_amp_follow=true&id=RH1_TwbRhxkrC2y1Hgap-Q

The post content generating the error is :

[video width="740" height="428" mp4="http://ads-up.fr/app/uploads/2016/06/BingAdsEditor-Mac-Multicomptes.mp4" loop="true" autoplay="true" preload="auto"][/video]

The AMP generated code is :

[video width="740" height="428" mp4="http://ads-up.fr/app/uploads/2016/06/BingAdsEditor-Mac-Multicomptes.mp4" loop="true" autoplay="true" preload="auto"][/video]

@westonruter

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

Yes, this is a problem I've come across as well. The quick fix on your end is to just modify the URL from http: to https:.

However, this should be getting done automatically, such in AMP_Video_Sanitizer by:

https://github.com/Automattic/amp-wp/blob/ac753c4aeb60386e5e1364747aeb1952b853d86d/includes/sanitizers/class-amp-base-sanitizer.php#L289-L313

I think the problem is just that the maybe_enforce_https_src method is not applying to the src attribute on source elements, which is what the video shortcode is (correctly) outputting for you:

<amp-video class="wp-video-shortcode amp-wp-enforced-sizes" width="740" height="428" loop="" autoplay="" controls="" sizes="(min-width: 600px) 600px, 100vw">
    <source type="video/mp4" src="http://ads-up.fr/app/uploads/2016/06/BingAdsEditor-Mac-Multicomptes.mp4?_=1"/>
</amp-video>

See also #969.

@westonruter westonruter added this to the v1.0 milestone May 2, 2018

@westonruter westonruter added this to Definition in v1.0 May 31, 2018

@postphotos postphotos added the release label Jul 9, 2018

@postphotos

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Hi @ierpe, We plan to work on this as part of our v1.0 release. I've added a user story and acceptance criteria for one of the XWP engineers to work on this. Thanks for submitting this issue!

@postphotos postphotos moved this from Definition to To do in v1.0 Jul 18, 2018

@hellofromtonya hellofromtonya self-assigned this Jul 18, 2018

hellofromtonya added a commit that referenced this issue Jul 19, 2018

Force HTTPS on video and source src attribute.
This commit forces HTTPS on the `src` attribute.  For each child node, it sets the new filtered `src` attribute.

Fixes #976.

@hellofromtonya hellofromtonya moved this from To do to In progress in v1.0 Jul 19, 2018

@hellofromtonya hellofromtonya moved this from In progress to Ready for review in v1.0 Jul 19, 2018

@hellofromtonya hellofromtonya moved this from Ready for review to Ready for QA in v1.0 Jul 23, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2018

Moving To "Ready For Merging"

If it's alright, I'm moving this to "Ready For Merging." If you think this could use functional testing, feel free to move it back.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Aug 3, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.