Skip to content

Race condition in new loader placeholders #23964

@dreamofabear

Description

@dreamofabear

The new loader adds a default placeholder if the AMP element doesn't already have one:

maybeAddDefaultPlaceholder_() {
const hasPlaceholder = !!this.element_.getPlaceholder();
const hasPoster = this.element_.hasAttribute('poster');
if (hasPlaceholder || hasPoster) {
return;
}
// Is it whitelisted for default placeholder?
const tagName = this.element_.tagName.toUpperCase();
if (
DEFAULT_PLACEHOLDER_WHITELIST_NONE_VIDEO[tagName] || // static white list
isIframeVideoPlayerComponent(tagName) // regex for various video players
) {
const html = htmlFor(this.element_);
const defaultPlaceholder = html`
<div placeholder class="i-amphtml-default-placeholder"></div>
`;
this.element_.insertBefore(defaultPlaceholder, this.element_.lastChild);
}
}

AMP elements like amp-img hide the placeholder via togglePlaceholder(false) on layout:

amphtml/builtins/amp-img.js

Lines 285 to 296 in 9d90a6a

firstLayoutCompleted() {
const placeholder = this.getPlaceholder();
if (
placeholder &&
placeholder.classList.contains('i-amphtml-blurry-placeholder') &&
isExperimentOn(this.win, 'blurry-placeholder')
) {
setImportantStyles(placeholder, {'opacity': 0});
} else {
this.togglePlaceholder(false);
}
}

This means if an amp-img lays out before the new loader is built, then the added default placeholder will never be hidden.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions