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

♻️ Support img element #34028

Merged
merged 58 commits into from
May 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
486988e
Allow IMG so long as it contains a loading attribute
kristoferbaxter Apr 27, 2021
5bc0093
Make width and height mandatory
kristoferbaxter Apr 27, 2021
afe3dd5
Move method from within AMP.BaseElement to src/core/dom since it must…
kristoferbaxter Apr 27, 2021
3412e94
Update validator output
kristoferbaxter Apr 27, 2021
0627d01
Mid way stopping point on amp-lightbox-gallery cloneLightboxableElement_
kristoferbaxter Apr 27, 2021
ce7e2ae
Lightbox gallery now supports image elements
kristoferbaxter Apr 27, 2021
bf9daa8
Use Sets instead of objects
kristoferbaxter Apr 27, 2021
4d6a070
PR feedback on name of utility and usage of other utilities
kristoferbaxter Apr 27, 2021
7b12a26
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter Apr 28, 2021
c2aff18
lightbox had a bad reference
kristoferbaxter Apr 29, 2021
0259f76
Merge latest main
kristoferbaxter Apr 29, 2021
54d8fdd
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter Apr 29, 2021
538c254
Spy on the right thing now
kristoferbaxter Apr 29, 2021
9b7d7a7
Remove validator changes for a later step
kristoferbaxter Apr 29, 2021
0f732e2
Carriage returns at the end of the out files for the validator
kristoferbaxter Apr 29, 2021
c2bb89a
Updated carriage returns at end of files
kristoferbaxter Apr 29, 2021
04b82e5
Update comments
kristoferbaxter May 5, 2021
dffe889
Move auto lightbox to loadPromise
kristoferbaxter May 5, 2021
e174c68
Remove Set in amp-auto-lightbox
kristoferbaxter May 5, 2021
4592e36
Remove set in amp-image-lightbox
kristoferbaxter May 5, 2021
39006bd
Remove logic specific to amp-img for the img path
kristoferbaxter May 5, 2021
0d4eb49
Additional fixes for Set usage
kristoferbaxter May 6, 2021
59455c9
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter May 6, 2021
8937bdc
test for supporting img on amp-image-lightbox
kristoferbaxter May 6, 2021
90ff796
Add tests for propagate attributes helper and fix usage in iframe-video
kristoferbaxter May 6, 2021
a538b4c
Additional fixes for propagateAttributes from AMPElements
kristoferbaxter May 6, 2021
5705f71
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter May 6, 2021
20127b1
Merge conflicts
kristoferbaxter May 11, 2021
c2d3bf5
Bad merge
kristoferbaxter May 11, 2021
0b63e1a
pass ampdoc in Criteria so its not obtained from an HTMLElement
kristoferbaxter May 11, 2021
519fa81
change order of arguments to reduce test rewriting for Criteria
kristoferbaxter May 11, 2021
1130dd1
Fix types in propagate attributes
kristoferbaxter May 12, 2021
d31c757
Prettier formatting
kristoferbaxter May 12, 2021
fade270
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter May 12, 2021
c4292e8
different prettier versions
kristoferbaxter May 12, 2021
bb8a2d9
Merge branch 'main' of github.com:ampproject/amphtml into support-img
kristoferbaxter May 13, 2021
21a8dc2
update core utility with improved typing from below
kristoferbaxter May 13, 2021
15c5b58
Address part of Alans feedback
kristoferbaxter May 13, 2021
acbfd3a
types
kristoferbaxter May 13, 2021
8f7fbfc
remove toLowerCase on tagName
kristoferbaxter May 13, 2021
b448094
Test changes from PR feedback (and applied elsewhere in the file)
kristoferbaxter May 13, 2021
7e8d6a9
Add support for native HTMLImageElements to amp-image-slider
kristoferbaxter May 14, 2021
f90f409
Add test using HTMLImageElements
kristoferbaxter May 14, 2021
643aef7
Revert changes to gestures for a later PR
kristoferbaxter May 14, 2021
1cdc0ec
Continued progress, pan zoom works and lightbox gallery is underway
kristoferbaxter May 18, 2021
a46e1ee
Merge in latest main
kristoferbaxter May 18, 2021
222d247
LayoutScheduled for amp-carousel 0.1 when unlayout happening
kristoferbaxter May 19, 2021
a6885da
Remove image support for amp-image-viewer for a future PR
kristoferbaxter May 19, 2021
1f6492d
Image Viewer no longer needs to exclude itself from using loadPromise…
kristoferbaxter May 19, 2021
83a0a07
Remove console logging for carousel debugging:
kristoferbaxter May 19, 2021
02fbc8c
Remove breaks in parsing of children for amp-image-slider
kristoferbaxter May 19, 2021
05d95d5
No need to provide an empty array for the Set constructor
kristoferbaxter May 19, 2021
033f3aa
Remaining console
kristoferbaxter May 19, 2021
d8f19fe
Nit
kristoferbaxter May 19, 2021
d8fd326
Remove more intermediary state changes
kristoferbaxter May 19, 2021
343024c
Naming nit
kristoferbaxter May 20, 2021
ccb6010
Merge conflicts
kristoferbaxter May 20, 2021
61c93db
prettier formatting in test
kristoferbaxter May 20, 2021
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: 2 additions & 0 deletions build-system/test-configs/forbidden-terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,8 @@ 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: 4 additions & 2 deletions builtins/amp-img/amp-img.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ 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 @@ -130,8 +131,9 @@ export class AmpImg extends BaseElement {
this.element
);
}
this.propagateAttributes(
propagateAttributes(
alanorozco marked this conversation as resolved.
Show resolved Hide resolved
attrs,
this.element,
this.img_,
/* opt_removeMissingAttrs */ true
);
Expand Down Expand Up @@ -216,7 +218,7 @@ export class AmpImg extends BaseElement {

// It is important to call this before setting `srcset` attribute.
this.maybeGenerateSizes_(/* sync setAttribute */ true);
this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, this.img_);
this.propagateDataset(this.img_);
if (!IS_ESM) {
guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
Expand Down
2 changes: 2 additions & 0 deletions examples/amp-lightbox.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ <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" />
kristoferbaxter marked this conversation as resolved.
Show resolved Hide resolved
<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: 7 additions & 0 deletions examples/image-lightbox.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@
<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: 4 additions & 2 deletions extensions/amp-anim/0.1/amp-anim.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ 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 @@ -55,7 +56,7 @@ export class AmpAnim extends AMP.BaseElement {
buildCallback() {
this.img_ = new Image();
this.img_.setAttribute('decoding', 'async');
this.propagateAttributes(BUILD_ATTRIBUTES, this.img_);
propagateAttributes(BUILD_ATTRIBUTES, this.element, this.img_);
this.applyFillContent(this.img_, true);
propagateObjectFitStyles(this.element, this.img_);

Expand Down Expand Up @@ -88,8 +89,9 @@ 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.
this.propagateAttributes(
propagateAttributes(
LAYOUT_ATTRIBUTES,
this.element,
img,
/* opt_removeMissingAttrs */ true
);
Expand Down
10 changes: 8 additions & 2 deletions extensions/amp-audio/0.1/amp-audio.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ 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 @@ -85,7 +86,11 @@ export class AmpAudio extends AMP.BaseElement {
if (src !== undefined) {
assertHttpsUrl(src, this.element);
}
this.propagateAttributes(['src', 'loop', 'controlsList'], this.audio_);
propagateAttributes(
['src', 'loop', 'controlsList'],
this.element,
this.audio_
);
}

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

Expand Down
64 changes: 37 additions & 27 deletions extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ 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 @@ -114,25 +115,23 @@ 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, renderWidth, renderHeight) {
static meetsAll(element, ampdoc, renderWidth, renderHeight) {
return (
Criteria.meetsSizingCriteria(element, renderWidth, renderHeight) &&
Criteria.meetsTreeShapeCriteria(element)
Criteria.meetsSizingCriteria(
element,
ampdoc,
renderWidth,
renderHeight
) && Criteria.meetsTreeShapeCriteria(element)
);
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase test coverage to justify that element is an img and doesn't break otherwise.

);

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

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

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

/**
* @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 @@ -298,7 +306,7 @@ export class Scanner {
* @return {!Array<!Element>}
*/
static getCandidates(root) {
const selector = candidateSelector('amp-img');
const selector = candidateSelector(['amp-img', '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 @@ -318,7 +326,7 @@ export class DocMetaAnnotations {
* @return {string|undefined}
*/
static getOgType(ampdoc) {
const tag = getRootNode(ampdoc).querySelector(META_OG_TYPE);
const tag = ampdoc.getRootNode().querySelector(META_OG_TYPE);
if (tag) {
return tag.getAttribute('content');
}
Expand All @@ -339,7 +347,7 @@ export class DocMetaAnnotations {
* @return {!Array<string>}
*/
static getAllLdJsonTypes(ampdoc) {
return toArray(getRootNode(ampdoc).querySelectorAll(SCRIPT_LD_JSON))
return toArray(ampdoc.getRootNode().querySelectorAll(SCRIPT_LD_JSON))
.map((el) => {
const {textContent} = el;
return (tryParseJson(textContent) || {})['@type'];
Expand Down Expand Up @@ -369,10 +377,8 @@ 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) => !!getRootNode(ampdoc).querySelector(selector);
const exists = (selector) => !!ampdoc.getRootNode().querySelector(selector);

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

const {width, height} = boundingClientRect;
if (!Criteria.meetsAll(candidate, width, height)) {
if (!Criteria.meetsAll(candidate, ampdoc, width, height)) {
return;
}
dev().info(TAG, 'apply', candidate);
Expand Down Expand Up @@ -475,7 +485,7 @@ export function scan(ampdoc, opt_root) {
AMP.extension(TAG, '0.1', (AMP) => {
const {ampdoc} = AMP;
ampdoc.whenReady().then(() => {
getRootNode(ampdoc).addEventListener(AmpEvents.DOM_UPDATE, (e) => {
ampdoc.getRootNode().addEventListener(AmpEvents.DOM_UPDATE, (e) => {
const {target} = e;
scan(ampdoc, dev().assertElement(target));
});
Expand Down
61 changes: 47 additions & 14 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,29 +140,62 @@ describes.realWin(

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit suggestion: consider destructuring in args


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

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

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

expect(
Criteria.meetsTreeShapeCriteria(candidate),
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-brid-player/0.1/amp-brid-player.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ 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 @@ -247,7 +248,7 @@ class AmpBridPlayer extends AMP.BaseElement {
<img placeholder referrerpolicy="origin" loading="lazy" />
`;

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

const altText = placeholder.hasAttribute('aria-label')
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-gfycat/0.1/amp-gfycat.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ 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 @@ -90,7 +91,7 @@ class AmpGfycat extends AMP.BaseElement {
const placeholder = this.win.document.createElement('img');
const videoid = dev().assertString(this.videoid_);
this.applyFillContent(placeholder);
this.propagateAttributes(['alt', 'aria-label'], placeholder);
propagateAttributes(['alt', 'aria-label'], this.element, placeholder);
placeholder.setAttribute('loading', 'lazy');
placeholder.setAttribute('placeholder', '');
placeholder.setAttribute('referrerpolicy', 'origin');
Expand Down
Loading