Bento: iframe components attemptChangeHeight

[caroqliu]

attemptChangeHeight is preferred to forceChangeHeight for increased CLS compliance as the default. This PR also includes documentation regarding resize recommendations for publishers. Related to #34969

cc @westonruter

[westonruter]

As per #34948 (comment):

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.

[westonruter]

I just encountered an issue with requiring static sizing up-front for embeds. With Twitter, for example, tweets will have different heights depending on the viewport. On mobile, a tweet may be 500px high but on desktop it may be 630px high. If this tweet is in the first viewport where automatic height changes will not be allowed, the only way I can think of this can be accommodated is to require authors to add multiple tweet embeds for each breakpoint, for example:

<amp-twitter class="mobile" media="(max-width: 360px)" height="500" layout="fixed-height" width="auto" data-tweetid="1405578489554743296"></amp-twitter>
<amp-twitter class="desktop" media="(min-width: 361px)" height="630" layout="fixed-height" width="auto" data-tweetid="1405578489554743296"></amp-twitter>

This seems like a heavy lift for developers and a poor DX. For more, see ampproject/amp-wp#4729 (comment).

[caroqliu]

I just encountered an issue with requiring static sizing up-front for embeds. With Twitter, for example, tweets will have different heights depending on the viewport. On mobile, a tweet may be 500px high but on desktop it may be 630px high. If this tweet is in the first viewport where automatic height changes will not be allowed, the only way I can think of this can be accommodated is to require authors to add multiple tweet embeds for each breakpoint, for example:

<amp-twitter class="mobile" media="(max-width: 360px)" height="500" layout="fixed-height" width="auto" data-tweetid="1405578489554743296"></amp-twitter>
<amp-twitter class="desktop" media="(min-width: 361px)" height="630" layout="fixed-height" width="auto" data-tweetid="1405578489554743296"></amp-twitter>

This seems like a heavy lift for developers and a poor DX. For more, see ampproject/amp-wp#4729 (comment).

@westonruter I will exclude amp-twitter from this PR to simplify things for now, but to clarify, is the point that the above is such a poor DX that it is not worth it to stabilize the component for end users? Wouldn't the above markup be ideal even without this change to remove existing unnecessary layout shifting behavior?

[westonruter]

is the point that the above is such a poor DX that it is not worth it to stabilize the component for end users? Wouldn't the above markup be ideal even without this change to remove existing unnecessary layout shifting behavior?

I just wanted to call out that for some embeds it may not be practical to obtain static dimensions. For these embed providers, AMP may need to be pragmatic to allow for layout shifting but then prioritize working with providers to provide embeds that do not require layout shifting.

The ideal would be for Twitter to serve back an SSR'ed embed so that we could use layout=container and wouldn't have to provide dimensions.

It's not an issue limited to amp-twitter, either. The amp-facebook embed also has device-specific dimensions that require layout shifting. It's not sufficient to rely on responsive layout with a fixed aspect-ratio either. For example:

<amp-facebook width="552" height="310"
    layout="responsive"
    data-href="https://www.facebook.com/ParksCanada/posts/1712989015384373">
</amp-facebook>

--	Mobile	Desktop
Screenshot
Dimensions	245x280	417x361
Aspect ratio	0.875	0.865

[caroqliu]

@westonruter Thank you for clarifying that some providers may have different or no solutions for resolving dimensions predictably and statically, and that it can be burdensome for developers to meet these needs without clear support or guidance. Given the examples for amp-twitter and amp-facebook, would it be reasonable to merge this PR in its original state (modifications to amp-facebook, amp-instagram, and amp-twitter) with an additional change:

• When attemptChangeHeight is rejected, check also for an overflow element and display a warn message if one is not available. This allows developers a clear path to make content available on user interaction without the burden of navigating provider-specific sizing behavior.

And these follow up steps:

• Update documentation where applicable with examples like you show with using media to display across the breakpoint.
• Contribute the same recommendations to the Page Experience tool on amp.dev to surface if the relevant components and 1.0 versions are used.

[westonruter]

@caroqliu yeah, merging with these additional changes sounds good to me.

[alanorozco]

For auto-envelope.

Suggested change

	if (!this.getOverflowElement()) {
	if (this.getOverflowElement && !this.getOverflowElement()) {

[caroqliu]

Good catch, thank you!

[alanorozco]

As you mentioned offline, we might want to use console.warn instead.

[westonruter]

I've done some more experimentation with obtaining sizes of an embed up-front and then serving multiple amp-twitter with media queries so that the best-sized element is shown when the page initially renders: AMP Playground Example. There's a lot of duplicate code and there's no way an author would be able to do that, so it would be something in the Optimizer realm.

It would be better if amp-twitter here could be given layout=fill and then all of the media query sizing could be done in CSS rather than having to duplicate the amp-twitter elements. But this doesn't work currently as the overflow button wouldn't work.