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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
00a3f08
Issue #841: Native AMP video playlists.
Feb 11, 2018
8140a40
Issue #841: Remove dependence on test files.
Feb 11, 2018
cddce46
Issue #841: Add audio playlist shortcode support.
Feb 13, 2018
f19c281
Issue #841: Align @param descriptions in PHP DocBlock.
Feb 13, 2018
ca78258
Issue #841: Enqueue Core playlist styling, and custom styling.
Feb 14, 2018
78f7368
Issue #841: Use wp_json_encode in 'on' attribute.
Feb 15, 2018
ccc066d
Issue #841: Move ternaary conditionals inside escaping functions.
Feb 15, 2018
aba8cf1
Issue #841: Empty string return in audio_playlist().
Feb 15, 2018
f13d2ad
Issue #841: Improve the PLAYLIST_REGEX.
Feb 15, 2018
d3cc386
Issue #841: Make remove_embed() add previous shortcode.
Feb 15, 2018
1b07e90
Issue #841: Return an array() inside the conditional.
Feb 15, 2018
168b9ba
Issue #841: Remove isset() check for $track['src'].
Feb 15, 2018
7cae35c
Issue #841: Set a default height and width for 'audio.'
Feb 15, 2018
d01e9ea
Issue #841: Remove the carousel buttons fro 'audio' playlist.
Feb 15, 2018
e08bc8b
Issue #841: Align equals signs to prevent Travis error.
Feb 15, 2018
b093c85
Enqueue playlist styles just-in-time when used
westonruter Feb 17, 2018
b9d9f09
Prevent playlist scripts from being enqueued which will be stripped o…
westonruter Feb 17, 2018
365af92
Eliminate use of data member var in favor of passing as function arg
westonruter Feb 17, 2018
d94888d
Fix AMP warnings regarding initial states and values
westonruter Feb 18, 2018
13370ed
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Feb 18, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions assets/css/amp-playlist-shortcode.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* For the custom AMP implementation of audio 'playlist' shortcode.
*/
.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;
}
1 change: 1 addition & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ function amp_get_content_embed_handlers( $post = null ) {
'AMP_Vine_Embed_Handler' => array(),
'AMP_Facebook_Embed_Handler' => array(),
'AMP_Pinterest_Embed_Handler' => array(),
'AMP_Playlist_Embed_Handler' => array(),
'AMP_Reddit_Embed_Handler' => array(),
'AMP_Tumblr_Embed_Handler' => array(),
'AMP_Gallery_Embed_Handler' => array(),
Expand Down
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class AMP_Autoloader {
'AMP_Issuu_Embed_Handler' => 'includes/embeds/class-amp-issuu-embed-handler',
'AMP_Meetup_Embed_Handler' => 'includes/embeds/class-amp-meetup-embed-handler',
'AMP_Pinterest_Embed_Handler' => 'includes/embeds/class-amp-pinterest-embed',
'AMP_Playlist_Embed_Handler' => 'includes/embeds/class-amp-playlist-embed-handler',
'AMP_Reddit_Embed_Handler' => 'includes/embeds/class-amp-reddit-embed-handler',
'AMP_SoundCloud_Embed_Handler' => 'includes/embeds/class-amp-soundcloud-embed',
'AMP_Tumblr_Embed_Handler' => 'includes/embeds/class-amp-tumblr-embed-handler',
Expand Down
307 changes: 307 additions & 0 deletions includes/embeds/class-amp-playlist-embed-handler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,307 @@
<?php
/**
* Class AMP_Playlist_Embed_Handler
*
* @package AMP
* @since 0.7
*/

/**
* Class AMP_Playlist_Embed_Handler
*
* Creates AMP-compatible markup for the WordPress 'playlist' shortcode.
*
* @package AMP
*/
class AMP_Playlist_Embed_Handler extends AMP_Base_Embed_Handler {

/**
* The tag of the shortcode.
*
* @var string.
*/
const SHORTCODE = 'playlist';

/**
* The max width of the audio thumbnail image.
*
* This corresponds to the max-width in wp-mediaelement.css:
* .wp-playlist .wp-playlist-current-item img
*
* @var string.
*/
const THUMB_MAX_WIDTH = 60;

/**
* The height of the carousel.
*
* @var string.
*/
const CAROUSEL_HEIGHT = 160;

/**
* The pattern to get the playlist data.
*
* @var string.
*/
const PLAYLIST_REGEX = '/(?s)\<script [^>]* class="wp-playlist-script"\>[^<]*?(.*).*?\<\/script\>/';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit applies your regex.


/**
* The ID of individual playlist.
*
* @var int
*/
public static $playlist_id = 0;

/**
* The parsed data for the playlist.
*
* @var array
*/
public $data;

/**
* Registers the playlist shortcode.
*/
public function register_embed() {
add_shortcode( self::SHORTCODE, array( $this, 'shortcode' ) );
add_action( 'wp_enqueue_scripts', array( $this, 'styling' ) );
Copy link
Contributor Author

@kienstra kienstra Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed in favor of $this->styling() being called inside of the audio_playlist or video_playlist methods. That should resolve #954 (comment) and it's the right approach anyway, since we only want to enqueue the styles if the shortcode is actually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @westonruter,
That would definitely be the cleaner appraoch. But it looks like it's not possible to call styling() within AMP_Playlist_Embed_Handler::audio_playlist(), as it's too late to enqueue styling and have AMP_Style_Sanitizer include it in <style amp-custom>. Maybe I'm missing something, though.

}

/**
* Unregisters the playlist shortcode.
*
* @return void.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit removes that extra period.

*/
public function unregister_embed() {
remove_shortcode( self::SHORTCODE );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Enqueues the playlist styling.
*
* @return void.
*/
public function styling() {
global $post;
if ( ! isset( $post->post_content ) || ! has_shortcode( $post->post_content, self::SHORTCODE ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}

wp_enqueue_style( 'wp-mediaelement' );
wp_enqueue_style(
'amp-playlist-shortcode',
amp_get_asset_url( 'css/amp-playlist-shortcode.css' ),
array(),
AMP__VERSION
);
}

/**
* Gets AMP-compliant markup for the playlist shortcode.
*
* Uses the JSON that wp_playlist_shortcode() produces.
* Gets the markup, based on the type of playlist.
*
* @param array $attr The playlist attributes.
* @return string Playlist shortcode markup.
*/
public function shortcode( $attr ) {
$this->data = $this->get_data( $attr );
if ( isset( $this->data['type'] ) && ( 'audio' === $this->data['type'] ) ) {
return $this->audio_playlist();
} elseif ( isset( $this->data['type'] ) && ( 'video' === $this->data['type'] ) ) {
return $this->video_playlist();
}
}

/**
* Gets an AMP-compliant audio playlist.
*
* @return string Playlist shortcode markup.
*/
public function audio_playlist() {
if ( ! isset( $this->data['tracks'] ) ) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit returns an empty string inside that conditional.

}
$container_id = 'ampPlaylistCarousel' . self::$playlist_id;
$selected_slide = $container_id . '.selectedSlide';
self::$playlist_id++;

ob_start();
?>
<div class="wp-playlist wp-audio-playlist wp-playlist-light">
<amp-carousel id="<?php echo esc_attr( $container_id ); ?>" [slide]="<?php echo esc_attr( $selected_slide ); ?>" height="<?php echo esc_attr( self::CAROUSEL_HEIGHT ); ?>" width="auto" type="slides">
<?php
foreach ( $this->data['tracks'] as $track ) :
$title = $this->get_title( $track );
$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'] : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


?>
<div>
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kienstra kienstra Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</div>
</div>
<amp-audio width="auto" height="50" src="<?php echo isset( $track['src'] ) ? esc_url( $track['src'] ) : ''; ?>"></amp-audio>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the src isn't defined, then it shouldn't be output at all. Why would it be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, Weston. This commit removes 2 isset() checks for $track['src']. The source should always be set.

</div>
<?php endforeach; ?>
</amp-carousel>
<?php $this->tracks( 'audio', $container_id ); ?>
</div>
<?php
return ob_get_clean();
}

/**
* Gets an AMP-compliant video playlist.
*
* 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be:

- @global content_width.
+ @global int $content_width

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit corrects the DocBlock.

* @return string $video_playlist Markup for the video playlist.
*/
public function video_playlist() {
global $content_width;
if ( ! isset( $this->data['tracks'], $this->data['tracks'][0]['src'] ) ) {
return;
}
$playlist = 'playlist' . self::$playlist_id;
self::$playlist_id++;

$amp_state = array(
'currentVideo' => '0',
);
foreach ( $this->data['tracks'] as $index => $track ) {
$amp_state[ $index ] = array(
'videoUrl' => isset( $track['src'] ) ? $track['src'] : '',
'thumb' => isset( $track['thumb']['src'] ) ? $track['thumb']['src'] : '',
);
}

$dimensions = isset( $this->data['tracks'][0]['dimensions']['resized'] ) ? $this->data['tracks'][0]['dimensions']['resized'] : null;
$width = isset( $dimensions['width'] ) ? $dimensions['width'] : $content_width;
$height = isset( $dimensions['height'] ) ? $dimensions['height'] : null;

ob_start();
?>
<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>
</amp-state>
<amp-video id="amp-video" src="<?php echo esc_url( $this->data['tracks'][0]['src'] ); ?>" [src]="<?php echo esc_attr( $playlist ); ?>[<?php echo esc_attr( $playlist ); ?>.currentVideo].videoUrl" width="<?php echo esc_attr( $width ); ?>" height="<?php echo isset( $height ) ? esc_attr( $height ) : ''; ?>" controls></amp-video>
<?php $this->tracks( 'video', $playlist ); ?>
</div>
<?php
return ob_get_clean(); // WPCS: XSS ok.
}

/**
* Gets the thumbnail image dimensions, including height and width.
*
* If the width is higher than the maximum width,
* reduces it to the maximum width.
* And it proportionally reduces the height.
*
* @param array $track The data for the track.
* @return array $dimensions The height and width of the thumbnail image.
*/
public function get_thumb_dimensions( $track ) {
if ( ! isset( $track['thumb']['width'], $track['thumb']['height'] ) ) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to return an array per the phpdoc. Otherwise, it should be declared as @return array|false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit returns array().

}
$original_width = intval( $track['thumb']['width'] );
$original_height = intval( $track['thumb']['height'] );
$image_width = min( self::THUMB_MAX_WIDTH, $original_width );
if ( $original_width > self::THUMB_MAX_WIDTH ) {
$ratio = $original_width / self::THUMB_MAX_WIDTH;
$image_height = floor( $original_height / $ratio );
} else {
$image_height = $original_height;
}
return array(
'height' => $image_height,
'width' => $image_width,
);
}

/**
* Outputs the playlist tracks, based on the type of playlist.
*
* These typically appear below the player.
* Clicking a track triggers the player to appear with its src.
*
* @param string $type The type of tracks: 'audio' or 'video'.
* @param string $container_id The ID of the container.
* @return void.
*/
public function tracks( $type, $container_id ) {
?>
<div class="wp-playlist-tracks">
<?php
$i = 0;
foreach ( $this->data['tracks'] as $index => $track ) {
$title = $this->get_title( $track );
if ( 'audio' === $type ) {
$on = 'tap:AMP.setState({' . $container_id . ': {selectedSlide: ' . $i . '}})';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) ) ) . ')';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit uses wp_json_encode().

$item_class = esc_attr( $i ) . ' == ' . esc_attr( $container_id ) . '.selectedSlide ? "wp-playlist-item wp-playlist-playing" : "wp-playlist-item"';
} elseif ( 'video' === $type ) {
$on = 'tap:AMP.setState({' . $container_id . ': {currentVideo: ' . $index . '}})';
$item_class = esc_attr( $index ) . ' == ' . esc_attr( $container_id ) . '.currentVideo ? "wp-playlist-item wp-playlist-playing" : "wp-playlist-item"';
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other such cases in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<a class="wp-playlist-caption" on="<?php echo isset( $on ) ? esc_attr( $on ) : ''; ?>">
<?php echo esc_html( strval( $i + 1 ) . '.' ); ?> <span class="wp-playlist-item-title"><?php echo isset( $title ) ? esc_html( $title ) : ''; ?></span>
</a>
<?php if ( isset( $track['meta']['length_formatted'] ) ) : ?>
<div class="wp-playlist-item-length"><?php echo esc_html( $track['meta']['length_formatted'] ); ?></div>
<?php endif; ?>
</div>
<?php
$i++;
}
?>
</div>
<?php
}

/**
* Gets the data for the playlist.
*
* @param array $attr The shortcode attributes.
* @return array $data The data for the playlist.
*/
public function get_data( $attr ) {
$markup = wp_playlist_shortcode( $attr );
preg_match( self::PLAYLIST_REGEX, $markup, $matches );
if ( empty( $matches[1] ) ) {
return;
}
return json_decode( $matches[1], true );
}

/**
* Gets the title for the track.
*
* @param array $track The track data.
* @return string $title The title of the track.
*/
public function get_title( $track ) {
if ( ! empty( $track['caption'] ) ) {
return $track['caption'];
} elseif ( ! empty( $track['title'] ) ) {
return $track['title'];
}
}

}
Loading