Skip to content

Commit cfe8f2f

Browse files
alanorozcoalanorozco
authored
✅ amp-video-iframe: Remove poster requirement (#33088)
Previously either `[poster]` or `> [placeholder]` were required.
1 parent c2f0984 commit cfe8f2f

7 files changed

Lines changed: 52 additions & 99 deletions

File tree

‎extensions/amp-video-iframe/0.1/amp-video-iframe.js‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,19 +27,18 @@ import {
2727
} from '../../../src/iframe-video';
2828
import {Services} from '../../../src/services';
2929
import {addParamsToUrl} from '../../../src/url';
30-
import {
31-
createElementWithAttributes,
32-
dispatchCustomEvent,
33-
getDataParamsFromAttributes,
34-
isFullscreenElement,
35-
removeElement,
36-
} from '../../../src/dom';
3730
import {dev, devAssert, user, userAssert} from '../../../src/log';
3831
import {dict} from '../../../src/utils/object';
3932
import {
4033
disableScrollingOnIframe,
4134
looksLikeTrackingIframe,
4235
} from '../../../src/iframe-helper';
36+
import {
37+
dispatchCustomEvent,
38+
getDataParamsFromAttributes,
39+
isFullscreenElement,
40+
removeElement,
41+
} from '../../../src/dom';
4342
import {getConsentDataToForward} from '../../../src/consent';
4443
import {getData, listen} from '../../../src/event-helper';
4544
import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl';
@@ -202,19 +201,15 @@ class AmpVideoIframe extends AMP.BaseElement {
202201
/** @override */
203202
createPlaceholderCallback() {
204203
const {element} = this;
205-
const src = addDataParamsToUrl(
206-
user().assertString(element.getAttribute('poster')),
207-
element
208-
);
209-
return createElementWithAttributes(
210-
devAssert(element.ownerDocument),
211-
'amp-img',
212-
dict({
213-
'src': src,
214-
'layout': 'fill',
215-
'placeholder': '',
216-
})
217-
);
204+
const poster = element.getAttribute('poster');
205+
if (!poster) {
206+
return null;
207+
}
208+
const img = new Image();
209+
img.src = addDataParamsToUrl(poster, element);
210+
img.setAttribute('placeholder', '');
211+
this.applyFillContent(img);
212+
return img;
218213
}
219214

220215
/** @override */

‎extensions/amp-video-iframe/0.1/test/test-amp-video-iframe.js‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import '../amp-video-iframe';
1818
import {Services} from '../../../../src/services';
1919
import {VideoEvents} from '../../../../src/video-interface';
2020
import {
21-
addAttributesToElement,
2221
createElementWithAttributes,
2322
whenUpgradedToCustomElement,
2423
} from '../../../../src/dom';
@@ -69,10 +68,12 @@ describes.realWin(
6968
};
7069

7170
function createVideoIframe(attrs = {}, opt_size) {
72-
const el = createElementWithAttributes(doc, 'amp-video-iframe', attrs);
73-
const {src = getIframeSrc(), poster = 'foo.png'} = attrs;
74-
addAttributesToElement(el, {src, poster});
75-
addAttributesToElement(el, layoutConfigAttrs(opt_size));
71+
const {src = getIframeSrc(), ...rest} = attrs;
72+
const el = createElementWithAttributes(doc, 'amp-video-iframe', {
73+
src,
74+
...rest,
75+
...layoutConfigAttrs(opt_size),
76+
});
7677
doc.body.appendChild(el);
7778
return el;
7879
}
@@ -200,24 +201,30 @@ describes.realWin(
200201
});
201202

202203
describe('#createPlaceholderCallback', () => {
204+
it('does not create placeholder without poster attribute', () => {
205+
const placeholder = createVideoIframe().createPlaceholder();
206+
expect(placeholder).to.be.null;
207+
});
208+
203209
it('creates an amp-img with the poster as src', () => {
204210
const poster = 'foo.bar';
205211
const placeholder = createVideoIframe({poster}).createPlaceholder();
206212
expect(placeholder).to.have.attribute('placeholder');
207-
expect(placeholder.tagName.toLowerCase()).to.equal('amp-img');
208-
expect(placeholder.getAttribute('layout')).to.equal('fill');
213+
expect(placeholder.tagName.toLowerCase()).to.equal('img');
214+
expect(placeholder).to.have.class('i-amphtml-fill-content');
209215
expect(placeholder.getAttribute('src')).to.equal(poster);
210216
});
211217

212218
it("uses data-param-* in the poster's src", () => {
213219
expect(
214220
createVideoIframe({
221+
poster: 'foo.png',
215222
'data-param-my-poster-param': 'my param',
216223
'data-param-another': 'value',
217224
})
218225
.createPlaceholder()
219226
.getAttribute('src')
220-
).to.match(/\?myPosterParam=my%20param&another=value$/);
227+
).to.match(/foo\.png\?myPosterParam=my%20param&another=value$/);
221228
});
222229
});
223230

