✅ amp-video-iframe: Remove poster requirement (#33088)

Previously either `[poster]` or `> [placeholder]` were required.
ampproject · Mar 9, 2021 · cfe8f2f · cfe8f2f
1 parent c2f0984
commit cfe8f2f
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 99 deletions.
diff --git a/extensions/amp-video-iframe/0.1/amp-video-iframe.js b/extensions/amp-video-iframe/0.1/amp-video-iframe.js
@@ -27,19 +27,18 @@ import {
 } from '../../../src/iframe-video';
 import {Services} from '../../../src/services';
 import {addParamsToUrl} from '../../../src/url';
-import {
-  createElementWithAttributes,
-  dispatchCustomEvent,
-  getDataParamsFromAttributes,
-  isFullscreenElement,
-  removeElement,
-} from '../../../src/dom';
 import {dev, devAssert, user, userAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {
   disableScrollingOnIframe,
   looksLikeTrackingIframe,
 } from '../../../src/iframe-helper';
+import {
+  dispatchCustomEvent,
+  getDataParamsFromAttributes,
+  isFullscreenElement,
+  removeElement,
+} from '../../../src/dom';
 import {getConsentDataToForward} from '../../../src/consent';
 import {getData, listen} from '../../../src/event-helper';
 import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl';
@@ -202,19 +201,15 @@ class AmpVideoIframe extends AMP.BaseElement {
   /** @override */
   createPlaceholderCallback() {
     const {element} = this;
-    const src = addDataParamsToUrl(
-      user().assertString(element.getAttribute('poster')),
-      element
-    );
-    return createElementWithAttributes(
-      devAssert(element.ownerDocument),
-      'amp-img',
-      dict({
-        'src': src,
-        'layout': 'fill',
-        'placeholder': '',
-      })
-    );
+    const poster = element.getAttribute('poster');
+    if (!poster) {
+      return null;
+    }
+    const img = new Image();
+    img.src = addDataParamsToUrl(poster, element);
+    img.setAttribute('placeholder', '');
+    this.applyFillContent(img);
+    return img;
   }
 
   /** @override */

diff --git a/extensions/amp-video-iframe/0.1/test/test-amp-video-iframe.js b/extensions/amp-video-iframe/0.1/test/test-amp-video-iframe.js
@@ -18,7 +18,6 @@ import '../amp-video-iframe';
 import {Services} from '../../../../src/services';
 import {VideoEvents} from '../../../../src/video-interface';
 import {
-  addAttributesToElement,
   createElementWithAttributes,
   whenUpgradedToCustomElement,
 } from '../../../../src/dom';
@@ -69,10 +68,12 @@ describes.realWin(
           };
 
     function createVideoIframe(attrs = {}, opt_size) {
-      const el = createElementWithAttributes(doc, 'amp-video-iframe', attrs);
-      const {src = getIframeSrc(), poster = 'foo.png'} = attrs;
-      addAttributesToElement(el, {src, poster});
-      addAttributesToElement(el, layoutConfigAttrs(opt_size));
+      const {src = getIframeSrc(), ...rest} = attrs;
+      const el = createElementWithAttributes(doc, 'amp-video-iframe', {
+        src,
+        ...rest,
+        ...layoutConfigAttrs(opt_size),
+      });
       doc.body.appendChild(el);
       return el;
     }
@@ -200,24 +201,30 @@ describes.realWin(
     });
 
     describe('#createPlaceholderCallback', () => {
+      it('does not create placeholder without poster attribute', () => {
+        const placeholder = createVideoIframe().createPlaceholder();
+        expect(placeholder).to.be.null;
+      });
+
       it('creates an amp-img with the poster as src', () => {
         const poster = 'foo.bar';
         const placeholder = createVideoIframe({poster}).createPlaceholder();
         expect(placeholder).to.have.attribute('placeholder');
-        expect(placeholder.tagName.toLowerCase()).to.equal('amp-img');
-        expect(placeholder.getAttribute('layout')).to.equal('fill');
+        expect(placeholder.tagName.toLowerCase()).to.equal('img');
+        expect(placeholder).to.have.class('i-amphtml-fill-content');
         expect(placeholder.getAttribute('src')).to.equal(poster);
       });
 
       it("uses data-param-* in the poster's src", () => {
         expect(
           createVideoIframe({
+            poster: 'foo.png',
             'data-param-my-poster-param': 'my param',
             'data-param-another': 'value',
           })
             .createPlaceholder()
             .getAttribute('src')
-        ).to.match(/\?myPosterParam=my%20param&another=value$/);
+        ).to.match(/foo\.png\?myPosterParam=my%20param&another=value$/);
       });
     });
 

diff --git a/extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.html b/extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.html
@@ -33,17 +33,16 @@
       width="640"
       height="360"
       layout="responsive"
-      src="https://foo.com"
-      poster="images/kitten-playing.png">
+      src="https://foo.com">
   </amp-video-iframe>
 
   <!-- Valid -->
   <amp-video-iframe
       width="640"
       height="360"
       layout="responsive"
-      src="https://foo.com">
-    <div placeholder></div>
+      src="https://foo.com"
+      poster="images/kitten-playing.png">
   </amp-video-iframe>
 
   <!-- Invalid: Incorrect attribute value for autoplay -->

diff --git a/extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.out b/extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.out
@@ -34,23 +34,22 @@ FAIL
 |        width="640"
 |        height="360"
 |        layout="responsive"
-|        src="https://foo.com"
-|        poster="images/kitten-playing.png">
+|        src="https://foo.com">
 |    </amp-video-iframe>
 |
 |    <!-- Valid -->
 |    <amp-video-iframe
 |        width="640"
 |        height="360"
 |        layout="responsive"
-|        src="https://foo.com">
-|      <div placeholder></div>
+|        src="https://foo.com"
+|        poster="images/kitten-playing.png">
 |    </amp-video-iframe>
 |
 |    <!-- Invalid: Incorrect attribute value for autoplay -->
 |    <amp-video-iframe
 >>   ^~~~~~~~~
-amp-video-iframe/0.1/test/validator-amp-video-iframe.html:50:2 The attribute 'autoplay' in tag 'amp-video-iframe' is set to the invalid value 'true'. (see https://amp.dev/documentation/components/amp-video-iframe/)
+amp-video-iframe/0.1/test/validator-amp-video-iframe.html:49:2 The attribute 'autoplay' in tag 'amp-video-iframe' is set to the invalid value 'true'. (see https://amp.dev/documentation/components/amp-video-iframe/)
 |        autoplay=true
 |        layout=fill
 |        width=300
@@ -82,7 +81,7 @@ amp-video-iframe/0.1/test/validator-amp-video-iframe.html:50:2 The attribute 'au
 |    <!-- Invalid: Incorrect attribute value for rotate-to-fullscreen -->
 |    <amp-video-iframe
 >>   ^~~~~~~~~
-amp-video-iframe/0.1/test/validator-amp-video-iframe.html:80:2 The attribute 'rotate-to-fullscreen' in tag 'amp-video-iframe' is set to the invalid value 'true'. (see https://amp.dev/documentation/components/amp-video-iframe/)
+amp-video-iframe/0.1/test/validator-amp-video-iframe.html:79:2 The attribute 'rotate-to-fullscreen' in tag 'amp-video-iframe' is set to the invalid value 'true'. (see https://amp.dev/documentation/components/amp-video-iframe/)
 |        rotate-to-fullscreen=true
 |        layout=fill
 |        width=300
@@ -91,4 +90,4 @@ amp-video-iframe/0.1/test/validator-amp-video-iframe.html:80:2 The attribute 'ro
 |        poster="images/kitten-playing.png">
 |    </amp-video-iframe>
 |  </body>
-|  </html>
+|  </html>
diff --git a/extensions/amp-video-iframe/amp-video-iframe.md b/extensions/amp-video-iframe/amp-video-iframe.md
@@ -75,9 +75,8 @@ source documents to know that they are embedded in the AMP context. This fragmen
 not already have a fragment.</td>
   </tr>
   <tr>
-    <td width="40%"><strong>poster (required)</strong></td>
-    <td>Points to an image URL that will be displayed while the video loads.
-</td>
+    <td width="40%"><strong>poster</strong></td>
+    <td>URL to an image displayed while the video loads. <strong>It's recommended to always set this attribute for best perceived performance.</strong></td>
   </tr>
   <tr>
     <td width="40%"><strong>autoplay</strong></td>

diff --git a/extensions/amp-video-iframe/validator-amp-video-iframe.protoascii b/extensions/amp-video-iframe/validator-amp-video-iframe.protoascii
@@ -48,6 +48,7 @@ attr_lists: {
     name: "implements-rotate-to-fullscreen"
     value: ""
   }
+  attrs: { name: "poster" }
   attrs: { name: "referrerpolicy" }
   attrs: {
     name: "rotate-to-fullscreen"
@@ -65,45 +66,15 @@ attr_lists: {
   attrs: { name: "[src]" }
 }
 
-tags: {  # <amp-video-iframe> with poster
-  html_format: AMP
-  tag_name: "AMP-VIDEO-IFRAME"
-  spec_name: "AMP-VIDEO-IFRAME[poster]"
-  requires_extension: "amp-video-iframe"
-  attrs: {
-    name: "poster"
-    mandatory: true
-  }
-  attr_lists: "extended-amp-global"
-  attr_lists: "amp-video-iframe-common"
-  attr_lists: "lightboxable-elements"
-  spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
-  amp_layout: {
-    supported_layouts: FILL
-    supported_layouts: FIXED
-    supported_layouts: FIXED_HEIGHT
-    supported_layouts: FLEX_ITEM
-    supported_layouts: INTRINSIC
-    supported_layouts: NODISPLAY
-    supported_layouts: RESPONSIVE
-  }
-}
-
-tags: {  # <amp-video-iframe> with [placeholder]
+tags: {  # <amp-video-iframe>
   html_format: AMP
   disabled_by: "transformed"
   tag_name: "AMP-VIDEO-IFRAME"
-  spec_name: "AMP-VIDEO-IFRAME with [placeholder]"
   requires_extension: "amp-video-iframe"
   attr_lists: "extended-amp-global"
   attr_lists: "amp-video-iframe-common"
   attr_lists: "lightboxable-elements"
   spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
-  reference_points: {
-    tag_spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
-    mandatory: true
-    unique: true
-  }
   amp_layout: {
     supported_layouts: FILL
     supported_layouts: FIXED
@@ -115,24 +86,17 @@ tags: {  # <amp-video-iframe> with [placeholder]
   }
 }
 
-tags: {  # <amp-video-iframe> with [placeholder] (transformed)
+tags: {  # <amp-video-iframe> (transformed)
   html_format: AMP
   enabled_by: "transformed"
   tag_name: "AMP-VIDEO-IFRAME"
-  spec_name: "AMP-VIDEO-IFRAME with [placeholder] (transformed)"
+  spec_name: "AMP-VIDEO-IFRAME (transformed)"
   requires_extension: "amp-video-iframe"
+  requires: "amp-video-iframe > i-amphtml-sizer"
   attr_lists: "extended-amp-global"
   attr_lists: "amp-video-iframe-common"
   attr_lists: "lightboxable-elements"
   spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
-  reference_points: {
-    tag_spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
-    mandatory: true
-    unique: true
-  }
-  reference_points: {
-    tag_spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER [style]"
-  }
   amp_layout: {
     supported_layouts: FILL
     supported_layouts: FIXED
@@ -144,23 +108,14 @@ tags: {  # <amp-video-iframe> with [placeholder] (transformed)
   }
 }
 
-tags: {
-  html_format: AMP
-  tag_name: "$REFERENCE_POINT"
-  spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
-  attrs: {
-    name: "placeholder"
-    mandatory: true
-  }
-  spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
-}
-
 tags: {
   html_format: AMP
   enabled_by: "transformed"
-  tag_name: "$REFERENCE_POINT"
-  spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER [style]"
+  tag_name: "I-AMPHTML-SIZER"
+  spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER"
+  mandatory_parent: "AMP-VIDEO-IFRAME"
   explicit_attrs_only: true
+  satisfies: "amp-video-iframe > i-amphtml-sizer"
   attrs: {
     name: "style"
     mandatory: true
@@ -171,5 +126,4 @@ tags: {
     }
     css_declaration: { name: "padding-top" }
   }
-  spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
 }
diff --git a/validator/testdata/transformed_feature_tests/i_amphtml_sizer_child.out b/validator/testdata/transformed_feature_tests/i_amphtml_sizer_child.out
@@ -61,9 +61,9 @@ FAIL
 |     <!-- Invalid: amp-video-iframe > div posing as i-amphtml-sizer -->
 |     <amp-video-iframe id="myVideo" src="/amp-video-iframe-videojs.html" width="720" height="405" layout="responsive" autoplay="" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive">
 |       <div style="color:blue;"></div>
->>      ^~~~~~~~~
-transformed_feature_tests/i_amphtml_sizer_child.html:62:5 The tag 'div', a child tag of 'AMP-VIDEO-IFRAME with [placeholder] (transformed)', does not satisfy one of the acceptable reference points: AMP-VIDEO-IFRAME > [placeholder], AMP-VIDEO-IFRAME > I-AMPHTML-SIZER [style]. (see https://amp.dev/documentation/components/amp-video-iframe/)
 |       <amp-img placeholder="" src="/img/amp-video-iframe-sample-placeholder.jpg" layout="fill" class="i-amphtml-layout-fill i-amphtml-layout-size-defined" i-amphtml-layout="fill"></amp-img>
 |     </amp-video-iframe>
 |  </body>
 |  </html>
+>>       ^~~~~~~~~
+transformed_feature_tests/i_amphtml_sizer_child.html:66:6 The tag 'amp-video-iframe > i-amphtml-sizer' is missing or incorrect, but required by 'amp-video-iframe'. (see https://amp.dev/documentation/components/amp-video-iframe/)