Bento: components fail to hide placeholder content once loaded

[westonruter]

Description

When using amp-twitter 1.0 (Bento) with placeholder content as follows:

<amp-twitter width="600" height="480" layout="responsive" data-tweetid="1397995435386679302" class="twitter-tweet" data-width="550" data-dnt="true">
  <blockquote class="twitter-tweet" data-width="550" data-dnt="true" placeholder="">
    <p lang="en" dir="ltr">
      ⛺🔥
      Fun fact: cAMPfire stories are the best stories.<br>
      <a href="https://twitter.com/CrystalOnScript?ref_src=twsrc%5Etfw">@CrystalOnScript</a>,
      <a href="https://twitter.com/nainar92?ref_src=twsrc%5Etfw">@nainar92</a>, and Caroline Liu gather around to
      unbox the Bento developer preview, which allows you to use AMP ⚡
      components everywhere!<br> <br>
      📺
      Tune in → <a href="https://t.co/RViEFP4AR9">https://t.co/RViEFP4AR9</a> <a href="https://t.co/Ni8B5VNZVz">pic.twitter.com/Ni8B5VNZVz</a>
    </p>
    — AMP Project (@AMPhtml) <a href="https://twitter.com/AMPhtml/status/1397995435386679302?ref_src=twsrc%5Etfw">May
    27, 2021</a>
  </blockquote>
</amp-twitter>

The placeholder content fails to be hidden when the component initializes, causing overlapping text:

See example on playground.

Reproduction Steps

Add amp-twitter 1.0 with placeholder content.

Relevant Logs

No response

Browser(s) Affected

No response

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

2105150310000

[westonruter]

While #35788 addresses the issue for amp-twitter, it appears to have done so for that component exclusively. It turns out that other Bento components also have the issue with placeholder content not being hidden when the component loads. See amp-youtube for example on AMP Playground.

[caroqliu]

While #35788 addresses the issue for amp-twitter, it appears to have done so for that component exclusively. It turns out that other Bento components also have the issue with placeholder content not being hidden when the component loads. See amp-youtube for example on AMP Playground.

Good call. I would recommend providing roughly three callbacks:

• onLoading - to show the loader if available
• onLoad or onReady - to remove placeholder/loader
• onError - to show fallback or placeholder

We can use amp-twitter as an example for onLoad and amp-render as an example for onError and onLoading:

amphtml/extensions/amp-render/1.0/amp-render.js

Lines 278 to 280 in 1e13253

	'onLoading': () => {
	this.toggleLoading(true);
	},

amphtml/extensions/amp-render/1.0/amp-render.js

Lines 319 to 329 in 1e13253

	'onError': () => {
	this.toggleLoading(false);
	// If the content fails to load and there's a fallback element, display the fallback.
	// Otherwise, continue displaying the placeholder.
	if (this.getFallback()) {
	this.togglePlaceholder(false);
	this.toggleFallback(true);
	} else {
	this.togglePlaceholder(true);
	}
	},

[kvchari]

working on a fix for this here: #35821

[samouri]

@kvchari: now that #35821 has merged, can this be closed?

[kvchari]

@samouri yes this should be closed. Going forward, all bento components with dynamic content will need to invoke the callbacks provided in BaseElement (onLoading/onLoad/onError) to ensure each component supports placeholders properly