‎extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.html‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,16 @@
3333
width="640"
3434
height="360"
3535
layout="responsive"
36-
src="https://foo.com"
37-
poster="images/kitten-playing.png">
36+
src="https://foo.com">
3837
</amp-video-iframe>
3938

4039
<!-- Valid -->
4140
<amp-video-iframe
4241
width="640"
4342
height="360"
4443
layout="responsive"
45-
src="https://foo.com">
46-
<div placeholder></div>
44+
src="https://foo.com"
45+
poster="images/kitten-playing.png">
4746
</amp-video-iframe>
4847

4948
<!-- Invalid: Incorrect attribute value for autoplay -->

‎extensions/amp-video-iframe/0.1/test/validator-amp-video-iframe.out‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,22 @@ FAIL
3434
| width="640"
3535
| height="360"
3636
| layout="responsive"
37-
| src="https://foo.com"
38-
| poster="images/kitten-playing.png">
37+
| src="https://foo.com">
3938
| </amp-video-iframe>
4039
|
4140
| <!-- Valid -->
4241
| <amp-video-iframe
4342
| width="640"
4443
| height="360"
4544
| layout="responsive"
46-
| src="https://foo.com">
47-
| <div placeholder></div>
45+
| src="https://foo.com"
46+
| poster="images/kitten-playing.png">
4847
| </amp-video-iframe>
4948
|
5049
| <!-- Invalid: Incorrect attribute value for autoplay -->
5150
| <amp-video-iframe
5251
>> ^~~~~~~~~
53-
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/)
52+
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/)
5453
| autoplay=true
5554
| layout=fill
5655
| width=300
@@ -82,7 +81,7 @@ amp-video-iframe/0.1/test/validator-amp-video-iframe.html:50:2 The attribute 'au
8281
| <!-- Invalid: Incorrect attribute value for rotate-to-fullscreen -->
8382
| <amp-video-iframe
8483
>> ^~~~~~~~~
85-
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/)
84+
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/)
8685
| rotate-to-fullscreen=true
8786
| layout=fill
8887
| width=300
@@ -91,4 +90,4 @@ amp-video-iframe/0.1/test/validator-amp-video-iframe.html:80:2 The attribute 'ro
9190
| poster="images/kitten-playing.png">
9291
| </amp-video-iframe>
9392
| </body>
94-
| </html>
93+
| </html>

‎extensions/amp-video-iframe/amp-video-iframe.md‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,8 @@ source documents to know that they are embedded in the AMP context. This fragmen
7575
not already have a fragment.</td>
7676
</tr>
7777
<tr>
78-
<td width="40%"><strong>poster (required)</strong></td>
79-
<td>Points to an image URL that will be displayed while the video loads.
80-
</td>
78+
<td width="40%"><strong>poster</strong></td>
79+
<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>
8180
</tr>
8281
<tr>
8382
<td width="40%"><strong>autoplay</strong></td>

‎extensions/amp-video-iframe/validator-amp-video-iframe.protoascii‎

