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

Embed block/floated embed block audit #10440

Closed
jasmussen opened this issue Oct 9, 2018 · 16 comments
Closed

Embed block/floated embed block audit #10440

jasmussen opened this issue Oct 9, 2018 · 16 comments
Labels
[Block] Embed Affects the Embed Block Good First Issue An issue that's suitable for someone looking to contribute for the first time

Comments

@jasmussen
Copy link
Contributor

In #10437 we discovered an issue with embedding Instagram posts.

The issue was that when we float an embed, we set a minimum and a maximum width on the floated embed, because the embed itself does not necessarily have a specific intrinsic width we can work with. In the case of Instagram, the max-width we set (290px) was smaller than the minimum width supported by Instagram embeds (326px).

Following this, we should do an audit on every embed block, floated or otherwise, and verify that they are not cropped at any time.

@jasmussen jasmussen added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Oct 9, 2018
@jasmussen jasmussen added this to the 4.1 milestone Oct 9, 2018
@gziolo
Copy link
Member

gziolo commented Oct 16, 2018

@jasmussen is there any more work left? I see one PR merged. Can we move to 4.2?

@jasmussen
Copy link
Contributor Author

Yes okay to move. But would be good to do a thorough test sooner rather than later 😅

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 27, 2018
@mtias mtias added [Block] Embed Affects the Embed Block Needs Testing Needs further testing to be confirmed. labels Nov 12, 2018
@mtias mtias modified the milestones: WordPress 5.0 RC, WordPress 5.0 Nov 15, 2018
@designsimply
Copy link
Member

designsimply commented Nov 15, 2018

Call to action: audit every embed block, floated or otherwise, and verify that they are not cropped at any time.

  1. Twitter
  2. YouTube
  3. Facebook
  4. Instagram
  5. WordPress
  6. soundCloud
  7. Spotify
  8. Flickr
  9. Vimeo
  10. Animoto
  11. Cloudup
  12. CollegeHumor
  13. Dailymotion
  14. Funny Or Die
  15. Hulu
  16. Imgur
  17. Issuu
  18. Kickstarter
  19. Meetup.com
  20. Mixcloud
  21. Photobucket
  22. Polldaddy
  23. Reddit
  24. ReverbNation
  25. Screencast
  26. Scribd
  27. Slideshare
  28. SmugMug
  29. Speaker Deck
  30. TED
  31. Tumblr
  32. VideoPress
  33. WordPress.tv

The list of core embeds above was pulled from https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/embed/core-embeds.js

@designsimply
Copy link
Member

I tested a WordPress.tv embed of https://wordpress.tv/2018/11/08/matt-mullenweg-qa-at-wordcamp-portland-2018/ using WordPress 4.9.8 and Gutenberg 4.4 cdeb40986 with Firefox 63.0.1 on macOS 10.13.6. All alignments look good (including wide and full) and the embed doesn't get cropped unexpectedly.

Aside: I noticed that the sizes of right-aligned WordPress.tv embeds don't match in the editor versus the front-end for Twenty Nineteen (but work properly for Twenty Seventeen) so I reported that separately at WordPress/twentynineteen#610

@jasmussen
Copy link
Contributor Author

Thanks for the call to action, Sheri! I've done some testing too:

  • VideoPress, can confirm it works right in floats left and right, and in vanilla and TwentyNineteen editor styles.
  • Tumblr, same, ✔.
  • Ted ✔
  • Speaker Deck ✔
  • Smug Mug — not so great. Technically the floats aspect works fine, which is why I've checked the box on this page, but there are issues with the embeds collapsing, and some of them use Flash, which causes weirdness. I will open a separate ticket for this.
  • Slideshare: ✔
  • Scribd: does not appear to embed anything at all, neither in Gutenberg or the classic editor. Could use a sanity check on this one. What are we supposed to be able to embed? Tried embedding https://www.scribd.com/article/392711110/The-Democrats-New-Power-Tools
  • Screencast.com: ✔
  • Twitter: ✔
  • YouTube: ✔
  • Instagram: ✔
  • WordPress: ✔
  • Soundcloud: ✔

@jasmussen
Copy link
Contributor Author

  • Spotify: ✔
  • Flickr: Appears square and pixelated in the editor but not the frontend. But checking regardless because the floats aspect works fine.
  • Vimeo: ✔
  • Kickstarter: ✔

@jasmussen
Copy link
Contributor Author

Tickets opened:

#11962
#11960

@arunsathiya
Copy link
Contributor

I tested the ones below, and I don't see any unexpected cropping when the embeds are set to float/aligned to center.

I also noticed that the Control + Option + Z command to remove the embed blocks don't work on most. I wasn't able to get it working on Reddit, Cloudup and Imgur so far.

Works fine on Facebook. I can search for the ones where it doesn't work and file a ticket, as required.

@designsimply
Copy link
Member

