Skip to content
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

⏪ Revert "♻️ Support img element (#34028)" #34589

Merged
merged 1 commit into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions build-system/test-configs/forbidden-terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -870,8 +870,6 @@ const forbiddenTermsSrcInclusive = {
'extensions/amp-analytics/0.1/transport.js',
'extensions/amp-web-push/0.1/iframehost.js',
'extensions/amp-recaptcha-input/0.1/amp-recaptcha-service.js',
'extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js',
'extensions/amp-image-slider/0.1/amp-image-slider.js',
],
},
'\\.getTime\\(\\)': {
Expand Down
6 changes: 2 additions & 4 deletions builtins/amp-img/amp-img.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {Services} from '../../src/services';
import {dev} from '../../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../../src/utils/img';
import {listen} from '../../src/event-helper';
import {propagateAttributes} from '../../src/core/dom/propagate-attributes';
import {propagateObjectFitStyles, setImportantStyles} from '../../src/style';
import {registerElement} from '../../src/service/custom-element-registry';
import {removeElement, scopedQuerySelector} from '../../src/dom';
Expand Down Expand Up @@ -131,9 +130,8 @@ export class AmpImg extends BaseElement {
this.element
);
}
propagateAttributes(
this.propagateAttributes(
attrs,
this.element,
this.img_,
/* opt_removeMissingAttrs */ true
);
Expand Down Expand Up @@ -218,7 +216,7 @@ export class AmpImg extends BaseElement {

// It is important to call this before setting `srcset` attribute.
this.maybeGenerateSizes_(/* sync setAttribute */ true);
propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, this.img_);
this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
this.propagateDataset(this.img_);
if (!IS_ESM) {
guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
Expand Down
2 changes: 0 additions & 2 deletions examples/amp-lightbox.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ <h2>Scrollable Lightbox</h2>
Lorem ipsum dolor sit amet, has nisl nihil convenire et, vim at aeque inermis reprehendunt. Propriae tincidunt id nec, elit nusquam te mea, ius noster platonem in. Mea an idque minim, sit sale deleniti apeirian et. Omnium legendos tractatos cu mea. Vix in stet dolorem accusamus. Iisque rationibus consetetur in cum, quo unum nulla legere ut. Simul numquam saperet no sit.
</p>
<p>
<!-- native image element example, it's not valid yet, but supported. -->
<img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" width=300 height=200 loading="lazy" />
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" width=300 height=200></amp-img>
</p>
<p class="lightbox-text">
Expand Down
7 changes: 0 additions & 7 deletions examples/image-lightbox.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
<!-- Test for native image element, it's not valid yet, but supported. -->
<img on="tap:ampimagelightbox"
role="button"
src="img/bigbuckbunny.jpg"
width="500"
height="500"
loading="lazy" />
<!--
- Test layout: intrinsic, fill, fixed, fixed-height, responsive
-->
Expand Down
6 changes: 2 additions & 4 deletions extensions/amp-anim/0.1/amp-anim.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
observeWithSharedInOb,
unobserveWithSharedInOb,
} from '../../../src/viewport-observer';
import {propagateAttributes} from '../../../src/core/dom/propagate-attributes';
import {propagateObjectFitStyles} from '../../../src/style';

const TAG = 'amp-anim';
Expand Down Expand Up @@ -56,7 +55,7 @@ export class AmpAnim extends AMP.BaseElement {
buildCallback() {
this.img_ = new Image();
this.img_.setAttribute('decoding', 'async');
propagateAttributes(BUILD_ATTRIBUTES, this.element, this.img_);
this.propagateAttributes(BUILD_ATTRIBUTES, this.img_);
this.applyFillContent(this.img_, true);
propagateObjectFitStyles(this.element, this.img_);

Expand Down Expand Up @@ -89,9 +88,8 @@ export class AmpAnim extends AMP.BaseElement {
const img = dev().assertElement(this.img_);
// Remove missing attributes to remove the placeholder srcset if none is
// specified on the element.
propagateAttributes(
this.propagateAttributes(
LAYOUT_ATTRIBUTES,
this.element,
img,
/* opt_removeMissingAttrs */ true
);
Expand Down
10 changes: 2 additions & 8 deletions extensions/amp-audio/0.1/amp-audio.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {closestAncestorElementBySelector} from '../../../src/dom';
import {dev, user} from '../../../src/log';
import {getMode} from '../../../src/mode';
import {listen} from '../../../src/event-helper';
import {propagateAttributes} from '../../../src/core/dom/propagate-attributes';
import {setIsMediaComponent} from '../../../src/video-interface';
import {triggerAnalyticsEvent} from '../../../src/analytics';

Expand Down Expand Up @@ -86,11 +85,7 @@ export class AmpAudio extends AMP.BaseElement {
if (src !== undefined) {
assertHttpsUrl(src, this.element);
}
propagateAttributes(
['src', 'loop', 'controlsList'],
this.element,
this.audio_
);
this.propagateAttributes(['src', 'loop', 'controlsList'], this.audio_);
}

const artist = mutations['artist'];
Expand Down Expand Up @@ -124,7 +119,7 @@ export class AmpAudio extends AMP.BaseElement {
if (src) {
assertHttpsUrl(src, this.element);
}
propagateAttributes(
this.propagateAttributes(
[
'src',
'preload',
Expand All @@ -136,7 +131,6 @@ export class AmpAudio extends AMP.BaseElement {
'aria-labelledby',
'controlsList',
],
this.element,
audio
);

Expand Down
64 changes: 27 additions & 37 deletions extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
whenUpgradedToCustomElement,
} from '../../../src/dom';
import {dev} from '../../../src/log';
import {loadPromise} from '../../../src/event-helper';
import {measureIntersectionNoRoot} from '../../../src/utils/intersection-no-root';
import {toArray} from '../../../src/core/types/array';
import {tryParseJson} from '../../../src/core/types/object/json';
Expand Down Expand Up @@ -115,23 +114,25 @@ const META_OG_TYPE = 'meta[property="og:type"]';

const NOOP = () => {};

/**
* For better minification.
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @return {!Document|!ShadowRoot}
*/
const getRootNode = (ampdoc) => ampdoc.getRootNode();

/** @visibleForTesting */
export class Criteria {
/**
* @param {!Element} element
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {number} renderWidth
* @param {number} renderHeight
* @return {boolean}
*/
static meetsAll(element, ampdoc, renderWidth, renderHeight) {
static meetsAll(element, renderWidth, renderHeight) {
return (
Criteria.meetsSizingCriteria(
element,
ampdoc,
renderWidth,
renderHeight
) && Criteria.meetsTreeShapeCriteria(element)
Criteria.meetsSizingCriteria(element, renderWidth, renderHeight) &&
Criteria.meetsTreeShapeCriteria(element)
);
}

Expand All @@ -154,17 +155,16 @@ export class Criteria {

/**
* @param {!Element} element
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {number} renderWidth
* @param {number} renderHeight
* @return {boolean}
*/
static meetsSizingCriteria(element, ampdoc, renderWidth, renderHeight) {
static meetsSizingCriteria(element, renderWidth, renderHeight) {
const {naturalHeight, naturalWidth} = getMaxNaturalDimensions(
dev().assertElement(element.querySelector('img') || element)
dev().assertElement(element.querySelector('img'))
);

const viewport = Services.viewportForDoc(ampdoc);
const viewport = Services.viewportForDoc(element);
const {height: vh, width: vw} = viewport.getSize();

return meetsSizingCriteria(
Expand Down Expand Up @@ -273,26 +273,18 @@ function markAsVisited(candidate) {
}

/**
* @param {!Array<string>} tagNames
* @param {string} tagName
* @return {string}
*/
function candidateSelector(tagNames) {
return tagNames
.map(
(tagName) =>
`${tagName}:not([${LIGHTBOXABLE_ATTR}]):not([${VISITED_ATTR}])`
)
.join(',');
function candidateSelector(tagName) {
return `${tagName}:not([${LIGHTBOXABLE_ATTR}]):not([${VISITED_ATTR}])`;
}

/**
* @param {!Element} element
* @return {!Promise}
*/
function whenLoaded(element) {
if (element.tagName === 'IMG') {
return loadPromise(element);
}
return whenUpgradedToCustomElement(element).then((element) =>
element.signals().whenSignal(CommonSignals.LOAD_END)
);
Expand All @@ -306,7 +298,7 @@ export class Scanner {
* @return {!Array<!Element>}
*/
static getCandidates(root) {
const selector = candidateSelector(['amp-img', 'img']);
const selector = candidateSelector('amp-img');
const candidates = toArray(root.querySelectorAll(selector));
// TODO(alanorozco): DOM mutations should be wrapped in mutate contexts.
// Alternatively, use in-memory "visited" marker instead of attribute.
Expand All @@ -326,7 +318,7 @@ export class DocMetaAnnotations {
* @return {string|undefined}
*/
static getOgType(ampdoc) {
const tag = ampdoc.getRootNode().querySelector(META_OG_TYPE);
const tag = getRootNode(ampdoc).querySelector(META_OG_TYPE);
if (tag) {
return tag.getAttribute('content');
}
Expand All @@ -347,7 +339,7 @@ export class DocMetaAnnotations {
* @return {!Array<string>}
*/
static getAllLdJsonTypes(ampdoc) {
return toArray(ampdoc.getRootNode().querySelectorAll(SCRIPT_LD_JSON))
return toArray(getRootNode(ampdoc).querySelectorAll(SCRIPT_LD_JSON))
.map((el) => {
const {textContent} = el;
return (tryParseJson(textContent) || {})['@type'];
Expand Down Expand Up @@ -377,8 +369,10 @@ export class DocMetaAnnotations {
function usesLightboxExplicitly(ampdoc) {
// TODO(alanorozco): Backport into Extensions service.
const requiredExtensionSelector = `script[custom-element="${REQUIRED_EXTENSION}"]`;

const lightboxedElementsSelector = `[${LIGHTBOXABLE_ATTR}]:not([${VISITED_ATTR}])`;
const exists = (selector) => !!ampdoc.getRootNode().querySelector(selector);

const exists = (selector) => !!getRootNode(ampdoc).querySelector(selector);

return (
exists(requiredExtensionSelector) && exists(lightboxedElementsSelector)
Expand Down Expand Up @@ -446,17 +440,13 @@ export function runCandidates(ampdoc, candidates) {
whenLoaded(candidate).then(() => {
return measureIntersectionNoRoot(candidate).then(
({boundingClientRect}) => {
if (
!candidate.tagName === 'IMG' &&
!candidate.signals().get(CommonSignals.LOAD_END)
) {
// <amp-img> will change the img's src inline data on unlayout and
// remove it from DOM.
// <amp-img> will change the img's src inline data on unlayout and
// remove it from DOM.
if (!candidate.signals().get(CommonSignals.LOAD_END)) {
return;
}

const {height, width} = boundingClientRect;
if (!Criteria.meetsAll(candidate, ampdoc, width, height)) {
if (!Criteria.meetsAll(candidate, width, height)) {
return;
}
dev().info(TAG, 'apply', candidate);
Expand Down Expand Up @@ -485,7 +475,7 @@ export function scan(ampdoc, opt_root) {
AMP.extension(TAG, '0.1', (AMP) => {
const {ampdoc} = AMP;
ampdoc.whenReady().then(() => {
ampdoc.getRootNode().addEventListener(AmpEvents.DOM_UPDATE, (e) => {
getRootNode(ampdoc).addEventListener(AmpEvents.DOM_UPDATE, (e) => {
const {target} = e;
scan(ampdoc, dev().assertElement(target));
});
Expand Down
61 changes: 14 additions & 47 deletions extensions/amp-auto-lightbox/0.1/test/test-amp-auto-lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,62 +140,29 @@ describes.realWin(

it(`${accepts ? 'accepts' : 'rejects'} ${accepts || rejects}`, () => {
[
{
markup: html`
<amp-img src="asada.png" layout="flex-item"></amp-img>
`,
tagName: 'AMP-IMG',
},
{
markup: html`
<div>
<amp-img src="adobada.png" layout="flex-item"></amp-img>
</div>
`,
tagName: 'AMP-IMG',
},
{
markup: html`
<div>
<div>
<amp-img src="carnitas.png" layout="flex-item"></amp-img>
</div>
</div>
`,
tagName: 'AMP-IMG',
},
{
markup: html` <img src="asada.png" layout="flex-item" /> `,
tagName: 'IMG',
},
{
markup: html`
<div>
<img src="adobada.png" layout="flex-item" />
</div>
`,
tagName: 'IMG',
},
{
markup: html`
html` <amp-img src="asada.png" layout="flex-item"></amp-img> `,
html`
<div>
<amp-img src="adobada.png" layout="flex-item"></amp-img>
</div>
`,
html`
<div>
<div>
<div>
<img src="carnitas.png" layout="flex-item" />
</div>
<amp-img src="carnitas.png" layout="flex-item"></amp-img>
</div>
`,
tagName: 'IMG',
},
</div>
`,
].forEach((unwrapped) => {
maybeMutate(unwrapped.markup);
maybeMutate(unwrapped);

const scenario = maybeWrap(unwrapped.markup);
const scenario = maybeWrap(unwrapped);
const candidate = firstElementLeaf(scenario);

env.win.document.body.appendChild(scenario);

expect(candidate).to.be.ok;
expect(candidate.tagName).to.equal(unwrapped.tagName);
expect(candidate.tagName).to.equal('AMP-IMG');

expect(
Criteria.meetsTreeShapeCriteria(candidate),
Expand Down
3 changes: 1 addition & 2 deletions extensions/amp-brid-player/0.1/amp-brid-player.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import {getData, listen} from '../../../src/event-helper';
import {htmlFor} from '../../../src/static-template';
import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl';
import {isLayoutSizeDefined} from '../../../src/layout';
import {propagateAttributes} from '../../../src/core/dom/propagate-attributes';

const TAG = 'amp-brid-player';

Expand Down Expand Up @@ -248,7 +247,7 @@ class AmpBridPlayer extends AMP.BaseElement {
<img placeholder referrerpolicy="origin" loading="lazy" />
`;

propagateAttributes(['aria-label'], this.element, placeholder);
this.propagateAttributes(['aria-label'], placeholder);
this.applyFillContent(placeholder);

const altText = placeholder.hasAttribute('aria-label')
Expand Down
3 changes: 1 addition & 2 deletions extensions/amp-gfycat/0.1/amp-gfycat.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
import {getData, listen} from '../../../src/event-helper';
import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl';
import {isLayoutSizeDefined} from '../../../src/layout';
import {propagateAttributes} from '../../../src/core/dom/propagate-attributes';

const TAG = 'amp-gfycat';

Expand Down Expand Up @@ -91,7 +90,7 @@ class AmpGfycat extends AMP.BaseElement {
const placeholder = this.win.document.createElement('img');
const videoid = dev().assertString(this.videoid_);
this.applyFillContent(placeholder);
propagateAttributes(['alt', 'aria-label'], this.element, placeholder);
this.propagateAttributes(['alt', 'aria-label'], placeholder);
placeholder.setAttribute('loading', 'lazy');
placeholder.setAttribute('placeholder', '');
placeholder.setAttribute('referrerpolicy', 'origin');
Expand Down
Loading