✨Bento `amp-wordpress-embed` #34948

ediamin · 2021-06-22T11:33:01Z

This PR adds a bento component for amp-wordpress-embed.

This PR solves the issues described in #18378

iframe Resizing: iframe height updates according to the height message sending by wp-embed-template.js.
Same-origin embeds: Embedding same origin url of a WordPress site is possible with this component.
Navigation: The links inside the embed are clickable and browser redirects to the related link.

amp-owners-bot · 2021-06-22T11:33:10Z

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-wordpress-embed/validator-amp-wordpress-embed.protoascii

caroqliu

A couple questions and recommendations, otherwise LGTM.

caroqliu · 2021-06-22T14:53:14Z

extensions/amp-wordpress-embed/1.0/amp-wordpress-embed.js

+  init() {
+    return dict({
+      'requestResize': (height) => {
+        this.forceChangeHeight(height);


Forcing height change may cause layout shift -- would it suffice to use this.attemptChangeHeight instead? The key difference is that the component makes a request to change height by the runtime which can be denied.

I followed amp-instagram bento component and it is using forceChangeHeight, so I used this in amp-wordpress-embed too. I've just tested with attemptChangeHeight, but it is giving me this following warning in console

[CustomElement] Cannot resize element and overflow is not available

I was testing with this code,

init() { return dict({ 'requestResize': (height) => this.attemptChangeHeight(height).catch(() => { /* ignore failures */ }), }); }

Please suggest what to do.

The code you were testing with (attemptChangeHeight) is recommended.

The warning you see is acceptable and encourages publishers to provide an overflow element. When resizing is not possible, such as when there is text below, the iframe element is clipped and can be resized on user interaction with an "overflow" prompt:

Clicking allows the component to be resized:

I've added a comment with suggested edits to your Storybook file so you can have the same sample I used.

While the overflow is best, should it be required? Namely, I think amp-wordpress-embed should be treated the same as amp-twitter, amp-gist, amp-facebook, etc which all will resize regardless of whether it will cause a layout shift.

@caroqliu I've updated the code using attemptChangeHeight. Please test the component in storybook and let me know what do you think.

While the overflow is best, should it be required? Namely, I think amp-wordpress-embed should be treated the same as amp-twitter, amp-gist, amp-facebook, etc which all will resize regardless of whether it will cause a layout shift.

The Bento port is an opportunity to remove legacy layout shift causing behavior whenever possible to provide the most stable end user experience. In design review yesterday we discussed moving amp-facebook to attemptChangeHeight (#34969). Note that the Preact version will still resize without user interaction as it is not managed by the AMP runtime.

We'll try to address this change in the AMP plugin by capturing the heights of embeds when they are rendered in the editor client-side to then be available server-side when the content is served: ampproject/amp-wp#4729.

extensions/amp-wordpress-embed/1.0/component.js

caroqliu · 2021-06-22T15:00:39Z

extensions/amp-wordpress-embed/OWNERS

+{
+  rules: [
+    {
+      owners: [{name: 'ampproject/wg-bento'}],


Is there a WP owner that would be appropriate to include?

Could you please suggest me which name/names should I use here?

@westonruter do you have any suggestions?

@westonruter Who would be a good owner for this component going forward?

Why not @ediamin? You could add me as well.

Updated. Added westonruter as an owner.

extensions/amp-wordpress-embed/validator-amp-wordpress-embed.protoascii

caroqliu · 2021-06-22T15:14:46Z

extensions/amp-wordpress-embed/1.0/component.js

+          // Only follow a link message for the currently-active iframe if the link is for the same origin.
+          // This replicates a constraint in WordPress's wp.receiveEmbedMessage() function.
+          if (matchesMessagingOrigin(data.value)) {
+            window.top.location.href = data.value;


Should this update the local window over the global one? If using the local window you can access from the ref given to the IframeEmbed:

amphtml/extensions/amp-base-carousel/1.0/component.js

Line 172 in 5afde32

const win = toWin(containRef.current.ownerDocument.defaultView);

The current ref given to IframeEmbed is a function and doesn't give me anything for ref.current is undefined. I tried using a new ref created by const containRef = useRef(null) and give it to the IframeEmbed, but containRef.current.ownerDocument returns undefined.

I would recommend getting a reference and setting win as a state variable so as to trigger callback redefinition:

const contentRef = useRef(null); const [win, setWin] = useState(null); const messageHandler = useCallback(() => { ... if (matchesMessagingOrigin(data.value) && win) { win.top.location.href = data.value; } }, [win]); useLayoutEffect(() => { setWin(contentRef.current?.ownerDocument?.defaultView); }, []);

Then give the IframeEmbed the contentRef={contentRef}

Thanks @caroqliu for the code snippet. I've updated the PR with your suggested code.

extensions/amp-wordpress-embed/1.0/test/test-amp-wordpress-embed.js

extensions/amp-wordpress-embed/validator-amp-wordpress-embed.protoascii

kristoferbaxter

Leaving review approval to @caroqliu, but left a few comments and questions.

kristoferbaxter · 2021-06-23T13:55:52Z

extensions/amp-wordpress-embed/1.0/component.js

+          if (typeof data.value === 'number') {
+            // Make sure the new height is between 200px and 1000px.
+            // This replicates a constraint in WordPress's wp.receiveEmbedMessage() function.
+            const height = Math.min(Math.max(data.value, 200), 1000);


Does this constraint need to be enforced on the embedder side or could it be done by the underlying embed?

Removed the constraint and simply using data.value as the height.

kristoferbaxter · 2021-06-23T13:57:51Z

extensions/amp-wordpress-embed/1.0/component.js

+/**
+ * @param {?string} message
+ */
+function displayWarning(message) {


@caroqliu Is this a common pattern? Seems like this should just be inlined.

displayWarning is slightly preferred to inlining so we don't have to have the /* OK */ when calling more than once in a file. Only used in two other components.

SGTM, our compiler should remove the indirection so long as the method is never exported or we use a single tree for a compilation unit.

extensions/amp-wordpress-embed/1.0/test/test-amp-wordpress-embed.js

kristoferbaxter · 2021-06-23T13:59:54Z

extensions/amp-wordpress-embed/OWNERS

+{
+  rules: [
+    {
+      owners: [{name: 'ampproject/wg-bento'}],


@westonruter Who would be a good owner for this component going forward?

caroqliu · 2021-06-23T13:52:16Z

extensions/amp-wordpress-embed/1.0/amp-wordpress-embed.js

+  init() {
+    return dict({
+      'requestResize': (height) => {
+        this.forceChangeHeight(height);


The code you were testing with (attemptChangeHeight) is recommended.

The warning you see is acceptable and encourages publishers to provide an overflow element. When resizing is not possible, such as when there is text below, the iframe element is clipped and can be resized on user interaction with an "overflow" prompt:

Clicking allows the component to be resized:

I've added a comment with suggested edits to your Storybook file so you can have the same sample I used.

extensions/amp-wordpress-embed/1.0/storybook/Basic.amp.js

caroqliu · 2021-06-23T13:54:55Z

extensions/amp-wordpress-embed/OWNERS

+{
+  rules: [
+    {
+      owners: [{name: 'ampproject/wg-bento'}],


@westonruter do you have any suggestions?

caroqliu · 2021-06-23T14:06:27Z

extensions/amp-wordpress-embed/1.0/component.js

+          // Only follow a link message for the currently-active iframe if the link is for the same origin.
+          // This replicates a constraint in WordPress's wp.receiveEmbedMessage() function.
+          if (matchesMessagingOrigin(data.value)) {
+            window.top.location.href = data.value;


I would recommend getting a reference and setting win as a state variable so as to trigger callback redefinition:

const contentRef = useRef(null); const [win, setWin] = useState(null); const messageHandler = useCallback(() => { ... if (matchesMessagingOrigin(data.value) && win) { win.top.location.href = data.value; } }, [win]); useLayoutEffect(() => { setWin(contentRef.current?.ownerDocument?.defaultView); }, []);

Then give the IframeEmbed the contentRef={contentRef}

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

Co-authored-by: Weston Ruter <westonruter@google.com>

extensions/amp-wordpress-embed/amp-wordpress-embed.md

Co-authored-by: Weston Ruter <westonruter@google.com>

caroqliu · 2021-06-28T13:39:24Z

extensions/amp-wordpress-embed/1.0/component.js

+        title={title}
+        {...rest}
+      />
+      {children}


What is an example of children that this component should expect? Do we expect publishers to provide them? I am wondering if this can be removed in both layers (here, and props.children entry from BaseElement).

Note that AMP specific elements, such as placeholder, fallback, and overflow, are displayable regardless.

@caroqliu the children is required for the overflow button and I've added examples in both amp-wordpress-embed.md and in Basic.amp.js files.

Note that AMP specific elements, such as placeholder, fallback, and overflow, are displayable regardless.

I've tested in the storybook component and without the children the overflow button doesn't show up.

Ah, this should be removable if you please merge with main to get #35003 in your branch.

Thank you @caroqliu. I've removed the children from both the component and the BaseElement.

ediamin · 2021-07-08T12:22:10Z

@caroqliu I've addressed all of the PR reviews. Could you please tell me if I need to do further updates? Thank you very much.

caroqliu · 2021-07-08T13:17:46Z

@caroqliu I've addressed all of the PR reviews. Could you please tell me if I need to do further updates? Thank you very much.

Looks good. I've enabled auto merge for when CI finishes up. Thank you!

caroqliu · 2021-07-08T14:32:42Z

It looks like validator tests are failing due to an unrelated issue, could you please merge with latest main to correct the issue?

ediamin · 2021-07-08T14:42:25Z

could you please merge with latest main to correct the issue?

Merged and pushed.

The `amp-wordpress-embed` component was created (in #34948) without any non-Bento version, so the Bento experiment is currently blocking it from being usable on valid AMP pages. This removes the experiment.

ediamin added 8 commits June 22, 2021 17:23

Intiate amp-wordpress-embed bento component with cli command

3a0be40

Add component logic

21bc43d

Refactor component using IframeEmbed

713392e

Add unit tests for amp-wordpress-embed

6e4c000

Update documentation

1a33305

Update validator protoascii

ae0d0f5

Add more unit test

ac23cfc

Update amp-wordpress-embed.md

66c0b6e

amp-owners-bot bot requested a review from rbeckthomas June 22, 2021 11:33

caroqliu approved these changes Jun 22, 2021

View reviewed changes

honeybadgerdontcare requested changes Jun 22, 2021

View reviewed changes

extensions/amp-wordpress-embed/validator-amp-wordpress-embed.protoascii Outdated Show resolved Hide resolved

ediamin added 6 commits June 23, 2021 11:43

Update iframeURL function using useMemo

8a8c54e

Add more url validation

8875cc9

Remove container from supported layout list

bbf9220

Removed amp true attibute from test

040ace4

Update test

d1a18c8

Update tag name in protoascii

813d8e8

ediamin requested review from caroqliu and honeybadgerdontcare June 23, 2021 06:54

Lint code with amp lint command

a3bf0e2

kristoferbaxter reviewed Jun 23, 2021

View reviewed changes

caroqliu reviewed Jun 23, 2021

View reviewed changes

ediamin and others added 7 commits June 24, 2021 11:11

Update storybook example

41bce32

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

Use forceChangeHeight instead of attemptChangeHeight

b3d1fd4

Add westonruter as code owner

b4661c7

Update the go to link logic

f1d8c10

Use simple height calculation

945d225

Run prettify command on local

afb4fb4

Use useEffect instead of useLayoutEffect to avoid re-render

2e9f7c5

ediamin and others added 5 commits June 25, 2021 10:42

Make height attribute non-optional

7e39059

Co-authored-by: Weston Ruter <westonruter@google.com>

Update layout attribute in example

0db0851

Co-authored-by: Weston Ruter <westonruter@google.com>

Update amp-wordpress-embed.md

805fbd1

Co-authored-by: Weston Ruter <westonruter@google.com>

Use WordPress instead of Wordpress

dd4ce93

Add overflow button in example snippet

b9a12cf

ediamin requested a review from westonruter June 25, 2021 05:18

westonruter reviewed Jun 25, 2021

View reviewed changes

extensions/amp-wordpress-embed/amp-wordpress-embed.md Outdated Show resolved Hide resolved

Added common attributes in doc

901bf56

Co-authored-by: Weston Ruter <westonruter@google.com>

ediamin requested a review from westonruter June 26, 2021 09:36

westonruter approved these changes Jun 26, 2021

View reviewed changes

Add ediamin as code owner

6883738

caroqliu reviewed Jun 28, 2021

View reviewed changes

honeybadgerdontcare added the WG: caching label Jun 29, 2021

ediamin added 2 commits June 29, 2021 13:57

Merge branch 'main' into add/amp-wordpress-embed

b7fd65a

Remove children props from the component

96c78df

caroqliu enabled auto-merge (squash) July 8, 2021 13:17

Merge branch 'main' into add/amp-wordpress-embed

d77f9e5

caroqliu merged commit a0ba919 into ampproject:main Jul 8, 2021

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jul 13, 2021

This was referenced Jul 20, 2021

Update amphtml spec to ed0504b ampproject/amp-wp#6436

Merged

Improve handling of WordPress post embeds ampproject/amp-wp#809

Closed

westonruter added the WG: bento label Sep 5, 2021

westonruter mentioned this pull request Dec 16, 2021

✨ Remove Bento experiments from amp-wordpress-embed #37237

Merged

✨Bento amp-wordpress-embed #34948

✨Bento amp-wordpress-embed #34948

Conversation

ediamin commented Jun 22, 2021

amp-owners-bot bot commented Jun 22, 2021 • edited by ampprojectbot Loading

caroqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ediamin Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ediamin commented Jul 8, 2021

caroqliu commented Jul 8, 2021

caroqliu commented Jul 8, 2021

ediamin commented Jul 8, 2021

✨Bento `amp-wordpress-embed` #34948

✨Bento `amp-wordpress-embed` #34948

amp-owners-bot bot commented Jun 22, 2021 •

edited by ampprojectbot

Loading

ediamin Jun 24, 2021 •

edited

Loading