♻️ Support img element (ampproject#34028)

* Allow IMG so long as it contains a loading attribute * Make width and height mandatory * Move method from within AMP.BaseElement to src/core/dom since it must act on native elements and extended AMP elements now * Update validator output * Mid way stopping point on amp-lightbox-gallery cloneLightboxableElement_ * Lightbox gallery now supports image elements * Use Sets instead of objects * PR feedback on name of utility and usage of other utilities * lightbox had a bad reference * Spy on the right thing now * Remove validator changes for a later step * Carriage returns at the end of the out files for the validator * Updated carriage returns at end of files * Update comments * Move auto lightbox to loadPromise * Remove Set in amp-auto-lightbox * Remove set in amp-image-lightbox * Remove logic specific to amp-img for the img path * Additional fixes for Set usage * test for supporting img on amp-image-lightbox * Add tests for propagate attributes helper and fix usage in iframe-video * Additional fixes for propagateAttributes from AMPElements * Bad merge * pass ampdoc in Criteria so its not obtained from an HTMLElement * change order of arguments to reduce test rewriting for Criteria * Fix types in propagate attributes * Prettier formatting * different prettier versions * update core utility with improved typing from below * Address part of Alans feedback * types * remove toLowerCase on tagName * Test changes from PR feedback (and applied elsewhere in the file) * Add support for native HTMLImageElements to amp-image-slider * Add test using HTMLImageElements * Revert changes to gestures for a later PR * Continued progress, pan zoom works and lightbox gallery is underway * LayoutScheduled for amp-carousel 0.1 when unlayout happening * Remove image support for amp-image-viewer for a future PR * Image Viewer no longer needs to exclude itself from using loadPromise directly * Remove console logging for carousel debugging: * Remove breaks in parsing of children for amp-image-slider * No need to provide an empty array for the Set constructor * Remaining console * Nit * Remove more intermediary state changes * Naming nit * prettier formatting in test
jridgewell · May 20, 2021 · 5a0936f · 5a0936f
1 parent 83f0c24
commit 5a0936f
Show file tree

Hide file tree

Showing 41 changed files with 709 additions and 334 deletions.
diff --git a/build-system/test-configs/forbidden-terms.js b/build-system/test-configs/forbidden-terms.js
@@ -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\\(\\)': {

diff --git a/builtins/amp-img/amp-img.js b/builtins/amp-img/amp-img.js
@@ -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';
@@ -130,8 +131,9 @@ export class AmpImg extends BaseElement {
           this.element
         );
       }
-      this.propagateAttributes(
+      propagateAttributes(
         attrs,
+        this.element,
         this.img_,
         /* opt_removeMissingAttrs */ true
       );
@@ -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_);

diff --git a/examples/amp-lightbox.amp.html b/examples/amp-lightbox.amp.html
@@ -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" />
       <amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" width=300 height=200></amp-img>
     </p>
     <p class="lightbox-text">

diff --git a/examples/image-lightbox.amp.html b/examples/image-lightbox.amp.html
@@ -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
   -->

diff --git a/extensions/amp-anim/0.1/amp-anim.js b/extensions/amp-anim/0.1/amp-anim.js
@@ -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';
@@ -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_);
 
@@ -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
     );

diff --git a/extensions/amp-audio/0.1/amp-audio.js b/extensions/amp-audio/0.1/amp-audio.js
@@ -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';
 
@@ -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'];
@@ -119,7 +124,7 @@ export class AmpAudio extends AMP.BaseElement {
     if (src) {
       assertHttpsUrl(src, this.element);
     }
-    this.propagateAttributes(
+    propagateAttributes(
       [
         'src',
         'preload',
@@ -131,6 +136,7 @@ export class AmpAudio extends AMP.BaseElement {
         'aria-labelledby',
         'controlsList',
       ],
+      this.element,
       audio
     );
 

diff --git a/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js b/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js
@@ -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';
@@ -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)
     );
   }
 
@@ -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)
     );
 
-    const viewport = Services.viewportForDoc(element);
+    const viewport = Services.viewportForDoc(ampdoc);
     const {width: vw, height: vh} = viewport.getSize();
 
     return meetsSizingCriteria(
@@ -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)
   );
@@ -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.
@@ -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');
     }
@@ -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'];
@@ -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)
@@ -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' &&
+            !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);
@@ -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));
     });

diff --git a/extensions/amp-auto-lightbox/0.1/test/test-amp-auto-lightbox.js b/extensions/amp-auto-lightbox/0.1/test/test-amp-auto-lightbox.js
@@ -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);
 
-              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),

diff --git a/extensions/amp-brid-player/0.1/amp-brid-player.js b/extensions/amp-brid-player/0.1/amp-brid-player.js
@@ -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';
 
@@ -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')

diff --git a/extensions/amp-gfycat/0.1/amp-gfycat.js b/extensions/amp-gfycat/0.1/amp-gfycat.js
@@ -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';
 
@@ -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');