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

Bento components sporadically fail to render on Transformed (SSR'ed) AMP pages #35485

Open
westonruter opened this issue Jul 31, 2021 · 7 comments
Assignees
Labels
Stale Inactive for one year or more Type: Bug WG: bento

Comments

@westonruter
Copy link
Member

westonruter commented Jul 31, 2021

Description

I've come across a strange issue where Bento components sporadically fail to render when they appear on some transformed AMP pages (i.e. SSR'ed with AMP Optimizer). I've put together a test case to demonstrate: https://amp-bento-component-ssr-init-fail.glitch.me/

See the following video for the buggy behavior in the third iframe with the heading “With WP Admin Bar w/ SSR”, after I reload the page a couple times:

bento-components-sporadically-fail-to-initialize-with-ssr.mov

This SSR'ed AMP with the WordPress admin bar consistently fails to have the amp-youtube and amp-video Bento components render.

Pass Fail
image image

The difference between the two is the i-amphtml-sizer element. In the working version, it has the slot="i-amphtml-svc" attribute whereas in the failing version, this slot attribute is absent.

The markup pre-SSR:

<p>
	<amp-youtube data-videoid="mGENRKrdoGY" layout="responsive" width="480" height="270"></amp-youtube>
</p>

<p>
	<amp-video controls width="640" height="360" layout="intrinsic" poster="https://playground.amp.dev/static/inline-examples/images/kitten-playing.png" src="https://playground.amp.dev/static/inline-examples/videos/kitten-playing.mp4"></amp-video>
</p>

The markup after SSR:

<p>
	<amp-youtube data-videoid="mGENRKrdoGY" layout="responsive" width="480" height="270" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive">
		<i-amphtml-sizer style="display:block;padding-top:56.25%"></i-amphtml-sizer>
	</amp-youtube>
</p>

<p>
	<amp-video controls width="640" height="360" layout="intrinsic" poster="https://playground.amp.dev/static/inline-examples/images/kitten-playing.png" src="https://playground.amp.dev/static/inline-examples/videos/kitten-playing.mp4" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
		<i-amphtml-sizer class="i-amphtml-sizer">
			<img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9IjM2MCIgd2lkdGg9IjY0MCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB2ZXJzaW9uPSIxLjEiLz4=">
		</i-amphtml-sizer>
	</amp-video>
</p>

When the WordPress admin bar is present (in which case the SSR'ed Bento components often fail to render), the following changes are present:

  1. Additional stylesheets are added.
  2. Additional scripts are added.
  3. Additional HTML elements are added.

There seems like a race condition going on, and it may be that the additional stylesheets and scripts are making the race condition more likely to happen.

Reproduction Steps

Go to https://amp-bento-component-ssr-init-fail.glitch.me/ (and potentially reload the page) to see the third iframe have Bento AMP components that fail to load.

I can reproduce consistently in Chrome, Edge, and Safari. In Firefox I've been able to reproduce the issue less frequently, mainly after doing hard refreshes.

Relevant Logs

No response

Browser(s) Affected

Chrome, Firefox, Safari, Edge

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

2107170150000

@westonruter
Copy link
Member Author

cc @caroqliu

@jridgewell
Copy link
Contributor

Given many people will need to update their servers to use the new optimizer transforms, I think we should also solve this in Runtime.

The issue is the race between DOM parsing, and the connectedCallback firing for our CE. During connectedCallback, we call applyStaticLayout. applyStaticLayout will determine this is an SSR'd element, and try to get its sizer.

But, children are not guaranteed to be parsed when connectedCallback fires. We try to handle this in in the CE when you fetch the sizer by re-querying if it wasn't found. This method should set slot="i-amphtml-svc" when it finds one.

But that alone doesn't fix it, because the method isn't called before layoutCallback. And the element needs a size to receive layoutCallback. So before buildCallback, we should call getSizer().

@kristoferbaxter
Copy link
Contributor

What's the trade off in making client side apply changes for invalid server output?

Perhaps I am reading this incorrectly, but if the server was rendering as expected this issue wouldn't occur... correct?

This could be a "optimized" versus generic solution if the cost is high to include in the client.

@jridgewell
Copy link
Contributor

What's the trade off in making client side apply changes for invalid server output?

2 lines of code, it's extremely simple. Because of backreferences in compress, it might be 20 bytes. It might cause some CLS, but the failure mode is display nothing at all which I think is worse.

Perhaps I am reading this incorrectly, but if the server was rendering as expected this issue wouldn't occur... correct?

Correct.

@kristoferbaxter
Copy link
Contributor

Makes sense, thanks.

Can we document this behavior as well so implementors can avoid the shift via Bento Compiler?

@caroqliu
Copy link
Contributor

Can we document this behavior as well so implementors can avoid the shift via Bento Compiler?

@CrystalOnScript can you advise on where we should put this documentation? I'm not positive implementers are likely to go to the Bento guide for this type of information. Is there a relevant section under "Optimize and Measure" that we can add to?

@stale
Copy link

stale bot commented Oct 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more Type: Bug WG: bento
Projects
None yet
Development

No branches or pull requests

4 participants