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

Improve handling of WordPress post embeds #809

Closed
westonruter opened this issue Nov 17, 2017 · 11 comments · Fixed by #6665
Closed

Improve handling of WordPress post embeds #809

westonruter opened this issue Nov 17, 2017 · 11 comments · Fixed by #6665
Labels
Blocked Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds P1 Medium priority Sanitizers WS:Core Work stream for Plugin core
Projects
Milestone

Comments

@westonruter
Copy link
Member

The handling of post embeds isn't great in AMP right now because the p.wp-embedded-content isn't getting replaced with the iframe.wp-embedded-content like it does in the non-AMP version. The result is some doubling of content.

Compare in a post I just published:

Canonical (non-AMP) version:

image

AMP version:

image

@westonruter
Copy link
Member Author

westonruter commented Sep 24, 2018

Things have regressed in the current version of the plugin. The iframe is not showing up at all now.

Given post_content that looks like:

See post on Make XWP:

https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/

Normal WordPress Rendering

The normal WordPress rendering looks like:

image

With the underlying HTML being:

<blockquote class="wp-embedded-content" data-secret="swkquRFsjz">
	<p><a href="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/">AMP Plugin Release v1.0-beta4</a></p>
</blockquote>
<p>
	<iframe 
		class="wp-embedded-content" 
		sandbox="allow-scripts" 
		security="restricted" 
		style="position: absolute; clip: rect(1px, 1px, 1px, 1px);" 
		src="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/embed/#?secret=swkquRFsjz" 
		data-secret="swkquRFsjz" 
		width="600" 
		height="338" 
		title="&#8220;AMP Plugin Release v1.0-beta4&#8221; &#8212; Make XWP" 
		frameborder="0" 
		marginwidth="0" 
		marginheight="0" 
		scrolling="no">
	</iframe>
</p>

The iframe src is https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/embed/

AMP Plugin Rendering

The rendered AMP response looks like this:

image

There is actually still an iframe there:

<blockquote class="wp-embedded-content" data-secret="swkquRFsjz">
	<p><a href="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/">AMP Plugin Release v1.0-beta4</a></p>
</blockquote>
<amp-iframe
	class="wp-embedded-content amp-wp-enforced-sizes amp-wp-5e9b0a2"
	sandbox="allow-scripts"
	src="https://make.xwp.co/2018/09/24/amp-plugin-release-v1-0-beta4/embed/#?secret=swkquRFsjz"
	data-secret="swkquRFsjz"
	width="600" 
	height="338"
	title="“AMP Plugin Release v1.0-beta4” — Make XWP" 
	frameborder="0" 
	scrolling="no" 
	layout="intrinsic"
	>
	<div placeholder="" class="amp-wp-iframe-placeholder"></div>
</amp-iframe>

But it is now showing up because of this CSS:

.amp-wp-5e9b0a2{position:absolute;clip:rect(1px,1px,1px,1px)}

This is coming from core, as JS in core is supposed to give the iframe the right dimensions when they are received after removing the above style.

Resizing iframe Problem

WordPress post embeds are designed to securely allow themselves to be rendered in iframes that size themselves to the appropriate dimensions. This is by sending a height message from wp-embed-template.js running in the embed template.

The host WordPress site then includes wp-embed.js which receives this height message to resize the iframe accordingly.

There are few challenges here. First of all, the way AMP allows iframes to resize themselves is:

  • The amp-iframe must set the allow-same-origin sandbox attribute.
  • The amp-iframe must be defined with the resizable attribute.
  • The amp-iframe must have an overflow child element.
  • The iframe document must send an embed-size request:
window.parent.postMessage({
  sentinel: 'amp',
  type: 'embed-size',
  height: document.body.scrollHeight
}, '*');

While we can add the iframe attributes without any problem, the message is not what is being sent in WordPress. We could add this JS to the embed template on a WordPress site that is running the AMP plugin, but this wouldn't help every other WordPress site that lacks the AMP plugin. That's a blocker. 🚫

A more severe problem is that the above does not work for post embeds from elsewhere on the same site, since the domain is the same. If attempting, then an error is raised and the amp-iframe is not rendered at all:

❌ Origin of <amp-iframe> must not be equal to container ... if allow-same-origin is set. See https://github.com/ampproject/amphtml/blob/master/spec/amp-iframe-origin-policy.md for details.

If allow-same-origin is removed, then the amp-iframe does render but the resize message is never received.

The easiest solution here would be to load the embed from another domain, but that's not feasible in the general WordPress context. 🚫

The ideal solution for the embedding of posts on the same domain would be to avoid the iframe altogether, and just inline the embed content template directly into the host page. For this to be done, we'd have to:

  1. Convert the embed template to AMP.
  2. Scope the CSS to only apply to the embed content.