Tested Meetup.com embeds using WordPress 4.9.8 and Gutenberg 4.4 7e814e393 with Firefox 63.0.1 on macOS 10.13.6 and found another small Twenty Nineteen nitpick ( WordPress/twentynineteen#624 ) but otherwise it looks good.

@AtrumGeost
Copy link

Did some testing :D

✅ Twitter
✅ YouTube
✅ Facebook
✅ Instagram
✅ WordPress
✅ SoundCloud
✅ Spotify
✅ Flickr
✅ Vimeo
👾 Animoto:
There's an extra space below the video http://cld.wthms.co/VdxgIP

URL: https://animoto.com/play/FQ12uazXH10WDez7vsfh2g

✅ Cloudup
👾 CollegeHumor:
Got an empty space.
http://cld.wthms.co/RqmGwc

URL: http://www.collegehumor.com/video/40004082/the-girl-with-a-birthday-month
✅ Dailymotion
👾 Funny Or Die:
Got this message: "Sorry, we could not embed that content."
http://cld.wthms.co/mAOqGR

URL: https://www.funnyordie.com/2018/11/16/18097374/depression-olympics-sketch-parody-al-madrigal
👾 Hulu:
Got an error since I'm outside US: http://cld.wthms.co/qsszHP
And an error when I tried with VPN: http://cld.wthms.co/JGZlQi

URL: https://www.hulu.com/watch/771496
✅ Imgur
✅ Issuu
👾 Kickstarter
Empty space on editor: http://cld.wthms.co/05Me72
Looks odd: http://cld.wthms.co/rr2jCJ

URL: https://www.kickstarter.com/projects/antsylabs/pixl-a-magnetic-building-system
✅ Meetup.com
✅ Mixcloud
👾 Photobucket
Didn't work. Tried two URLS. They gave me this error: "Sorry, we could not embed that content."

URL 1: http://i1044.photobucket.com/albums/b444/zzztttzzz/20181117_084341_zpswlblzpk2.jpg
URL 2: http://photobucket.com/gallery/user/zzztttzzz/media/bWVkaWFJZDoxOTY3ODY4MjE=/?ref=

✅ Polldaddy (Crowdsignal)
✅ Reddit
👾 ReverbNation
Big space below embed block: http://cld.wthms.co/y7bdkf

URL: https://www.reverbnation.com/sammybrue

👾 Screencast
Appears a black space when aligning the image:
Centered: http://cld.wthms.co/bV381m
Aligned: http://cld.wthms.co/ldsG38

URL: http://www.screencast.com/t/9QpXpRrzf

✅ Scribd
✅ Slideshare
👾 SmugMug
Works but doesn't show a preview: http://cld.wthms.co/PXm1NZ
I'm guessing it is because it uses Flash.

URL: https://foaloce.smugmug.com/Travels/Italy/Toscana-april-2012/

👾 Speaker Deck
It looks cropped: http://cld.wthms.co/HBfKQg

URL: https://speakerdeck.com/maltzj/code-reviewing-like-a-champion

✅ TED
✅ Tumblr
✅ VideoPress
✅ WordPress.tv

WordPress 4.9.8 running Twenty Seventeen theme

@notnownikki
Copy link
Member

funnyordie no longer supports oembed, we should remove the block and remove the provider from core WP.

@notnownikki
Copy link
Member

SmugMug Works but doesn't show a preview: http://cld.wthms.co/PXm1NZ I'm guessing it is because it uses Flash.

Confirmed, Failed to load 'https://cdn.smugmug.com/ria/ShizamSlides-2015060701.swf' as a plugin, because the frame into which the plugin is loading is sandboxed. in the console, so this might have to go on the list of domains we can't show previews for.

@notnownikki
Copy link
Member

Photobucket Didn't work. Tried two URLS. They gave me this error: "Sorry, we could not embed that content."

Photobucket no longer supports oembed. Another one we should remove from the blocks and from core code.

@thehenrybyrd
Copy link

thehenrybyrd commented Nov 23, 2018

👾 CollegeHumor:
Got an empty space.
http://cld.wthms.co/RqmGwc

Confirmed - same happened to me when embedding http://www.collegehumor.com/video/40004155/sorry-im-a-slow-eater
However, using the "share" link from CollegeHumor generates a YouTube link, and embedding that worked fine.

Sidenote: sometimes when a blank box is embedded as with CollegeHumor, it's difficult to work with in the editor because the edges can't be seen: https://cloudup.com/cdC9adRwwVu
That means clicking around where you think it is until you get the block selected.
Unfortunately I wasn't able to reproduce this behavior in another post, but here's what I was seeing: https://cloudup.com/cuj2l7eeBmb

WordPress 4.9.8 running Twenty Seventeen theme.

@jasmussen
Copy link
Contributor Author

Thank you all so much for testing here.

This audit has revealed a number of issues, some that were present even in 4.9.

The initial issue was related to how the embeds work with regards to floats, though, and it seems this aspect has worked well. For that reason I'm tempted to close this ticket in favor of the individual issues that have been surfaced as a result of this audit.

But for now atleast, I'm moving it out of the 5.0 milestone as the audit has technically been completed.

@jasmussen jasmussen removed this from the WordPress 5.0 milestone Nov 29, 2018
@youknowriad
Copy link
Contributor

This served its purpose I think. Let's close and open individual issues.

@designsimply designsimply removed the Needs Testing Needs further testing to be confirmed. label Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block Good First Issue An issue that's suitable for someone looking to contribute for the first time
Projects
None yet
Development

No branches or pull requests

9 participants