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.

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.

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:

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.

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.

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

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

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.

@westonruter do you have any suggestions?

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}

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.