The first would be doable, but the second would be highly problematic especially in the context of styles on the host page adversely affecting the styling of the embed content. We'd need a way to block all styles from being inherited into the embed content's wrapper element. The way to do this is to attach the embed content as a shadow DOM. If the embed template is converted to AMP, then we can use AMP shadow DOM. However, this is not available for us to do in AMP since it requires a custom script, unless we do it as part of a custom AMP extension for handling WordPress embeds.

Top Navigation Problem

Another problem has to do with clicking on links in the iframe. All link clicks are intercepted and the URL is sent in a message to the parent window, and the then parent window follows the URL if the host is the same as the host of the URL being embedded. This being the case, even if the amp-iframe has allow-popups-to-escape-sandbox and allow-top-navigation-by-user-activation, clicking on links will not do anything for the normal WordPress embed template.

The logic for whitelisting links to follow is really only important for external URLs being embedded. For embeds for the same site, this is not necessary. But for handling this for external embeds, a custom AMP extension would be needed.

@westonruter westonruter added this to the v1.1 milestone Sep 25, 2018
@westonruter
Copy link
Member Author

Proposal to implement AMP component for embedding WordPress posts: ampproject/amphtml#18378

@westonruter
Copy link
Member Author

We'd need a way to block all styles from being inherited into the embed content's wrapper element. The way to do this is to attach the embed content as a shadow DOM. If the embed template is converted to AMP, then we can use AMP shadow DOM.

Correction: We don't have to use Shadow DOM. The component can create an iframe without the restrictions of amp-iframe.

@westonruter westonruter modified the milestones: v1.1, v1.2 Mar 14, 2019
@westonruter westonruter removed this from the v1.2 milestone May 22, 2019
@amedina amedina changed the title Improve handling of post embeds Improve handling of WordPress post embeds Aug 5, 2019
@jerclarke
Copy link

There's a lot of tickets about WP embeds in AMP, but it seems like this one is the "parent" that's still open.

Just want to confirm current status as a user: This work is still ongoing, and for now WP embeds in AMP simply don't work, and will show as the basic blockquote.

If so that's fine. Luckily the WP embed has a great fallback format (unlike FB and most of the others). Looking forward to the day when all this work you're doing pays off :)

@westonruter
Copy link
Member Author

@jerclarke That's correct. The ultimate resolution for this issue is to introduce a new amp-wordpress-embed component in AMP itself: ampproject/amphtml#24952

Once that is available, we can finalize #3465 which will close this issue.

I don't have a good timetable on when we'll get to this. Perhaps April?

@jerclarke
Copy link

Awesome. Safe pitch to my org to keep using the embeds and be patient for an AMP-native solution in the near future 🙏🏻

@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@westonruter westonruter modified the milestones: v2.1.4, v2.2 Jul 20, 2021
@westonruter westonruter added this to Backlog in Ongoing via automation Jul 20, 2021
@westonruter westonruter moved this from Backlog to To Do in Ongoing Jul 20, 2021
@westonruter
Copy link
Member Author

This is no longer blocked! The amp-wordpress-embed component is now a thing as of ampproject/amphtml#34948.

See example in AMP Playground.

Work can proceed once #6436 is merged or by basing off of that branch.

@westonruter
Copy link
Member Author

Work can proceed once #6436 is merged or by basing off of that branch.

The PR has been merged into develop. This work can proceed.

@westonruter westonruter added the P1 Medium priority label Oct 25, 2021
@bartoszgadomski bartoszgadomski self-assigned this Oct 27, 2021
@westonruter westonruter moved this from To Do to Ready for Review in Ongoing Nov 30, 2021
@westonruter westonruter modified the milestones: v2.2, v2.3 Dec 2, 2021
@westonruter
Copy link
Member Author

westonruter commented Dec 2, 2021

Punting since the bento experiment is still required: https://tundra-functional-double.glitch.me/

image

@westonruter westonruter modified the milestones: v2.3, v2.2.2 Feb 9, 2022
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Mar 3, 2022
@pooja-muchandikar
Copy link

QA Passed ✅ The Post Embeds looks fine.

Before: Posts embeds were not rendering properly

Screenshot 2022-03-22 at 1 08 07 PM


After: Posts embeds are rendering properly

Screenshot 2022-03-22 at 1 07 58 PM


But one observation here:

As per comment the expand button persist or instead the element should increase it's height as soon as it receives the message from the origin. But seems like it's expected behavior.

@maitreyie-chavan maitreyie-chavan moved this from Ready for QA to QA Passed in Ongoing Mar 28, 2022
@westonruter
Copy link
Member Author

As per comment the expand button persist or instead the element should increase it's height as soon as it receives the message from the origin. But seems like it's expected behavior.

If the embed is in the (initial) viewport when it loads, then it's expected that it would not resize since this would cause a layout shift.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds P1 Medium priority Sanitizers WS:Core Work stream for Plugin core
Projects
Ongoing
  
QA Passed
8 participants