-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Prerender amp-video poster image. #25456
Conversation
I don't believe this is safe as-is, allowing prerender will cause the element to be built, which will trigger Perhaps we could check if the current document state is prerender in |
Great catch, thanks! |
const {element} = this; | ||
const sources = toArray(childElementsByTag(element, 'source')); | ||
sources.push(element); | ||
for (let i = 0; i < sources.length; i++) { | ||
if (this.isCachedByCDN_(sources[i])) { | ||
return true; | ||
return sources[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns an element, not a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and added unit tests for preconnectCallback
.
Don't we also need to check whether the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const {element} = this; | ||
const sources = toArray(childElementsByTag(element, 'source')); | ||
sources.push(element); | ||
for (let i = 0; i < sources.length; i++) { | ||
if (this.isCachedByCDN_(sources[i])) { | ||
return true; | ||
return sources[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and added unit tests for preconnectCallback
.
* Prerender amp-video poster image. * Preconnect to the first cached URL during prerendering. * More unit tests for preconnect.
amp-video
used to load the poster image in prerender (#1657 + #1718).When introducing prerendering for cached videos (#12193 + #17493), we broke the case for prerendering the poster when the video is not cached.
If the video has cached sources OR a poster image, it should allow prerendering.
It is safe to prerender a video with a poster image but no cached source, as the
amp-video.layoutCallback
will only propagate the cached source until the viewer is visible (source).Fixes #25446