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

kienstra
Copy link
Contributor

@kienstra kienstra 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.

Ryan Kienstra added 3 commits February 11, 2018 13:54
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.
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.
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
Copy link
Contributor Author

kienstra 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
Copy link
Member

@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.

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
Copy link
Contributor Author

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

@westonruter westonruter mentioned this pull request Feb 15, 2018
2 tasks
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().

}

?>
<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.

<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.

*/
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.

* 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.

*/
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.

$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().

*
* @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.

* @return void.
*/
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.

/**
* 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.

Ryan Kienstra added 8 commits February 15, 2018 11:43
At Weston's suggestion.
This is easier to understand.
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.
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.'
Props @westonruter for the new regex.
Also, remove periods from '@return void.'
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().
As Weston mentioned, the DocBlock indicates an array().
So return an empty array instead of void.
This should always be present.
@see wp_playlist_shortcode().
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
Copy link
Contributor Author

kienstra 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.

Ryan Kienstra and others added 5 commits February 15, 2018 16:04
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.
* Restore wp_print_head_scripts and wp_print_footer_scripts.
* Return associative array instead of positional array in get_thumb_dimensions.
* Further clean phpdoc.
…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>
Copy link
Member

Choose a reason for hiding this comment

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

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

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
@westonruter westonruter deleted the add/841-amp-video-playlist branch February 18, 2018 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants