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

VIP: Support for protected-embeds #10

Closed
mjangda opened this issue Sep 29, 2015 · 12 comments
Closed

VIP: Support for protected-embeds #10

mjangda opened this issue Sep 29, 2015 · 12 comments
Labels
Enhancement New feature or improvement of an existing one Integration: Jetpack

Comments

@mjangda
Copy link
Contributor

mjangda commented Sep 29, 2015

Protected Embeds are kinda weird (they rely on a javascript) and we probably need some workarounds for them.

@mjangda mjangda added Enhancement New feature or improvement of an existing one Integration: Jetpack labels Sep 29, 2015
@mjangda mjangda added this to the Post-Developer Preview milestone Sep 29, 2015
@mjangda mjangda removed this from the Post-Developer Preview milestone Oct 30, 2015
@philipjohn
Copy link
Contributor

What does the protected embeds JS do? I see some resize stuff.

Is there an easy way, within Protected Embeds that we can just say if ( is_amp() ) or something, and conditionally print the JS? Or is that lazy wishful thinking?

@mjangda
Copy link
Contributor Author

mjangda commented Jan 22, 2016

We can add an is_amp_endpoint() function to check.

@mjangda
Copy link
Contributor Author

mjangda commented Feb 10, 2016

This is a bit of a pain. Protected embeds use javascript (form submit to an iframe via POST) to avoid GET URL limits in some browsers. Since javascript isn't allowed, we can't swap this out as-is so will need to either revert back to GET with an iframe for AMP or find some other clever approach.

@maxhartshorn
Copy link

This would be a very good feature to have. We try to leverage shortcodes as much as possible but still many of our producers copy/paste embeds directly which get converted into protected embeds. It's hard to justify keeping this as an option if protected embeds don't work in AMP

@brianlayman
Copy link

Likewise, sites that cannot directly include youtube videos use this method to display them. Not having this feature disables all videos on the site.

@brianlayman
Copy link

Also on VIP scribd embeds convert to protected-iframes and so are used without any choice in the matter.

Here is an example of the embed from the site:

<iframe class="scribd_iframe_embed" src="https://www.scribd.com/embeds/300107315/content?start_page=1&view_mode=scroll&show_recommendations=true" data-auto-height="false" data-aspect-ratio="undefined" scrolling="no" id="doc_5698" width="100%" height="600" frameborder="0"></iframe>

Which gets converted to:
[protected-iframe id="455971f3fb2c791c8e5026f76bd27baa-62957618-80319310" info="https://www.scribd.com/embeds/300107315/content?start_page=1&view_mode=scroll&show_recommendations=true" width="100%" height="600" frameborder="0" class="scribd_iframe_embed" scrolling="no"]

@brianlayman
Copy link

Would it be possible to GET in there for now? That would be much better having over 8000 posts with missing data.

Could you please provide the protected-iframe embed source so that we can look at a solution?

@brianlayman
Copy link

For additional testing and clarification of the of the severity of this problem, please note that Live stream embeds also convert to protected-iframes:

<iframe src="http://livestream.com/accounts/5383514/events/2395882/player?width=780&height=439&autoPlay=false&mute=false" width="780" height="439" frameborder="0" scrolling="no"> </iframe>

@mjangda
Copy link
Contributor Author

mjangda commented Mar 3, 2016

This should mostly work now. There's a wpcom commit to allow switching to standard iframes and 702b18e forces it on for AMP content.

Caveats:

  • some embeds may not work in some browsers (because of the really long URLs).
  • dynamic resizing most likely won’t work either (although, i haven’t tested).

@mjangda mjangda closed this as completed Mar 3, 2016
@bcampeau
Copy link
Contributor

bcampeau commented Apr 21, 2016

There's actually still a bug with this for AMP. Sometimes the protected embeds don't set a width and/or height param for the iframe. This isn't allowed in AMP and will cause a validation error. I'm seeing a lot fo these on nypost.com. Here's an example: http://nypost.com/2015/12/06/almost-impossible-to-stop-all-terrorism-homeland-security-chair/amp/

Given that protected iframes don't work off VIP, I'm a little unsure of the best path to resolve.

@mjangda
Copy link
Contributor Author

mjangda commented Apr 21, 2016

I think the bigger issue is that the iframe-sanitizer doesn't handle empty width/height values well. For now, I've pushed a workaround for protected embeds (internal ref: r134693-wpcom).

@bcampeau
Copy link
Contributor

Fair point. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one Integration: Jetpack
Projects
None yet
Development

No branches or pull requests

5 participants