Lines changed: 9 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ attr_lists: {
4848
name: "implements-rotate-to-fullscreen"
4949
value: ""
5050
}
51+
attrs: { name: "poster" }
5152
attrs: { name: "referrerpolicy" }
5253
attrs: {
5354
name: "rotate-to-fullscreen"
@@ -65,45 +66,15 @@ attr_lists: {
6566
attrs: { name: "[src]" }
6667
}
6768

68-
tags: { # <amp-video-iframe> with poster
69-
html_format: AMP
70-
tag_name: "AMP-VIDEO-IFRAME"
71-
spec_name: "AMP-VIDEO-IFRAME[poster]"
72-
requires_extension: "amp-video-iframe"
73-
attrs: {
74-
name: "poster"
75-
mandatory: true
76-
}
77-
attr_lists: "extended-amp-global"
78-
attr_lists: "amp-video-iframe-common"
79-
attr_lists: "lightboxable-elements"
80-
spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
81-
amp_layout: {
82-
supported_layouts: FILL
83-
supported_layouts: FIXED
84-
supported_layouts: FIXED_HEIGHT
85-
supported_layouts: FLEX_ITEM
86-
supported_layouts: INTRINSIC
87-
supported_layouts: NODISPLAY
88-
supported_layouts: RESPONSIVE
89-
}
90-
}
91-
92-
tags: { # <amp-video-iframe> with [placeholder]
69+
tags: { # <amp-video-iframe>
9370
html_format: AMP
9471
disabled_by: "transformed"
9572
tag_name: "AMP-VIDEO-IFRAME"
96-
spec_name: "AMP-VIDEO-IFRAME with [placeholder]"
9773
requires_extension: "amp-video-iframe"
9874
attr_lists: "extended-amp-global"
9975
attr_lists: "amp-video-iframe-common"
10076
attr_lists: "lightboxable-elements"
10177
spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
102-
reference_points: {
103-
tag_spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
104-
mandatory: true
105-
unique: true
106-
}
10778
amp_layout: {
10879
supported_layouts: FILL
10980
supported_layouts: FIXED
@@ -115,24 +86,17 @@ tags: { # <amp-video-iframe> with [placeholder]
11586
}
11687
}
11788

118-
tags: { # <amp-video-iframe> with [placeholder] (transformed)
89+
tags: { # <amp-video-iframe> (transformed)
11990
html_format: AMP
12091
enabled_by: "transformed"
12192
tag_name: "AMP-VIDEO-IFRAME"
122-
spec_name: "AMP-VIDEO-IFRAME with [placeholder] (transformed)"
93+
spec_name: "AMP-VIDEO-IFRAME (transformed)"
12394
requires_extension: "amp-video-iframe"
95+
requires: "amp-video-iframe > i-amphtml-sizer"
12496
attr_lists: "extended-amp-global"
12597
attr_lists: "amp-video-iframe-common"
12698
attr_lists: "lightboxable-elements"
12799
spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
128-
reference_points: {
129-
tag_spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
130-
mandatory: true
131-
unique: true
132-
}
133-
reference_points: {
134-
tag_spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER [style]"
135-
}
136100
amp_layout: {
137101
supported_layouts: FILL
138102
supported_layouts: FIXED
@@ -144,23 +108,14 @@ tags: { # <amp-video-iframe> with [placeholder] (transformed)
144108
}
145109
}
146110

147-
tags: {
148-
html_format: AMP
149-
tag_name: "$REFERENCE_POINT"
150-
spec_name: "AMP-VIDEO-IFRAME > [placeholder]"
151-
attrs: {
152-
name: "placeholder"
153-
mandatory: true
154-
}
155-
spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
156-
}
157-
158111
tags: {
159112
html_format: AMP
160113
enabled_by: "transformed"
161-
tag_name: "$REFERENCE_POINT"
162-
spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER [style]"
114+
tag_name: "I-AMPHTML-SIZER"
115+
spec_name: "AMP-VIDEO-IFRAME > I-AMPHTML-SIZER"
116+
mandatory_parent: "AMP-VIDEO-IFRAME"
163117
explicit_attrs_only: true
118+
satisfies: "amp-video-iframe > i-amphtml-sizer"
164119
attrs: {
165120
name: "style"
166121
mandatory: true
@@ -171,5 +126,4 @@ tags: {
171126
}
172127
css_declaration: { name: "padding-top" }
173128
}
174-
spec_url: "https://amp.dev/documentation/components/amp-video-iframe/"
175129
}

‎validator/testdata/transformed_feature_tests/i_amphtml_sizer_child.out‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ FAIL
6161
| <!-- Invalid: amp-video-iframe > div posing as i-amphtml-sizer -->
6262
| <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">
6363
| <div style="color:blue;"></div>
64-
>> ^~~~~~~~~
65-
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/)
6664
| <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>
6765
| </amp-video-iframe>
6866
| </body>
6967
| </html>
68+
>> ^~~~~~~~~
69+
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/)

0 commit comments

Comments
 (0)