🐛 Toggle placeholders/fallbacks properly on Bento components (#35821)

* create centralized hooks for onLoad/onLoading/onError in PreactBaseElement

* update instances where components used their own (similar) hooks

* toggle placeholders for bento components with dynamic content

* update storybook stories to demonstrate placeholders/fallbacks
* update closure typings
* update docs

docs/building-a-bento-amp-extension.md

@@ -25,6 +25,7 @@ Read this document to learn how to create a new Bento AMP component.
     -   [Actions and events](#actions-and-events)
     -   [PreactBaseElement callbacks](#preactbaseelement-callbacks)
     -   [Element and Component classes](#element-and-component-classes)
+    -   [Loading state](#loading-state)
     -   [Element styling](#element-styling)
     -   [Layouts supported in your element](#layouts-supported-in-your-element)
     -   [Register element with AMP](#register-element-with-amp)
@@ -324,6 +325,27 @@ You must document your element's actions and events in its own reference documen
 -   **Usage**: If your extension 'props' definition is not sufficient, or when registering AMP actions and events.
 -   **Example Usage**: `amp-base-carousel`, `amp-lightbox`
 
+#### handleOnLoading
+
+-   **Default**: Optional.
+-   **Override**: Sometimes
+-   **Usage**: Method to override to customize the display of the Placeholder/Fallback/Loader
+-   **Example Usage**: `amp-render`
+
+#### handleOnLoad
+
+-   **Default**: Optional.
+-   **Override**: Sometimes
+-   **Usage**: Method to override to customize the display of the Placeholder/Fallback/Loader
+-   **Example Usage**: `amp-render`, `amp-iframe`
+
+#### handleOnError
+
+-   **Default**: Optional.
+-   **Override**: Sometimes
+-   **Usage**: Method to override to customize the display of the Placeholder/Fallback/Loader
+-   **Example Usage**: `amp-render`
+
 #### mutationObserverCallback
 
 -   **Default**: Optional.
@@ -419,6 +441,22 @@ export function MyElement({propName1, propName2, ...rest}) {
 }
 ```
 
+### Loading state
+
+For components that may load asynchronously, be sure to support `onLoading`, `onLoad`, and `onError` callbacks as props on your Preact component to allow components to fail gracefully.
+
+By default, `PreactBaseElement` provides callbacks which do the following from the AMP layer:
+
+-   `onLoading` toggles on a loader
+-   `onLoad` toggles off any loaders, placeholders, or fallbacks
+-   `onError` toggles on the fallpack (or, if unavailable, the placeholder)
+
+Your Preact component **must** invoke the `onLoad()` callback once your component has loaded. This is especially important for components that display content dynamically, for example through an iframe or after fetching remote data, because they may take time to load which would affect perceived performance.
+
+Your Preact component _should_ invoke the `onLoading()` and `onError()` callbacks so document authors in any environment can implement graceful behavior when appropriate.
+
+Your AMP extension may customize this behavior by overriding the `handleOnLoading()` / `handleOnLoad()` / `handleOnError()` methods in your AMP extension's class.
+
 ### Element styling
 
 You can write a stylesheet to style your element to provide a minimal visual appeal. Your element structure should account for whether you want users (publishers and developers using your element) to customize the default styling you're providing and allow for easy CSS classes and/or well-structure DOM elements.

docs/building-a-bento-iframe-component.md

@@ -21,6 +21,7 @@
     -   [Use `ProxyIframeEmbed` directly](#use-proxyiframeembed-directly)
         -   [Resizing components in AMP](#resizing-components-in-amp)
         -   [Passing or overriding props](#passing-or-overriding-props)
+    -   [Placeholders](#placeholders)
 -   [Completing your extension](#completing-your-extension)
 -   [Example Pull Requests](#example-pull-requests)
 
@@ -306,6 +307,12 @@ function FantasticEmbedInternalWithRef(
 
 You may similarly choose to pass or override properties at the higher level, passed from `FantasticEmbed` into the `ProxyIframeEmbed` we instantiate. For a list of these properties [see `component.type.js`](../src/preact/component/component.type.js)
 
+### Placeholders
+
+All Bento components implemented using iframes should utilize placeholders to mitigate the risk of poor perceived performance. Placeholders, Fallbacks, and Loaders can be toggled on/off using the hooks provided to the Preact Component. See [here](https://github.com/ampproject/amphtml/blob/main/docs/building-a-bento-amp-extension.md#placeholders) for detailed instructions on how to toggle placeholders/fallbacks/loaders.
+
+It is encouraged to create a Storybook story that specifically demonstrates utilization of placeholders and fallbacks.
+
 ## Completing your extension
 
 Follow the [guide to Building a Bento AMP Component](./building-a-bento-amp-extension.md) for other instructions that you should complete, including:

docs/building-a-bento-video-player.md

@@ -23,6 +23,7 @@
         -   [`origin`](#origin)
         -   [Playback methods with `makeMethodMessage`](#playback-methods-with-makemethodmessage)
         -   [Handling events with `onMessage`](#handling-events-with-onmessage)
+        -   [Placeholders](#placeholders)
     -   [Use `VideoWrapper` directly](#use-videowrapper-directly)
         -   [Specifying `component`](#specifying-component)
         -   [Passing or overriding props](#passing-or-overriding-props)
@@ -348,6 +349,12 @@ function FantasticPlayerWithRef(
 
 Your iframe's interface to post messages is likely different, but your component should always dispatch [`HTMLMediaElement` events](https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement#events) upstream.
 
+#### Placeholders
+
+All Bento components implemented using iframes should utilize placeholders to mitigate the risk of poor perceived performance. Placeholders, Fallbacks, and Loaders can be toggled on/off using the hooks provided to the Preact Component. See [here](https://github.com/ampproject/amphtml/blob/main/docs/building-a-bento-amp-extension.md#placeholders) for detailed instructions on how to toggle placeholders/fallbacks/loaders.
+
+It is encouraged to create a Storybook story that specifically demonstrates utilization of placeholders and fallbacks.
+
 ### Use `VideoWrapper` directly
 
 > **If you need an iframe, you should ignore this section and use `VideoIframe` instead.** It requires less work and likely provides all you need. Read on if you're sure that you need to write to a host document directly to create `<video>` elements, or otherwise manage frames manually (like creating proxy frames).

extensions/amp-brightcove/1.0/component.js

@@ -5,6 +5,7 @@ import {VideoIframe} from '../../amp-video/1.0/video-iframe';
 import {mutedOrUnmutedEvent} from '../../../src/iframe-video';
 import {dispatchCustomEvent} from '#core/dom';
 import {BRIGHTCOVE_EVENTS, getBrightcoveIframeSrc} from '../brightcove-api';
+import {useValueRef} from '#preact/component';
 
 /** @const {string} */
 const DEFAULT = 'default';
@@ -37,10 +38,12 @@ export function BrightcoveWithRef(props, ref) {
     videoId,
     onPlayingState,
     urlParams,
+    onLoad,
     ...rest
   } = props;
 
   const playerStateRef = useRef({});
+  const onLoadRef = useValueRef(onLoad);
   const [muted, setMuted] = useState(mutedProp);
   const src = useMemo(
     () =>
@@ -62,6 +65,7 @@ export function BrightcoveWithRef(props, ref) {
       switch (eventType) {
         case 'ready':
           dispatchCustomEvent(currentTarget, 'canplay');
+          onLoadRef.current?.();
           break;
         case 'playing':
           onPlayingState?.(true);
@@ -96,7 +100,7 @@ export function BrightcoveWithRef(props, ref) {
         dispatchCustomEvent(currentTarget, mutedOrUnmutedEvent(isMuted));
       }
     },
-    [muted, onPlayingState]
+    [muted, onPlayingState, onLoadRef]
   );
 
   // Check for valid props

extensions/amp-brightcove/1.0/component.type.js

@@ -9,6 +9,7 @@ var BrightcoveDef = {};
  *   autoplay: (boolean|undefined),
  *   embed: (string|undefined),
  *   muted: (boolean|undefined),
+ *   onLoad: (function():undefined|undefined),
  *   player: (string|undefined),
  *   playlistId: (string|undefined),
  *   videoId: (string|undefined),

extensions/amp-brightcove/1.0/storybook/Basic.amp.js

@@ -1,6 +1,6 @@
 import * as Preact from '#preact';
 import {withAmp} from '@ampproject/storybook-addon';
-import {withKnobs} from '@storybook/addon-knobs';
+import {text, withKnobs} from '@storybook/addon-knobs';
 
 export default {
   title: 'amp-brightcove-1_0',
@@ -27,6 +27,35 @@ export const Default = () => {
   );
 };
 
+export const WithPlaceholderAndFallback = () => {
+  const videoid = text('videoid', 'ref:amp-docs-sample');
+  const playerid = text('playerid', 'SyIOV8yWM');
+  const account = text('account', '1290862519001');
+  const height = text('height', '270');
+  const width = text('width', '480');
+
+  return (
+    <amp-brightcove
+      id="myPlayer"
+      data-referrer="EXTERNAL_REFERRER"
+      data-account={account}
+      data-video-id={videoid}
+      data-player-id={playerid}
+      layout="responsive"
+      width={width}
+      height={height}
+    >
+      <div placeholder style="background:red">
+        Placeholder. Loading content...
+      </div>
+
+      <div fallback style="background:blue">
+        Fallback. Could not load content...
+      </div>
+    </amp-brightcove>
+  );
+};
+
 export const Actions = () => {
   return (
     <>

extensions/amp-embedly-card/1.0/component.js

@@ -3,6 +3,7 @@ import {MessageType, deserializeMessage} from '#core/3p-frame-messaging';
 import * as Preact from '#preact';
 import {useCallback, useContext, useState} from '#preact';
 import {forwardRef} from '#preact/compat';
+import {useValueRef} from '#preact/component';
 import {ProxyIframeEmbed} from '#preact/component/3p-frame';
 
 import {EmbedlyContext} from './embedly-context';
@@ -22,10 +23,11 @@ const FULL_HEIGHT = '100%';
  * @return {PreactDef.Renderable}
  */
 export function EmbedlyCardWithRef(
-  {requestResize, style, title, url, ...rest},
+  {onLoad, requestResize, style, title, url, ...rest},
   ref
 ) {
   const [height, setHeight] = useState(null);
+  const onLoadRef = useValueRef(onLoad);
   const messageHandler = useCallback(
     (event) => {
       const data = deserializeMessage(event.data);
@@ -37,9 +39,11 @@ export function EmbedlyCardWithRef(
         } else {
           setHeight(height);
         }
+
+        onLoadRef.current?.();
       }
     },
-    [requestResize]
+    [requestResize, onLoadRef]
   );
 
   const {apiKey} = useContext(EmbedlyContext);

extensions/amp-embedly-card/1.0/component.type.js

@@ -5,6 +5,7 @@ var EmbedlyCardDef = {};
 
 /**
  * @typedef {{
+ *   onLoad: (function():undefined|undefined),
  *   requestResize: (function(number):*|undefined),
  *   title: (string|undefined),
  *   url: (string),

extensions/amp-embedly-card/1.0/storybook/Basic.amp.js

@@ -42,3 +42,26 @@ export const WithAPIKey = () => {
     </>
   );
 };
+
+export const WithPlaceholderAndFallback = () => {
+  const apiKey = text('Embedly API Key', 'valid-api-key');
+  return (
+    <>
+      <amp-embedly-key layout="nodisplay" value={apiKey}></amp-embedly-key>
+      <amp-embedly-card
+        data-url="https://www.youtube.com/watch?v=lBTCB7yLs8Y"
+        layout="responsive"
+        width="300"
+        height="200"
+      >
+        <div placeholder style="background:red">
+          Placeholder. Loading content...
+        </div>
+
+        <div fallback style="background:blue">
+          Fallback. Could not load content...
+        </div>
+      </amp-embedly-card>
+    </>
+  );
+};

extensions/amp-facebook/1.0/amp-facebook.js

@@ -40,7 +40,6 @@ class AmpFacebook extends BaseElement {
   /** @override */
   init() {
     return dict({
-      'onReady': () => this.togglePlaceholder(false),
       'requestResize': (height) => this.attemptChangeHeight(height),
     });
   }

extensions/amp-facebook/1.0/component.js

@@ -5,6 +5,7 @@ import {MessageType, deserializeMessage} from '#core/3p-frame-messaging';
 import {forwardRef} from '#preact/compat';
 import {tryParseJson} from '#core/types/object/json';
 import {useCallback, useLayoutEffect, useMemo, useState} from '#preact';
+import {useValueRef} from '#preact/component';
 
 /** @const {string} */
 const TYPE = 'facebook';
@@ -30,7 +31,7 @@ function FacebookWithRef(
     layout,
     locale: localeProp,
     numPosts,
-    onReady,
+    onLoad,
     orderBy,
     refLabel,
     requestResize,
@@ -47,11 +48,12 @@ function FacebookWithRef(
   ref
 ) {
   const [height, setHeight] = useState(null);
+  const onLoadRef = useValueRef(onLoad);
   const messageHandler = useCallback(
     (event) => {
       const data = tryParseJson(event.data) ?? deserializeMessage(event.data);
       if (data['action'] == 'ready') {
-        onReady?.();
+        onLoadRef.current?.();
       }
       if (data['type'] == MessageType.EMBED_SIZE) {
         const height = data['height'];
@@ -63,7 +65,7 @@ function FacebookWithRef(
         }
       }
     },
-    [requestResize, onReady]
+    [requestResize, onLoadRef]
   );
 
   const [locale, setLocale] = useState(localeProp);

extensions/amp-facebook/1.0/component.type.js

@@ -15,6 +15,7 @@ var FacebookDef = {};
  *   locale: (string|undefined),
  *   layout: (string|undefined),
  *   numPosts: (number|undefined),
+ *   onLoad: (function():undefined|undefined),
  *   orderBy: (string|undefined),
  *   onReadyState: (function(string, *=)|undefined),
  *   refLabel: (string|undefined),

extensions/amp-facebook/1.0/storybook/Basic.amp.js

@@ -48,7 +48,13 @@ export const Default = () => {
       height="200"
       layout="responsive"
     >
-      <div placeholder>Loading...</div>
+      <div placeholder style="background:red">
+        Placeholder. Loading content...
+      </div>
+
+      <div fallback style="background:blue">
+        Fallback. Could not load content...
+      </div>
     </amp-facebook>
   );
 };

extensions/amp-iframe/1.0/amp-iframe.js

@@ -26,8 +26,9 @@ class AmpIframe extends BaseElement {
    * Callback for onload event of iframe. Checks if the component has a placeholder
    * element and hides it if it does. Also checks the position of the iframe on the
    * document.
+   * @override
    */
-  handleOnLoad_() {
+  handleOnLoad() {
     if (this.getPlaceholder()) {
       this.togglePlaceholder(false);
       return;
@@ -81,9 +82,6 @@ class AmpIframe extends BaseElement {
   /** @override */
   init() {
     return dict({
-      'onLoad': () => {
-        this.handleOnLoad_();
-      },
       'requestResize': (height, width) => {
         return this.updateSize_(height, width);
       },

extensions/amp-iframe/1.0/storybook/Basic.amp.js

@@ -1,6 +1,6 @@
 import * as Preact from '#preact';
 import {withAmp} from '@ampproject/storybook-addon';
-import {withKnobs} from '@storybook/addon-knobs';
+import {text, withKnobs} from '@storybook/addon-knobs';
 
 export default {
   title: 'amp-iframe-1_0',
@@ -25,9 +25,11 @@ export const WithSrc = () => {
 WithSrc.storyName = 'amp-iframe with src attribute';
 
 export const WithPlaceholder = () => {
+  const src = text('src', `https://www.wikipedia.org/`);
   return (
-    <amp-iframe width="800" height="600" src="https://www.wikipedia.org/">
-      <h1>Placeholder</h1>
+    <amp-iframe width="800" height="600" src={src}>
+      <h1 placeholder>Placeholder</h1>
+      <h1 fallback>Fallback</h1>
     </amp-iframe>
   );
 };