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

Force videopress to use html5 player for AMP #1125

Merged
merged 9 commits into from May 10, 2018

Conversation

Projects
None yet
2 participants
@yurynix
Contributor

yurynix commented May 7, 2018

Videopress embed uses an iframe and a script tag,
script tag is not allowed in AMP, so it's stripped and a warning is generated.

screen shot 2018-05-07 at 10 19 17

This PR tries to force html5 player, so the player would be actually a <video /> tag
that would be converted to <amp-video />.

@yurynix

This comment has been minimized.

Show comment
Hide comment
@yurynix

yurynix May 7, 2018

Contributor

Despite the code in this PR, I still see the validation warning, I'm not sure why...

Contributor

yurynix commented May 7, 2018

Despite the code in this PR, I still see the validation warning, I'm not sure why...

@yurynix

This comment has been minimized.

Show comment
Hide comment
@yurynix

yurynix May 7, 2018

Contributor

Ok, got it, it's because of this validation, so shortcodes are processed, but there is no way to hook to it for allowing modification for AMP.

Maybe do_action( 'pre_amp_render_post', $post_id ); ?

Contributor

yurynix commented May 7, 2018

Ok, got it, it's because of this validation, so shortcodes are processed, but there is no way to hook to it for allowing modification for AMP.

Maybe do_action( 'pre_amp_render_post', $post_id ); ?

@yurynix yurynix changed the title from [WIP] Force videopress to use html5 player for AMP to Force videopress to use html5 player for AMP May 7, 2018

@yurynix yurynix requested a review from mjangda May 7, 2018

@westonruter

Note: Eventually the Jetpack-specific code will be merged into Jetpack code itself per #1021.

Show outdated Hide outdated jetpack-helper.php Outdated
Show outdated Hide outdated includes/utils/class-amp-validation-utils.php Outdated
@westonruter

westonruter requested changes May 7, 2018 edited

To ensure that this change applies when amp theme support is present, there should be also something like this:

add_action( 'template_redirect', function() {
    if ( is_amp_endpoint() ) {
        amp_jetpack_mods();
    }
}, 9 );

This could be used instead of the pre_amp_render_post action.

@yurynix

This comment has been minimized.

Show comment
Hide comment
@yurynix

yurynix May 7, 2018

Contributor

@westonruter Thank you for the review, I'll address your comments tomorrow unless someone beats me to it.

I'm not really clear what you meant by your comment here though:
#1125 (review)

Why hooking to template_redirect action is better than pre_amp_render_post?
Sorry if that's obvious, It's my first time around that repo.

Contributor

yurynix commented May 7, 2018

@westonruter Thank you for the review, I'll address your comments tomorrow unless someone beats me to it.

I'm not really clear what you meant by your comment here though:
#1125 (review)

Why hooking to template_redirect action is better than pre_amp_render_post?
Sorry if that's obvious, It's my first time around that repo.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 7, 2018

Collaborator

@yurynix In v0.7 we've added amp theme support where you can have the entire theme be served as AMP, not just the post templates. For example, you could have your blog index template be served as AMP now (see example site: https://ampdemo.xwp.io/). In that case, there is no $post to be passed as the arg for the pre_amp_render_post action. But you can see from the source that the pre_amp_render_post action actually runs at template_redirect (priority 10) anyway:

amp-wp/amp.php

Lines 326 to 383 in 7439d2a

add_action( 'template_redirect', 'amp_render' );
}
/**
* Render AMP for queried post.
*
* @since 0.1
*/
function amp_render() {
// Note that queried object is used instead of the ID so that the_preview for the queried post can apply.
$post = get_queried_object();
if ( $post instanceof WP_Post ) {
amp_render_post( $post );
exit;
}
}
/**
* Render AMP post template.
*
* @since 0.5
* @param WP_Post|int $post Post.
* @global WP_Query $wp_query
*/
function amp_render_post( $post ) {
global $wp_query;
if ( ! ( $post instanceof WP_Post ) ) {
$post = get_post( $post );
if ( ! $post ) {
return;
}
}
$post_id = $post->ID;
/*
* If amp_render_post is called directly outside of the standard endpoint, is_amp_endpoint() will return false,
* which is not ideal for any code that expects to run in an AMP context.
* Let's force the value to be true while we render AMP.
*/
$was_set = isset( $wp_query->query_vars[ amp_get_slug() ] );
if ( ! $was_set ) {
$wp_query->query_vars[ amp_get_slug() ] = true;
}
// Prevent New Relic from causing invalid AMP responses due the NREUM script it injects after the meta charset.
if ( extension_loaded( 'newrelic' ) ) {
newrelic_disable_autorum();
}
/**
* Fires before rendering a post in AMP.
*
* @since 0.2
*
* @param int $post_id Post ID.
*/
do_action( 'pre_amp_render_post', $post_id );

So by doing amp_jetpack_mods() at template_redirect priority 9, then this ensures that the mods are present whether WordPress is serving the old AMP post templates or whether amp theme support is present.

Collaborator

westonruter commented May 7, 2018

@yurynix In v0.7 we've added amp theme support where you can have the entire theme be served as AMP, not just the post templates. For example, you could have your blog index template be served as AMP now (see example site: https://ampdemo.xwp.io/). In that case, there is no $post to be passed as the arg for the pre_amp_render_post action. But you can see from the source that the pre_amp_render_post action actually runs at template_redirect (priority 10) anyway:

amp-wp/amp.php

Lines 326 to 383 in 7439d2a

add_action( 'template_redirect', 'amp_render' );
}
/**
* Render AMP for queried post.
*
* @since 0.1
*/
function amp_render() {
// Note that queried object is used instead of the ID so that the_preview for the queried post can apply.
$post = get_queried_object();
if ( $post instanceof WP_Post ) {
amp_render_post( $post );
exit;
}
}
/**
* Render AMP post template.
*
* @since 0.5
* @param WP_Post|int $post Post.
* @global WP_Query $wp_query
*/
function amp_render_post( $post ) {
global $wp_query;
if ( ! ( $post instanceof WP_Post ) ) {
$post = get_post( $post );
if ( ! $post ) {
return;
}
}
$post_id = $post->ID;
/*
* If amp_render_post is called directly outside of the standard endpoint, is_amp_endpoint() will return false,
* which is not ideal for any code that expects to run in an AMP context.
* Let's force the value to be true while we render AMP.
*/
$was_set = isset( $wp_query->query_vars[ amp_get_slug() ] );
if ( ! $was_set ) {
$wp_query->query_vars[ amp_get_slug() ] = true;
}
// Prevent New Relic from causing invalid AMP responses due the NREUM script it injects after the meta charset.
if ( extension_loaded( 'newrelic' ) ) {
newrelic_disable_autorum();
}
/**
* Fires before rendering a post in AMP.
*
* @since 0.2
*
* @param int $post_id Post ID.
*/
do_action( 'pre_amp_render_post', $post_id );

So by doing amp_jetpack_mods() at template_redirect priority 9, then this ensures that the mods are present whether WordPress is serving the old AMP post templates or whether amp theme support is present.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 7, 2018

Collaborator

@yurynix also, you may want to rebase your commits onto the 0.7 branch so that the changes could be included in 0.7.1.

Collaborator

westonruter commented May 7, 2018

@yurynix also, you may want to rebase your commits onto the 0.7 branch so that the changes could be included in 0.7.1.

yurynix and others added some commits May 7, 2018

Call amp_jetpack_mods() at template_redirect for sake of amp theme su…
…pport

* Remove obsolete filter addition for videopress_shortcode_options when validate query var is present.
* Add missing phpdoc.
* Clean up phpcs issues.

@westonruter westonruter changed the base branch from develop to 0.7 May 8, 2018

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 8, 2018

Collaborator

@yurynix I've rebased onto 0.7 so please reset your branch to the remote. I've also made the changes I described. Please test to confirm it works as expected.

Collaborator

westonruter commented May 8, 2018

@yurynix I've rebased onto 0.7 so please reset your branch to the remote. I've also made the changes I described. Please test to confirm it works as expected.

@westonruter westonruter added this to the v0.7.1 milestone May 8, 2018

@yurynix

This comment has been minimized.

Show comment
Hide comment
@yurynix

yurynix May 8, 2018

Contributor

Thank you for all the explanation and the work here @westonruter .
I've taken it to a spin now, and while it indeed generates the html5 player on AMP page,
the issue of the editor warning is still present.

This is caused because of the code that validates the output don't call redirect_template action, it only registers content embed handlers.

Which may be this workaround should be, a content embed handler, so I've pushed a commit to do just that, ideally, it should live in the Jetpack plugin with the rest of the workarounds.

Contributor

yurynix commented May 8, 2018

Thank you for all the explanation and the work here @westonruter .
I've taken it to a spin now, and while it indeed generates the html5 player on AMP page,
the issue of the editor warning is still present.

This is caused because of the code that validates the output don't call redirect_template action, it only registers content embed handlers.

Which may be this workaround should be, a content embed handler, so I've pushed a commit to do just that, ideally, it should live in the Jetpack plugin with the rest of the workarounds.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 9, 2018

Collaborator

@yurynix I dropped your two commits as requested, and I merged 0.7 into this branch. Could you confirm it works as expected now?

Collaborator

westonruter commented May 9, 2018

@yurynix I dropped your two commits as requested, and I merged 0.7 into this branch. Could you confirm it works as expected now?

@yurynix

This comment has been minimized.

Show comment
Hide comment
@yurynix

yurynix May 10, 2018

Contributor

I've tested it and it works great 👍
Thanks.

Contributor

yurynix commented May 10, 2018

I've tested it and it works great 👍
Thanks.

@westonruter westonruter merged commit 4dd1023 into 0.7 May 10, 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 add/force-videopress-use-html5 branch May 10, 2018

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