Support all default embeds where relevant #841
Comments
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.
|
Current Embed Support The matrix at the top of this pull request seems to track the current state of embed support. But I haven't verified it. |
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.
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().
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.
|
Request For QA Hi @csossi, The embeds in @kaitnyl's matrix that weren't supported are almost all supported now, thanks to her work. Could you please verify that all of the items in that matrix appear as expected? The only remaining issues that I see are with the CollegeHumor video and Screencast. Also, the video shortcode seems to output a thumbnail that's unrelated to the video. |
|
Test Page Hi @csossi, |
Before, this was $current_plugin_output. This needs to account for themes and plugins.
|
Thanks, gents - on it |
|
@westonruter, @kienstra - QA Results:
Please let me know if any of these are "ok as is" |
This is expected. It's a problem on the CollegeHumor side. “The embed attempts to load http content through an https amp-iframe src (amp-iframe requires https). This causes the browser to reject it.” #889
This is expected. This is a problem on the Screencast side. “I'm seeing the same error appear on the desktop version. This looks like it might be the downside of Screencast using .swfs.” #889 I do not know about these:
|
|
Initial Findings
This seems to be related to Jetpack. On deactivating the 'Image Performance' setting in the Jetpack dashboard on the dev site, this displayed as expected.
Jetpack seems to change the
This is output as an
The URL on the test page doesn't even render without this plugin. Nor do some other URLs from the Photobucket documentation. I'll look at this more, but there's a chance that this isn't even supported fully in WordPress. |
|
Since this issue is closed I moved it into the QA column. Or is it considered QA'ed? |
|
Hi @westonruter, |
|
Remaining Issues
The Cloudup image embed on this test page now displays as expected, as Jetpack isn't setup on that site, only activate. The issue described above with the incorrect URL occurs on Jetpack whether this AMP plugin is active or inactive. So it's not specifically an issue with this plugin.
Flickr’s oEmbed response for video would ideally return a See the I tried to post a support issue to the Flickr forums, asking if they plan to support AMP in their video oEmbeds. But at the moment, the page says this, even with my new Pro account:
This is still an issue, but it's also an issue with non-AMP pages. I'll look briefly for embed URLs that work, but I think this isn't very well supported even in non-AMP WordPress. |
|
Photobucket It looks like WordPress doesn't support Photobucket embeds very well. These embed URLs don't render, even with this plugin disabled: http://i13.photobucket.com/albums/a280/barty1974/Quarterback%20Memorabilia/Rodgers%20Aaron2.jpg This Trac ticket (from 3 years ago) mentions that "Finding an oEmbeddable URL for an image on Photobucket is all but impossible..." I'm happy to look at this if there is a URL that renders. Flickr So I sent an email to support, asking if an oEmbed response for a video could return a |
|
Ready For Merging There's no more action needed on this, unless I get a response from Flickr about their oEmbed endpoint. |
Acceptance Criteria
AC1: Add AMP support for “Media Video Playlist” content
AC2: Add AMP support for “Media Audio Playlist” content
AC3: Add AMP support for “Issu Embed” content
AC4: Enhance the support for “Post Embed” content. Currently, Post Embeds doubles content in places. The issue stems from the p.wp-embedded-content not getting replaced with the iframe.wp-embedded-content.
AC5: Enhance the support for “CollegeHumor Video Embed”. Currently the video player appears, but does not play. Ensure the video content plays.
AC6: Enhance the support for “Meetup Embed”. Currently, the markup appears, but there is no styling. Ensure the styling works properly in AMP.
AC7: Enhance the support for “Reddit Embed”. Currently, some markups appear, but without styling. Ensure that the styling works properly in AMP.
AC8: Enhance the support for the “Screencast Embed”. Currently, even with the domain added to Chrome’s Flash “Allow” list, when playing it displays “Unable to display content. Adobe Flash is required.”. Ensure that the error does not display, and that the video plays successfully.
AC9: Enhance the support for “Tumblr Embed”. Currently, only the link displays. Ensure that the content displays properly within AMP.
AC10: Enhance the support for the “WordPress Plugin Directory Embed”. Currently it displays mostly as expected, but with slightly different styling and an extra link. Ensure that the styling appears properly, and an extra link is not displayed.
See #806 as reference
The text was updated successfully, but these errors were encountered: