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

Issue #841: Native AMP audio and video playlists. #954

Merged
merged 20 commits into from Feb 18, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Copy link
Collaborator

commented Feb 11, 2018

This WIP pull request adds a custom shortcode handler for video playlists. It uses <amp-video> and <amp-state>, on Weston's suggestion.

Here's a screencast. It uses much of the same markup as the native shortcode, so it appears styled when adding wp-mediaelement.min.css.

Fixes #841.

kienstra added some commits Feb 11, 2018

Issue #841: Native AMP video playlists.
Create a custom shortcode handler for video playlists.
Use <amp-video> and <amp-state>, on Weston't suggestion.
This still doesn't support audio playlists.
Issue #841: Remove dependence on test files.
Before, test_shortcode() used test files from Core.
But these did not exist in 4.7, and caused a failure.
So create new mock files to test.
Issue #841: Add audio playlist shortcode support.
Use an <amp-carousel>,
As <amp-bind> doesn't work with <amp-audio>.
Abstract common logic into helper methods.
This mainly allows using styling from
wp-mediaelement.css.
But there are 4 rules needed to correct the styling.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2018

Audio Playlist Support, Screencast

This commit adds support for 'audio' playlists. Here's a screencast.

This uses an <amp-carousel>, as <amp-bind> doesn't work with <amp-audio>.

audio-playlist-amp

This looks similar to the Core playlist, after pasting wp-mediaelement.css into the dev tools' 'inspector-stylesheet.' But these rules are also needed:

.wp-playlist .wp-playlist-current-item img {
    margin-right: 0;
}
.wp-playlist .wp-playlist-current-item amp-img {
    float: left;
    margin-right: 10px;
}
.wp-playlist audio {
    display: block;
}

Those rules aren't included in this PR, nor is wp-mediaelement.css.

@kienstra kienstra changed the title [WIP] Issue #841: Native AMP video playlists. Issue #841: Native AMP video playlists. Feb 13, 2018

@westonruter

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@kienstra for the styles I suggest putting it into a CSS file and then enqueueing it. The preprocessor/sanitizer will automatically concatenate it into style[amp-custom]. Actually, whatever styles are included in the default stylesheet you can just enqueue the stylesheet. It'll get concatenated. And when we do the tree shaking of the CSS then it will automatically remove what we don't need.

Issue #841: Enqueue Core playlist styling, and custom styling.
The video and audio playlist need 'wp-mediaelement.'
And the audio playlist needs a simple custom stylesheet.
Use the action 'wp_enqueue_scripts,'
instead of 'wp_playlist_script.'
That enqueues too late.
Also, update the tests.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2018

Thanks, Enqueued Stylesheets

Hi @westonruter,
Thanks for your suggestions. This commit enqueues wp-mediaelement.min.css, and a custom stylesheet for 'audio' playlists.

It looks like with the theme style.css, there aren't enough KB left on the page to print it. But that might be handled when we remove the needless style rules, which you referred to above.

<!--Skipped including https://local.plug/wp-includes/js/mediaelement/wp-mediaelement.min.css?ver=4.9.2 stylesheet since too large.-->
@@ -57,7 +64,8 @@ class AMP_Playlist_Embed_Handler extends AMP_Base_Embed_Handler {
* Registers the playlist shortcode.
*/
public function register_embed() {
add_shortcode( 'playlist', array( $this, 'shortcode' ) );
add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );
add_action( 'wp_enqueue_scripts', array( $this, 'styling' ) );

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 14, 2018

Author Collaborator

This uses wp_enqueue_scripts, instead of wp_playlist_script. That enqueues too late.

@westonruter westonruter referenced this pull request Feb 15, 2018

Merged

Update sanitization reporting #951

2 of 2 tasks complete
foreach ( $this->data['tracks'] as $index => $track ) {
$title = $this->get_title( $track );
if ( 'audio' === $type ) {
$on = 'tap:AMP.setState({' . $container_id . ': {selectedSlide: ' . $i . '}})';

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

We should use wp_json_encode() here:

- $on         = 'tap:AMP.setState({' . $container_id . ': {selectedSlide: ' . $i . '}})';
+ $on         = 'tap:AMP.setState(' . wp_json_encode( array( $container_id => array( 'selectedSlide' => $i ) ) ) . ')';

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit uses wp_json_encode().

}
?>
<div class="wp-playlist-item" [class]="<?php echo isset( $item_class ) ? esc_attr( $item_class ) : ''; ?>">

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

I think it is better to do echo esc_attr( isset( $item_class ) ? $item_class : '' );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

See other such cases in this PR.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit moves the ternary conditionals inside the escaping functions.

<div class="wp-playlist-current-item">
<amp-img src="<?php echo esc_url( $image_url ); ?>" height="<?php echo esc_attr( $image_height ); ?>" width="<?php echo esc_attr( $image_width ); ?>"></amp-img>
<div class="wp-playlist-caption">
<span class="wp-playlist-item-meta wp-playlist-item-title"><?php echo isset( $title ) ? esc_html( $title ) : ''; ?></span>

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

The $title is always going to be set since get_title returns a string. No need for the isset check.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

The commit from above also removes the isset() check for $title.

*/
public function audio_playlist() {
if ( ! isset( $this->data['tracks'] ) ) {
return;

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

The method is declared as returning a string, so this should return an empty string not void.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

This commit returns an empty string inside that conditional.

* This uses similar markup to the native playlist shortcode output.
* So the styles from wp-mediaelement.min.css will apply to it.
*
* @global content_width.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

Should be:

- @global content_width.
+ @global int $content_width

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit corrects the DocBlock.

*/
public function styling() {
global $post;
if ( ! isset( $post->post_content ) || ! has_shortcode( $post->post_content, self::SHORTCODE ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

This conditional can be removed if styling is called from inside the playlist method.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Hi @westonruter,
It'd be good to remove this conditional. I'll do this if I can figure out the issue that we're discussing above.

$image_url = isset( $track['thumb']['src'] ) ? esc_url( $track['thumb']['src'] ) : '';
$thumb_dimensions = $this->get_thumb_dimensions( $track );
$image_height = isset( $thumb_dimensions['height'] ) ? $thumb_dimensions['height'] : '';
$image_width = isset( $thumb_dimensions['width'] ) ? $thumb_dimensions['width'] : '';

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

The width and height must be defined in AMP, so if it is not available then we need to supply a fallback.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit sets a default width and height for the thumbnails. These are based on the defaults in wp_playlist_shortcode().

*
* @var string.
*/
const PLAYLIST_REGEX = '/(?s)\<script [^>]* class="wp-playlist-script"\>[^<]*?(.*).*?\<\/script\>/';

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

I don't think this regex is quite right. This should be more robust:

const PLAYLIST_REGEX = ':<script type="application/json" class="wp-playlist-script">(.+?)</script>:s';

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit applies your regex.

* @return void.
*/
public function unregister_embed() {
remove_shortcode( self::SHORTCODE );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

I think the other examples of embeds in the plugin aren't quite right in how they call remove_shortcode(). This should actually be restoring the previous shortcode that was added. So in the register_embed method instead of:

add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );

It should do:

global $shortcode_tags;
if ( shortcode_exists( self::SHORTCODE ) ) {
    $this->removed_shortcode = $shortcode_tags[ self::SHORTCODE ];
}
add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );

And then in unregister_embed it should do:

add_shortcode( self::SHORTCODE, $this->removed_shortcode );

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit applies your suggestion of restoring the shortcode before register_embed() changed it.

/**
* Unregisters the playlist shortcode.
*
* @return void.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 15, 2018

Member

The period shouldn't be used here. A period should only appear after a description when it is provided, for example:

@return int The count.

See examples https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 15, 2018

Author Collaborator

Thanks, this commit removes that extra period.

kienstra added some commits Feb 15, 2018

Issue #841: Use wp_json_encode in 'on' attribute.
At Weston's suggestion.
This is easier to understand.
Issue #841: Move ternaary conditionals inside escaping functions.
Before, the output was only escaped
if the value was set.
Also, remove the isset() check for $title.
As Weston mentioned, this is always a string.
Issue #841: Empty string return in audio_playlist().
Inside a conditional, return an empty string.
As Weston mentioned, the PHP DocBlock
indicates a string return value.
Also, add an assertion for this.
And correct the documentation of 'content_width.'
Issue #841: Improve the PLAYLIST_REGEX.
Props @westonruter for the new regex.
Also, remove periods from '@return void.'
Issue #841: Make remove_embed() add previous shortcode.
On Weston's suggestion.
This function should return the shortcode
to the state before this embed handler changed it.
So it stores the previous callback in $removed_shortcode.
And it adds that callback in remove_embed().
Issue #841: Return an array() inside the conditional.
As Weston mentioned, the DocBlock indicates an array().
So return an empty array instead of void.
Issue #841: Remove isset() check for $track['src'].
This should always be present.
@see wp_playlist_shortcode().
Issue #841: Set a default height and width for 'audio.'
Audio playlists have thumbnail images.
If the height and width aren't defined in $data,
Set fallbacks.
These are based on the fallback heights in:
wp_playlist_shortcode().
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2018

Applied Or Responded To Suggestions

Hi @westonruter,
Thanks a lot for reviewing this. I applied all of your points but 2, and replied to those.

kienstra and others added some commits Feb 15, 2018

Issue #841: Remove the carousel buttons fro 'audio' playlist.
The playlist is actually a carousel,
as it's not possible to use <amp-bind>.
But that doesn't match native WP UI.
So remove the buttons that go other carousel items.
One can click the tracks to go to another track.
Enqueue playlist styles just-in-time when used
* Restore wp_print_head_scripts and wp_print_footer_scripts.
* Return associative array instead of positional array in get_thumb_dimensions.
* Further clean phpdoc.
Prevent playlist scripts from being enqueued which will be stripped o…
…ut anyway

* Remove script output by wp_comment_form_unfiltered_html_nonce().
* Remove wp-embed script enqueued by wp_oembed_add_host_js().
?>
<div class="wp-playlist wp-video-playlist wp-playlist-light">
<amp-state id="<?php echo esc_attr( $playlist ); ?>">
<script type="application/json"><?php echo wp_unslash( wp_json_encode( $amp_state ) ); // WPCS: XSS ok. ?></script>

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 17, 2018

Member

Note that wp_unslash() isn't right here. If you want to not have slashes in the output, you should use JSON_UNESCAPED_SLASHES. Otherwise, the slashes are important for security since it ensures that if a string contains </script> it will get output as <\/script> and prevented from breaking out of the scriptl

I'm amending the PR.

@westonruter

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

I'm seeing warnings in the console, including:

  • ampPlaylistCarousel0 is not defined; returning null.
  • Cannot read property "selectedSlide" of "selectedSlide"; returning null.
  • Default value for <DIV [class]="0 == playlist1.currentVideo ? "wp-playlist-item wp-playlist-playing" : "wp-playlist-item""> does not match first result (wp-playlist-item wp-playlist-playing). We recommend writing expressions with matching default values, but this can be safely ignored if intentional.

@westonruter westonruter changed the title Issue #841: Native AMP video playlists. Issue #841: Native AMP audio and video playlists. Feb 18, 2018

@westonruter westonruter added this to the v0.7 milestone Feb 18, 2018

@westonruter westonruter merged commit fe5b33b into develop Feb 18, 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/841-amp-video-playlist branch Feb 18, 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.