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

Support all default embeds where relevant #841

Closed
MackenzieHartung opened this Issue Jan 4, 2018 · 13 comments

Comments

Projects
6 participants
@MackenzieHartung
Copy link

MackenzieHartung commented Jan 4, 2018

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

@MackenzieHartung MackenzieHartung added this to To Do in v0.7 Jan 4, 2018

@ThierryA ThierryA added this to the v0.7 milestone Jan 8, 2018

@MackenzieHartung MackenzieHartung moved this from To Do to Definition in v0.7 Jan 9, 2018

@MackenzieHartung MackenzieHartung removed this from Definition in v0.7 Jan 9, 2018

kienstra added a commit that referenced this issue 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.

kienstra added a commit that referenced this issue Feb 11, 2018

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.

kienstra added a commit that referenced this issue Feb 13, 2018

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

kienstra commented Feb 14, 2018

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.

kienstra added a commit that referenced this issue Feb 14, 2018

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 added a commit that referenced this issue Feb 15, 2018

Issue #841: Use wp_json_encode in 'on' attribute.
At Weston's suggestion.
This is easier to understand.

kienstra added a commit that referenced this issue Feb 15, 2018

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.

kienstra added a commit that referenced this issue Feb 15, 2018

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

kienstra added a commit that referenced this issue Feb 15, 2018

Issue #841: Improve the PLAYLIST_REGEX.
Props @westonruter for the new regex.
Also, remove periods from '@return void.'

kienstra added a commit that referenced this issue Feb 15, 2018

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

kienstra added a commit that referenced this issue Feb 15, 2018

Issue #841: Return an array() inside the conditional.
As Weston mentioned, the DocBlock indicates an array().
So return an empty array instead of void.

kienstra added a commit that referenced this issue Feb 15, 2018

Issue #841: Remove isset() check for $track['src'].
This should always be present.
@see wp_playlist_shortcode().

kienstra added a commit that referenced this issue Feb 15, 2018

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 added a commit that referenced this issue 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.

kienstra added a commit that referenced this issue Feb 15, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 21, 2018

Request For QA

Hi @csossi,
Could you please test this? We should run the embed test script for you, as it outputs all of the embeds.

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.

@kienstra kienstra assigned csossi and unassigned kaitnyl Feb 21, 2018

@kienstra kienstra reopened this Feb 21, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 22, 2018

Test Page

Hi @csossi,
Could you please use this test page, which @westonruter created?

kienstra added a commit that referenced this issue Feb 23, 2018

Issue #841: Rename property $current_source.
Before, this was $current_plugin_output.
This needs to account for themes and plugins.
@csossi

This comment has been minimized.

Copy link

csossi commented Feb 24, 2018

Thanks, gents - on it

@csossi

This comment has been minimized.

Copy link

csossi commented Feb 24, 2018

@westonruter, @kienstra - QA Results:

  • CloudUp Image Embed (blank/white space)
  • CollegeHumor Embed - (blank/black space)
  • Flickr Video Embed - image appears, but no embedded video
  • Photobucket Embed - displays URL only
  • Screencast embed - "Unable to display content. Adobe Flash os required"

Please let me know if any of these are "ok as is"

@csossi csossi assigned westonruter and unassigned csossi Feb 25, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 26, 2018

CollegeHumor Embed - (blank/black space)

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

Screencast embed - "Unable to display content. Adobe Flash os required"

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:

CloudUp Image Embed (blank/white space)
Flickr Video Embed - image appears, but no embedded video
Photobucket Embed - displays URL only

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 26, 2018

Thanks, Looking At The Results Now

Hi @csossi,
Thanks a lot for testing this. I'm looking at the results now on the test page.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 26, 2018

Initial Findings

CloudUp Image Embed (blank/white space)

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

Jetpack seems to change the src of this embed's <amp-img> from:
https://i.cloudup.com/cTZsPiiemn-900x900.jpeg
to:
https://i2.wp.com/i.cloudup.com/cTZsPiiemn.jpeg?resize=694%2C460&ssl=1
And that URL has a 404 response.

Flickr Video Embed - image appears, but no embedded video

This is output as an <amp-img>, but should probably be an <amp-iframe> instead. Without this plugin, it's simply an <iframe>.

Photobucket Embed - displays URL only

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.

@westonruter westonruter assigned kienstra and unassigned westonruter Mar 7, 2018

@ThierryA ThierryA closed this Mar 8, 2018

@ThierryA ThierryA added the Sprint 5 label Mar 8, 2018

@ThierryA ThierryA added this to QA in v0.7 Mar 8, 2018

@kienstra kienstra moved this from QA to To Do in v0.7 Mar 15, 2018

@westonruter westonruter moved this from To Do to QA in v0.7 Mar 21, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 21, 2018

Since this issue is closed I moved it into the QA column. Or is it considered QA'ed?

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 21, 2018

Hi @westonruter,
It might be good to reopen this and move it into 'To Do.' The issues raised in QA still need development, I think.

@westonruter westonruter reopened this Mar 21, 2018

@westonruter westonruter moved this from QA to To Do in v0.7 Mar 21, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 27, 2018

Remaining Issues

CloudUp Image Embed (blank/white space)

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 Video Embed - image appears, but no embedded video

Flickr’s oEmbed response for video would ideally return a <video> or <iframe>. It currently depends on an (AMP-disallowed) inline script to create a <video>.

See the html value in this response:
https://www.flickr.com/services/oembed/?format=json&url=http%3A//flic.kr/p/5TPDWa

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:

Your account is not able to post to the forums. If you require help, use the Help by Email system instead.

Photobucket Embed - displays URL only

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.

@postphotos postphotos moved this from To Do to In Progress in v0.7 Mar 27, 2018

@MackenzieHartung MackenzieHartung added Sprint 2 and removed Sprint 5 labels Mar 27, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 28, 2018

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
http://i1259.photobucket.com/albums/ii543/iamnotpeterpan/EditPostlsaquoDennisDoesCricketmdashWordPress_zpsf72cc13d.png
http://i1067.photobucket.com/albums/u434/madrianson/IMG_6979_zpsymenfin2.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
When I tried to post a support issue on Flickr.com with my new Pro account, this appeared:

Oops! Looks like you followed a bad link.

So I sent an email to support, asking if an oEmbed response for a video could return a <video> or <iframe>, instead of depending on an inline <script>

@kienstra kienstra moved this from In Progress to Ready for Merging in v0.7 Mar 28, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 28, 2018

Ready For Merging

There's no more action needed on this, unless I get a response from Flickr about their oEmbed endpoint.

@ThierryA ThierryA closed this Apr 2, 2018

@kevincoleman kevincoleman moved this from Ready for Merging to Production Release in v0.7 May 8, 2018

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