diff --git a/build-system/test-configs/forbidden-terms.js b/build-system/test-configs/forbidden-terms.js index 2a3d68c03aa6..ceb6767eb50d 100644 --- 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 index dc6cd6518e2c..b405a2e13147 100644 --- 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 index 0dcfa9652c18..30c236916a3c 100644 --- a/examples/amp-lightbox.amp.html +++ b/examples/amp-lightbox.amp.html @@ -63,6 +63,8 @@

Scrollable Lightbox

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.

+ +

diff --git a/examples/image-lightbox.amp.html b/examples/image-lightbox.amp.html index 25e861cf1ba8..2b908c612cee 100644 --- a/examples/image-lightbox.amp.html +++ b/examples/image-lightbox.amp.html @@ -22,6 +22,13 @@ + + diff --git a/extensions/amp-anim/0.1/amp-anim.js b/extensions/amp-anim/0.1/amp-anim.js index 47205b27615c..7f3dab9cb7c3 100644 --- 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 index 9491dfaeba9f..92baf1cf7473 100644 --- 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 index 97a4be2146c4..11748995f7e2 100644 --- 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,11 +273,16 @@ function markAsVisited(candidate) { } /** - * @param {string} tagName + * @param {!Array} 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(','); } /** @@ -285,6 +290,9 @@ function candidateSelector(tagName) { * @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} */ 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} */ 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}) => { - // 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) + ) { + // 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 index b8759821db77..5943624278f3 100644 --- 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` `, - html` -

- -
- `, - html` -
+ { + markup: html` + + `, + tagName: 'AMP-IMG', + }, + { + markup: html`
- +
-
- `, + `, + tagName: 'AMP-IMG', + }, + { + markup: html` +
+
+ +
+
+ `, + tagName: 'AMP-IMG', + }, + { + markup: html` `, + tagName: 'IMG', + }, + { + markup: html` +
+ +
+ `, + tagName: 'IMG', + }, + { + markup: html` +
+
+ +
+
+ `, + 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 index 4b29157fd4dd..5716e8ecb40a 100644 --- 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 { `; - 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 index 33d55715deed..b2f6f8d2858f 100644 --- 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'); diff --git a/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js b/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js index 45ba247a2082..6ccaecc02c63 100644 --- a/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js +++ b/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js @@ -31,6 +31,7 @@ import {Services} from '../../../src/services'; import {addParamToUrl} from '../../../src/url'; import {dev, userAssert} from '../../../src/log'; import {isLayoutSizeDefined} from '../../../src/layout'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {removeElement} from '../../../src/dom'; export const TAG = 'amp-google-document-embed'; @@ -90,7 +91,7 @@ export class AmpDriveViewer extends AMP.BaseElement { iframe.setAttribute('frameborder', '0'); iframe.setAttribute('allowfullscreen', ''); - this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, iframe); + propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, iframe); iframe.src = this.getSrc_(this.element.getAttribute('src')); @@ -105,7 +106,12 @@ export class AmpDriveViewer extends AMP.BaseElement { (value) => mutations[value] !== undefined ); const iframe = dev().assertElement(this.iframe_); - this.propagateAttributes(attrs, iframe, /* opt_removeMissingAttrs */ true); + propagateAttributes( + attrs, + this.element, + iframe, + /* opt_removeMissingAttrs */ true + ); const src = mutations['src']; if (src) { iframe.src = this.getSrc_(src); diff --git a/extensions/amp-iframe/0.1/amp-iframe.js b/extensions/amp-iframe/0.1/amp-iframe.js index ca1fab43a74f..cd35cf94d5ce 100644 --- a/extensions/amp-iframe/0.1/amp-iframe.js +++ b/extensions/amp-iframe/0.1/amp-iframe.js @@ -36,6 +36,7 @@ import {isAdPositionAllowed} from '../../../src/ad-helper'; import {isExperimentOn} from '../../../src/experiments'; import {moveLayoutRect} from '../../../src/layout-rect'; import {parseJson} from '../../../src/core/types/object/json'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {removeElement} from '../../../src/dom'; import {removeFragment} from '../../../src/url'; import {setStyle} from '../../../src/style'; @@ -414,7 +415,7 @@ export class AmpIframe extends AMP.BaseElement { setStyle(iframe, 'zIndex', -1); } - this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, iframe); + propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, iframe); // TEMPORARY: disable `allow=autoplay` // This is a workaround for M72-M74 user-activation breakage. @@ -619,7 +620,7 @@ export class AmpIframe extends AMP.BaseElement { } if (this.iframe_ && mutations['title']) { // only propagating title because propagating all causes e2e error: - this.propagateAttributes(['title'], this.iframe_); + propagateAttributes(['title'], this.element, this.iframe_); } } diff --git a/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js b/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js index c492ebc6485b..805044e11370 100644 --- a/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js +++ b/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js @@ -39,16 +39,14 @@ import { layoutRectLtwh, moveLayoutRect, } from '../../../src/layout-rect'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setStyles, toggle} from '../../../src/style'; import {srcsetFromElement} from '../../../src/srcset'; const TAG = 'amp-image-lightbox'; -/** @private @const {!Object} */ -const SUPPORTED_ELEMENTS_ = { - 'amp-img': true, - 'amp-anim': true, -}; +/** @private @const {!Set} */ +const SUPPORTED_ELEMENTS_ = new Set(['amp-img', 'amp-anim', 'img']); /** @private @const */ const ARIA_ATTRIBUTES = ['aria-label', 'aria-describedby', 'aria-labelledby']; @@ -253,9 +251,15 @@ export class ImageViewer { this.setSourceDimensions_(sourceElement, sourceImage); this.srcset_ = srcsetFromElement(sourceElement); - sourceElement.getImpl().then((elem) => { - elem.propagateAttributes(ARIA_ATTRIBUTES, this.image_); - }); + if (sourceElement.tagName.toLowerCase() === 'img') { + propagateAttributes(ARIA_ATTRIBUTES, sourceElement, this.image_); + } else { + sourceElement + .getImpl() + .then((impl) => + propagateAttributes(ARIA_ATTRIBUTES, impl.element, this.image_) + ); + } if (sourceImage && isLoaded(sourceImage) && sourceImage.src) { // Set src provisionally to the known loaded value for fast display. @@ -869,8 +873,9 @@ class AmpImageLightbox extends AMP.BaseElement { this.buildLightbox_(); const source = invocation.caller; + const tagName = source.tagName.toLowerCase(); userAssert( - source && SUPPORTED_ELEMENTS_[source.tagName.toLowerCase()], + source && SUPPORTED_ELEMENTS_.has(tagName), 'Unsupported element: %s', source.tagName ); diff --git a/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js b/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js index b3b42f79ee23..367f2473ef20 100644 --- a/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js +++ b/extensions/amp-image-lightbox/0.1/test/test-amp-image-lightbox.js @@ -65,19 +65,13 @@ describes.realWin( const impl = await lightbox.getImpl(false); const noop = () => {}; - impl.getViewport = () => { - return { - onChanged: noop, - enterLightboxMode: noop, - }; - }; - impl.getHistory_ = () => { - return { - push: () => { - return Promise.resolve(); - }, - }; - }; + impl.getViewport = () => ({ + onChanged: noop, + enterLightboxMode: noop, + }); + impl.getHistory_ = () => ({ + push: () => Promise.resolve(), + }); impl.enter_ = noop; const ampImage = doc.createElement('amp-img'); @@ -87,23 +81,69 @@ describes.realWin( const container = lightbox.querySelector( '.i-amphtml-image-lightbox-container' ); - expect(container).to.not.equal(null); + expect(container).to.not.be.null; + + const caption = container.querySelector( + '.i-amphtml-image-lightbox-caption' + ); + expect(caption).to.not.be.null; + expect(caption).to.have.class('amp-image-lightbox-caption'); + + const viewer = container.querySelector( + '.i-amphtml-image-lightbox-viewer' + ); + expect(viewer).to.not.be.null; + + const image = viewer.querySelector( + '.i-amphtml-image-lightbox-viewer-image' + ); + expect(image).to.not.be.null; + + // Very important. Image must have transform-origin=50% 50%. + const win = image.ownerDocument.defaultView; + expect(win.getComputedStyle(image)['transform-origin']).to.equal( + '50% 50%' + ); + }); + + it('should render correctly with an img element', async () => { + const lightbox = await getImageLightbox(); + const impl = await lightbox.getImpl(false); + + const noop = () => {}; + impl.getViewport = () => ({ + onChanged: noop, + enterLightboxMode: noop, + }); + impl.getHistory_ = () => ({ + push: () => Promise.resolve(), + }); + impl.enter_ = noop; + + const img = doc.createElement('img'); + img.setAttribute('src', 'data:'); + impl.open_({caller: img}); + + const container = lightbox.querySelector( + '.i-amphtml-image-lightbox-container' + ); + expect(container).to.not.be.null; const caption = container.querySelector( '.i-amphtml-image-lightbox-caption' ); - expect(caption).to.not.equal(null); + expect(caption).to.not.be.null; expect(caption).to.have.class('amp-image-lightbox-caption'); const viewer = container.querySelector( '.i-amphtml-image-lightbox-viewer' ); - expect(viewer).to.not.equal(null); + expect(viewer).to.not.be.null; const image = viewer.querySelector( '.i-amphtml-image-lightbox-viewer-image' ); - expect(image).to.not.equal(null); + expect(image).to.not.be.null; // Very important. Image must have transform-origin=50% 50%. const win = image.ownerDocument.defaultView; @@ -309,6 +349,7 @@ describes.realWin( let loadPromiseStub; const sourceElement = { + tagName: 'amp-img', offsetWidth: 101, offsetHeight: 201, getAttribute: (name) => { diff --git a/extensions/amp-image-slider/0.1/amp-image-slider.js b/extensions/amp-image-slider/0.1/amp-image-slider.js index 4641468401cb..1b9cf26c8de7 100644 --- a/extensions/amp-image-slider/0.1/amp-image-slider.js +++ b/extensions/amp-image-slider/0.1/amp-image-slider.js @@ -22,13 +22,16 @@ import {Services} from '../../../src/services'; import {SwipeXRecognizer} from '../../../src/gesture-recognizers'; import {clamp} from '../../../src/utils/math'; import {dev, user, userAssert} from '../../../src/log'; +import {htmlFor} from '../../../src/static-template'; import {isLayoutSizeDefined} from '../../../src/layout'; -import {listen} from '../../../src/event-helper'; +import {listen, loadPromise} from '../../../src/event-helper'; import { observeWithSharedInOb, unobserveWithSharedInOb, } from '../../../src/viewport-observer'; -import {setStyles} from '../../../src/style'; +import {setStyle, setStyles} from '../../../src/style'; + +const VALID_IMAGE_TAGNAMES = new Set(['AMP-IMG', 'IMG']); export class AmpImageSlider extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -42,10 +45,11 @@ export class AmpImageSlider extends AMP.BaseElement { this.container_ = null; /** @private {?Element} */ - this.leftAmpImage_ = null; - + this.leftImage_ = null; /** @private {?Element} */ - this.rightAmpImage_ = null; + this.rightImage_ = null; + /** @private {boolean} */ + this.containsAmpImages_ = false; /** @private {?Element} */ this.leftLabelWrapper_ = null; @@ -59,16 +63,12 @@ export class AmpImageSlider extends AMP.BaseElement { /** @private {?Element} */ this.leftMask_ = null; - /** @private {?Element} */ this.rightMask_ = null; /** @private {?Element} */ this.bar_ = null; - /** @private {?Element} */ - this.barStick_ = null; - /** @private {?Element} */ this.hintLeftArrow_ = null; /** @private {?Element} */ @@ -112,22 +112,21 @@ export class AmpImageSlider extends AMP.BaseElement { buildCallback() { const children = this.getRealChildren(); - for (let i = 0; i < children.length; i++) { - const child = children[i]; - if (child.tagName.toLowerCase() === 'amp-img') { + for (const child of children) { + if (VALID_IMAGE_TAGNAMES.has(child.tagName)) { // First encountered = left image // Second encountered = right image - if (!this.leftAmpImage_) { - this.leftAmpImage_ = child; - } else if (!this.rightAmpImage_) { - this.rightAmpImage_ = child; + if (!this.leftImage_) { + this.leftImage_ = child; + } else if (!this.rightImage_) { + this.rightImage_ = child; } else { user().error( 'AMP-IMAGE-SLIDER', - 'Should not contain more than 2 s.' + 'Should not contain more than 2 images.' ); } - } else if (child.tagName.toLowerCase() === 'div') { + } else if (child.tagName === 'DIV') { if (child.hasAttribute('first')) { this.leftLabel_ = child; } else if (child.hasAttribute('second')) { @@ -143,18 +142,25 @@ export class AmpImageSlider extends AMP.BaseElement { } userAssert( - this.leftAmpImage_ && this.rightAmpImage_, - '2 s must be provided for comparison' + this.leftImage_ && this.rightImage_, + '2 images must be provided for comparison' ); // see comment in layoutCallback // When layers not enabled const owners = Services.ownersForDoc(this.element); - owners.setOwner(dev().assertElement(this.leftAmpImage_), this.element); - owners.setOwner(dev().assertElement(this.rightAmpImage_), this.element); + if (this.leftImage_.tagName === 'AMP-IMG') { + owners.setOwner(dev().assertElement(this.leftImage_), this.element); + this.containsAmpImages_ = true; + } + if (this.rightImage_.tagName === 'AMP-IMG') { + owners.setOwner(dev().assertElement(this.rightImage_), this.element); + this.containsAmpImages_ = true; + } - this.container_ = this.doc_.createElement('div'); - this.container_.classList.add('i-amphtml-image-slider-container'); + this.container_ = htmlFor( + this.doc_ + )`
`; this.buildImageWrappers_(); this.buildBar_(); @@ -187,8 +193,8 @@ export class AmpImageSlider extends AMP.BaseElement { return this.mutateElement(() => { this.element.appendChild(this.container_); // Ensure ampdoc exists on the amp-imgs - this.leftMask_.appendChild(this.leftAmpImage_); - this.rightMask_.appendChild(this.rightAmpImage_); + this.leftMask_.appendChild(this.leftImage_); + this.rightMask_.appendChild(this.rightImage_); // Set initial positioning if (initialPositionString) { const initialPosition = Number(initialPositionString); @@ -196,9 +202,7 @@ export class AmpImageSlider extends AMP.BaseElement { } // Prevent Edge horizontal swipe for go back/forward if (this.isEdge_) { - setStyles(this.element, { - 'touch-action': 'pan-y', // allow browser only default y behavior - }); + setStyle(this.element, 'touch-action', 'pan-y'); // allow browser only default y behavior } }); } @@ -226,7 +230,7 @@ export class AmpImageSlider extends AMP.BaseElement { this.rightMask_.classList.add('i-amphtml-image-slider-right-mask'); this.rightMask_.classList.add('i-amphtml-image-slider-push-right'); - this.rightAmpImage_.classList.add('i-amphtml-image-slider-push-left'); + this.rightImage_.classList.add('i-amphtml-image-slider-push-left'); if (this.rightLabel_) { this.rightLabelWrapper_ = this.doc_.createElement('div'); this.rightLabelWrapper_.classList.add( @@ -243,14 +247,11 @@ export class AmpImageSlider extends AMP.BaseElement { * @private */ buildBar_() { - this.bar_ = this.doc_.createElement('div'); - this.barStick_ = this.doc_.createElement('div'); - this.bar_.appendChild(this.barStick_); - - this.bar_.classList.add('i-amphtml-image-slider-bar'); - this.bar_.classList.add('i-amphtml-image-slider-push-right'); - this.barStick_.classList.add('i-amphtml-image-slider-bar-stick'); - this.barStick_.classList.add('i-amphtml-image-slider-push-left'); + this.bar_ = htmlFor( + this.doc_ + )`
+
+
`; this.container_.appendChild(this.bar_); } @@ -295,8 +296,13 @@ export class AmpImageSlider extends AMP.BaseElement { * @private */ checkARIA_() { - const leftAmpImage = dev().assertElement(this.leftAmpImage_); - const rightAmpImage = dev().assertElement(this.rightAmpImage_); + if (!this.containsAmpImages_) { + return; + } + + // Only if there are AMP-IMG Elements in use should this pathway execute. + const leftAmpImage = dev().assertElement(this.leftImage_); + const rightAmpImage = dev().assertElement(this.rightImage_); leftAmpImage .signals() .whenSignal(CommonSignals.LOAD_END) @@ -666,7 +672,7 @@ export class AmpImageSlider extends AMP.BaseElement { this.updateTranslateX_(this.bar_, percentFromLeft); this.updateTranslateX_(this.rightMask_, percentFromLeft); - this.updateTranslateX_(this.rightAmpImage_, -percentFromLeft); + this.updateTranslateX_(this.rightImage_, -percentFromLeft); const adjustedDeltaFromLeft = percentFromLeft - 0.5; this.updateTranslateX_(this.hintLeftBody_, adjustedDeltaFromLeft); this.updateTranslateX_(this.hintRightBody_, adjustedDeltaFromLeft); @@ -708,45 +714,44 @@ export class AmpImageSlider extends AMP.BaseElement { observeWithSharedInOb(this.element, (inViewport) => this.viewportCallback_(inViewport) ); - // Extensions such as amp-carousel still uses .setOwner() - // This would break the rendering of the images as carousel - // will call .scheduleLayout on the slider but not the contents - // while Resources would found amp-imgs' parent has owner and - // refuse to run the normal scheduling in discoverWork_. - // SIMPLER SOL: simply always call scheduleLayout no matter what - const owners = Services.ownersForDoc(this.element); - owners.scheduleLayout( - this.element, - dev().assertElement(this.leftAmpImage_) - ); - owners.scheduleLayout( - this.element, - dev().assertElement(this.rightAmpImage_) - ); + + const appendHints = () => { + this.container_.appendChild(this.hintLeftBody_); + this.container_.appendChild(this.hintRightBody_); + }; this.registerEvents_(); + if (this.containsAmpImages_) { + // Extensions such as amp-carousel still uses .setOwner() + // This would break the rendering of the images as carousel + // will call .scheduleLayout on the slider but not the contents + // while Resources would found amp-imgs' parent has owner and + // refuse to run the normal scheduling in discoverWork_. + // SIMPLER SOL: simply always call scheduleLayout no matter what + const owners = Services.ownersForDoc(this.element); + owners.scheduleLayout(this.element, dev().assertElement(this.leftImage_)); + owners.scheduleLayout( + this.element, + dev().assertElement(this.rightImage_) + ); + + return Promise.all([ + dev() + .assertElement(this.leftImage_) + .signals() + .whenSignal(CommonSignals.LOAD_END), + dev() + .assertElement(this.rightImage_) + .signals() + .whenSignal(CommonSignals.LOAD_END), + ]).then(appendHints, appendHints); + } + return Promise.all([ - dev() - .assertElement(this.leftAmpImage_) - .signals() - .whenSignal(CommonSignals.LOAD_END), - dev() - .assertElement(this.rightAmpImage_) - .signals() - .whenSignal(CommonSignals.LOAD_END), - ]).then( - () => { - // Notice: hints are attached after amp-img finished loading - this.container_.appendChild(this.hintLeftBody_); - this.container_.appendChild(this.hintRightBody_); - }, - () => { - // Do the same thing when signal rejects - this.container_.appendChild(this.hintLeftBody_); - this.container_.appendChild(this.hintRightBody_); - } - ); + loadPromise(this.leftImage_), + loadPromise(this.rightImage_), + ]).then(appendHints, appendHints); } /** @override */ diff --git a/extensions/amp-image-slider/0.1/test/integration/test-amp-image-slider.js b/extensions/amp-image-slider/0.1/test/integration/test-amp-image-slider.js index 7afec4dc61bd..9f9e13a38e59 100644 --- a/extensions/amp-image-slider/0.1/test/integration/test-amp-image-slider.js +++ b/extensions/amp-image-slider/0.1/test/integration/test-amp-image-slider.js @@ -25,18 +25,30 @@ t.run('amp-image-slider', {}, function () { // We have 2 sliders, #s1 and #s2 // #s2 has attribute `disable-hint-reappear` set // A huge padding is added to the bottom to allow room for scrolling - const sliderBody = ` - + const sliderBody = /* HTML */ ` +
BEFORE
AFTER
 - +
BEFORE
@@ -45,6 +57,32 @@ t.run('amp-image-slider', {}, function () {

HUGE PADDING

+ + + + +
BEFORE
+
AFTER
+ + `; const css = ` @@ -61,6 +99,11 @@ t.run('amp-image-slider', {}, function () { font-family: Arial, Helvetica, sans-serif; box-shadow: 2px 2px 27px 5px rgba(0,0,0,0.75); } + img.custom-fill { + object-fit: cover; + width: 100%; + height: 100%; + } `; const extensions = ['amp-image-slider']; @@ -79,6 +122,7 @@ t.run('amp-image-slider', {}, function () { let observerTimeout; let s1; // sliderInfo of slider 1 let s2; // sliderInfo of slider 2 + let s3; // sliderInfo of slider 3 beforeEach(() => { win = env.win; @@ -668,6 +712,24 @@ t.run('amp-image-slider', {}, function () { ); }); + it('should hide hint on user interaction (e.g. mousedown) with images', () => { + const dispatchMouseDownEventFunction = () => + s3.slider.dispatchEvent(createMouseDownEvent(s3.pos.percent40)); + const isHintHiddenCallback = () => + s3.leftHint.classList.contains( + 'i-amphtml-image-slider-hint-hidden' + ); + // Initially hint should be displayed + expect(isHintHiddenCallback()).to.be.false; + // Make sure hint is hidden after interaction + return invokeAndObserve( + /*invokeFunc*/ dispatchMouseDownEventFunction, + /*target*/ s3.slider, + /*cb*/ isHintHiddenCallback, + /*opt_errorMessage*/ 'Hint failed to be hidden' + ); + }); + // TODO: (#17581) // This test flakes. May require events/signals to help solve the issue. it.skip( @@ -810,15 +872,17 @@ t.run('amp-image-slider', {}, function () { // Guaranteed that sliders are both here const slider1 = doc.getElementById('s1'); const slider2 = doc.getElementById('s2'); + const slider3 = doc.getElementById('s3'); // Check if signals have been installed const areSignalsInstalled = () => - !!slider1.signals && !!slider2.signals; + !!slider1.signals && !!slider2.signals && !!slider3.signals; // LOAD_END promises of sliders const haveSlidersLoadEnded = () => { return Promise.all([ slider1.signals().whenSignal('load-end'), slider2.signals().whenSignal('load-end'), + slider3.signals().whenSignal('load-end'), ]); }; // Start observer first to capture signal @@ -844,6 +908,7 @@ t.run('amp-image-slider', {}, function () { function setup() { const slider1 = doc.getElementById('s1'); const slider2 = doc.getElementById('s2'); + const slider3 = doc.getElementById('s3'); // Creates a sliderInfo of slider // sliderInfo is a collection of information of the slider @@ -897,6 +962,7 @@ t.run('amp-image-slider', {}, function () { s1 = createSliderInfo(slider1); s2 = createSliderInfo(slider2); + s3 = createSliderInfo(slider3); // The viewport test has been flaky for quite a while // A possibility is that the viewport might be high enough to keep diff --git a/extensions/amp-image-viewer/0.1/amp-image-viewer.js b/extensions/amp-image-viewer/0.1/amp-image-viewer.js index bc8b7dab0054..a468ddeddce8 100644 --- a/extensions/amp-image-viewer/0.1/amp-image-viewer.js +++ b/extensions/amp-image-viewer/0.1/amp-image-viewer.js @@ -46,6 +46,7 @@ import { observeContentSize, unobserveContentSize, } from '../../../src/utils/size-observer'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setStyles} from '../../../src/style'; import {srcsetFromElement} from '../../../src/srcset'; @@ -54,10 +55,7 @@ const TAG = 'amp-image-viewer'; const ARIA_ATTRIBUTES = ['aria-label', 'aria-describedby', 'aria-labelledby']; const DEFAULT_MAX_SCALE = 2; -const ELIGIBLE_TAGS = { - 'amp-img': true, - 'amp-anim': true, -}; +const ELIGIBLE_TAGS = new Set(['AMP-IMG', 'AMP-ANIM', 'IMG']); export class AmpImageViewer extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -122,7 +120,7 @@ export class AmpImageViewer extends AMP.BaseElement { this.motion_ = null; /** @private {?Element} */ - this.sourceAmpImage_ = null; + this.sourceImage_ = null; /** @private {?Promise} */ this.loadPromise_ = null; @@ -147,9 +145,9 @@ export class AmpImageViewer extends AMP.BaseElement { TAG ); - this.sourceAmpImage_ = children[0]; + this.sourceImage_ = children[0]; Services.ownersForDoc(this.element).setOwner( - this.sourceAmpImage_, + this.sourceImage_, this.element ); } @@ -177,14 +175,14 @@ export class AmpImageViewer extends AMP.BaseElement { // TODO(sparhami, cathyxz) Refactor image viewer once auto sizes lands to // use the amp-img as-is, which means we can simplify this logic to just // wait for the layout signal. - const ampImg = dev().assertElement(this.sourceAmpImage_); + const img = dev().assertElement(this.sourceImage_); const haveImg = !!this.image_; const laidOutPromise = haveImg ? Promise.resolve() - : ampImg.signals().whenSignal(CommonSignals.LOAD_END); + : img.signals().whenSignal(CommonSignals.LOAD_END); if (!haveImg) { - Services.ownersForDoc(this.element).scheduleLayout(this.element, ampImg); + Services.ownersForDoc(this.element).scheduleLayout(this.element, img); } this.loadPromise_ = laidOutPromise @@ -269,7 +267,7 @@ export class AmpImageViewer extends AMP.BaseElement { * @private */ elementIsSupported_(element) { - return ELIGIBLE_TAGS[element.tagName.toLowerCase()]; + return ELIGIBLE_TAGS.has(element.tagName); } /** @@ -305,9 +303,9 @@ export class AmpImageViewer extends AMP.BaseElement { this.image_ = this.element.ownerDocument.createElement('img'); this.image_.classList.add('i-amphtml-image-viewer-image'); - const ampImg = dev().assertElement(this.sourceAmpImage_); - this.setSourceDimensions_(ampImg); - this.srcset_ = srcsetFromElement(ampImg); + const img = dev().assertElement(this.sourceImage_); + this.setSourceDimensions_(img); + this.srcset_ = srcsetFromElement(img); observeContentSize(this.element, this.onResize_); @@ -318,10 +316,10 @@ export class AmpImageViewer extends AMP.BaseElement { width: 0, height: 0, }); - st.toggle(ampImg, false); + st.toggle(img, false); this.element.appendChild(this.image_); - return ampImg.getImpl().then((ampImg) => { - ampImg.propagateAttributes(ARIA_ATTRIBUTES, this.image_); + return img.getImpl().then((impl) => { + propagateAttributes(ARIA_ATTRIBUTES, impl.element, this.image_); }); }); } diff --git a/extensions/amp-inline-gallery/0.1/amp-inline-gallery-thumbnails.js b/extensions/amp-inline-gallery/0.1/amp-inline-gallery-thumbnails.js index 2115ec6e9321..d7664df81d10 100644 --- a/extensions/amp-inline-gallery/0.1/amp-inline-gallery-thumbnails.js +++ b/extensions/amp-inline-gallery/0.1/amp-inline-gallery-thumbnails.js @@ -21,6 +21,7 @@ import {dict} from '../../../src/core/types/object'; import {htmlFor} from '../../../src/static-template'; import {isLayoutSizeDefined} from '../../../src/layout'; import {matches, scopedQuerySelector} from '../../../src/dom'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setStyle} from '../../../src/style'; /** @@ -246,7 +247,7 @@ export class AmpInlineGalleryThumbnails extends AMP.BaseElement { // We create with loop defaulting to false above, and allow it to be // overwriten. - this.propagateAttributes(['loop'], this.carousel_); + propagateAttributes(['loop'], this.element, this.carousel_); this.element.appendChild(this.carousel_); } } diff --git a/extensions/amp-jwplayer/0.1/amp-jwplayer.js b/extensions/amp-jwplayer/0.1/amp-jwplayer.js index ff86779e816f..d51675997456 100644 --- a/extensions/amp-jwplayer/0.1/amp-jwplayer.js +++ b/extensions/amp-jwplayer/0.1/amp-jwplayer.js @@ -42,6 +42,7 @@ import {getMode} from '../../../src/mode'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isLayoutSizeDefined} from '../../../src/layout'; import {once} from '../../../src/core/types/function'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; const JWPLAYER_EVENTS = { 'ready': VideoEvents.LOAD, @@ -349,7 +350,7 @@ class AmpJWPlayer extends AMP.BaseElement { return; } const placeholder = this.win.document.createElement('img'); - this.propagateAttributes(['aria-label'], placeholder); + propagateAttributes(['aria-label'], this.element, placeholder); this.applyFillContent(placeholder); placeholder.setAttribute('placeholder', ''); placeholder.setAttribute('referrerpolicy', 'origin'); diff --git a/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js b/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js index ce704b25b6f4..6d984925ca43 100644 --- a/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js +++ b/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js @@ -20,6 +20,7 @@ import {addParamsToUrl} from '../../../src/url'; import {dict} from '../../../src/core/types/object'; import {getDataParamsFromAttributes} from '../../../src/dom'; import {isLayoutSizeDefined} from '../../../src/layout'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setIsMediaComponent} from '../../../src/video-interface'; import {userAssert} from '../../../src/log'; @@ -125,7 +126,7 @@ class AmpKaltura extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.win.document.createElement('img'); - this.propagateAttributes(['aria-label'], placeholder); + propagateAttributes(['aria-label'], this.element, placeholder); this.applyFillContent(placeholder); placeholder.setAttribute('loading', 'lazy'); placeholder.setAttribute('placeholder', ''); diff --git a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js index 6ff5492bc446..eb8a43b70a53 100644 --- a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js +++ b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js @@ -275,11 +275,11 @@ export class AmpLightboxGallery extends AMP.BaseElement { * @private */ cloneLightboxableElement_(element) { - const fallback = element.getFallback(); - const shouldCloneFallback = - element.classList.contains('amp-notsupported') && !!fallback; - if (shouldCloneFallback) { - element = fallback; + if (element.classList.contains('amp-notsupported')) { + const fallback = element.getFallback(); + if (!!fallback) { + element = fallback; + } } const deepClone = !element.classList.contains('i-amphtml-element'); const clonedNode = element.cloneNode(deepClone); @@ -308,7 +308,7 @@ export class AmpLightboxGallery extends AMP.BaseElement { element: dev().assertElement(clonedNode), }; let slide = clonedNode; - if (ELIGIBLE_TAP_TAGS[clonedNode.tagName]) { + if (ELIGIBLE_TAP_TAGS.has(clonedNode.tagName)) { const container = this.doc_.createElement('div'); const imageViewer = htmlFor(this.doc_)` `; @@ -811,7 +811,7 @@ export class AmpLightboxGallery extends AMP.BaseElement { if (!element || !isLoaded(element)) { return false; } - if (!ELIGIBLE_TAP_TAGS[element.tagName]) { + if (!ELIGIBLE_TAP_TAGS.has(element.tagName)) { return false; } const img = elementByTag(dev().assertElement(element), 'img'); @@ -1332,11 +1332,11 @@ export class AmpLightboxGallery extends AMP.BaseElement { const thumbnailElement = this.createThumbnailElement_(thumbnail); thumbnails.push(thumbnailElement); }); - this.mutateElement(() => { - thumbnails.forEach((thumbnailElement) => { - this.gallery_.appendChild(thumbnailElement); - }); - }); + this.mutateElement(() => + thumbnails.forEach((thumbnailElement) => + this.gallery_.appendChild(thumbnailElement) + ) + ); } /** diff --git a/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js b/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js index de867e549d25..012c05248fd0 100644 --- a/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js +++ b/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js @@ -38,21 +38,16 @@ import {map} from '../../../../src/core/types/object'; import {srcsetFromElement, srcsetFromSrc} from '../../../../src/srcset'; import {toArray} from '../../../../src/core/types/array'; -const LIGHTBOX_ELIGIBLE_TAGS = { - 'AMP-IMG': true, -}; +const LIGHTBOX_ELIGIBLE_TAGS = new Set(['AMP-IMG', 'IMG']); -export const ELIGIBLE_TAP_TAGS = { - 'AMP-IMG': true, -}; +// eslint-disable-next-line local/no-export-side-effect +export const ELIGIBLE_TAP_TAGS = new Set(['AMP-IMG', 'IMG']); -export const VIDEO_TAGS = { - 'AMP-YOUTUBE': true, - 'AMP-VIDEO': true, -}; +// eslint-disable-next-line local/no-export-side-effect +export const VIDEO_TAGS = new Set(['AMP-YOUTUBE', 'AMP-VIDEO']); const GALLERY_TAG = 'amp-lightbox-gallery'; -const CAROUSEL_TAGS = ['AMP-CAROUSEL', 'AMP-BASE-CAROUSEL']; +const CAROUSEL_TAGS = new Set(['AMP-CAROUSEL', 'AMP-BASE-CAROUSEL']); const FIGURE_TAG = 'FIGURE'; const SLIDE_SELECTOR = '.amp-carousel-slide, .i-amphtml-carousel-slotted'; @@ -126,9 +121,9 @@ export class LightboxManager { // mapped unique ids. /** * List of lightbox elements that have already been scanned. - * @private {!Array} + * @private {!Set} */ - this.seen_ = []; + this.seen_ = new Set(); } /** @@ -166,7 +161,9 @@ export class LightboxManager { */ scanLightboxables_() { return this.ampdoc_.whenReady().then(() => { - const matches = this.ampdoc_.getRootNode().querySelectorAll('[lightbox]'); + const matches = this.ampdoc_ + .getRootNode() + .querySelectorAll('[lightbox],[data-lightbox]'); const processLightboxElement = this.processLightboxElement_.bind(this); iterateCursor(matches, processLightboxElement); }); @@ -179,7 +176,7 @@ export class LightboxManager { * @private */ baseElementIsSupported_(element) { - return LIGHTBOX_ELIGIBLE_TAGS[element.tagName]; + return LIGHTBOX_ELIGIBLE_TAGS.has(element.tagName); } /** @@ -203,11 +200,11 @@ export class LightboxManager { return; } const baseElement = getBaseElementForSlide(slide); - if (this.seen_.includes(baseElement)) { + if (this.seen_.has(baseElement)) { return; } baseElement.setAttribute('lightbox', lightboxGroupId); - this.seen_.push(baseElement); + this.seen_.add(baseElement); this.processBaseLightboxElement_(baseElement, lightboxGroupId); }); }); @@ -218,11 +215,11 @@ export class LightboxManager { * @private */ processLightboxElement_(element) { - if (this.seen_.includes(element)) { + if (this.seen_.has(element)) { return; } - this.seen_.push(element); - if (CAROUSEL_TAGS.includes(element.tagName)) { + this.seen_.add(element); + if (CAROUSEL_TAGS.has(element.tagName)) { this.processLightboxCarousel_(element); } else { const lightboxGroupId = element.getAttribute('lightbox') || 'default'; @@ -405,7 +402,7 @@ export class LightboxManager { if (element.hasAttribute('lightbox-thumbnail-id')) { const thumbnailId = element.getAttribute('lightbox-thumbnail-id'); const thumbnailImage = this.ampdoc_.getElementById(thumbnailId); - if (thumbnailImage && thumbnailImage.tagName == 'AMP-IMG') { + if (LIGHTBOX_ELIGIBLE_TAGS.has(thumbnailImage?.tagName)) { return srcsetFromElement(thumbnailImage); } } @@ -419,7 +416,7 @@ export class LightboxManager { * @private */ getUserPlaceholderSrcset_(element) { - if (element.tagName == 'AMP-IMG') { + if (LIGHTBOX_ELIGIBLE_TAGS.has(element.tagName)) { return srcsetFromElement(element); } if (element.tagName == 'AMP-VIDEO') { diff --git a/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js b/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js index 9a93fdd15cce..4aa7e5044374 100644 --- a/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js +++ b/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js @@ -33,6 +33,7 @@ import {createCustomEvent, listen} from '../../../src/event-helper'; import {dev, userAssert} from '../../../src/log'; import {dict} from '../../../src/core/types/object'; import {dispatchCustomEvent} from '../../../src/dom'; +import {htmlFor} from '../../../src/static-template'; import {layoutRectFromDomRect, layoutRectLtwh} from '../../../src/layout-rect'; import {numeric} from '../../../src/transition'; import { @@ -46,13 +47,14 @@ const TAG = 'amp-pan-zoom'; const DEFAULT_MAX_SCALE = 3; const MAX_ANIMATION_DURATION = 250; -const ELIGIBLE_TAGS = { - 'svg': true, - 'DIV': true, - 'AMP-IMG': true, - 'AMP-LAYOUT': true, - 'AMP-SELECTOR': true, -}; +const ELIGIBLE_TAGS = new Set([ + 'svg', + 'DIV', + 'AMP-IMG', + 'AMP-LAYOUT', + 'AMP-SELECTOR', + 'IMG', +]); /** * @extends {AMP.BaseElement} @@ -174,7 +176,7 @@ export class AmpPanZoom extends AMP.BaseElement { TAG ); userAssert( - this.elementIsSupported_(children[0]), + ELIGIBLE_TAGS.has(children[0].tagName), '%s is not supported by %s', children[0].tagName, TAG @@ -265,24 +267,15 @@ export class AmpPanZoom extends AMP.BaseElement { } } - /** - * Checks to see if an element is supported. - * @param {Element} element - * @return {boolean} - * @private - */ - elementIsSupported_(element) { - return ELIGIBLE_TAGS[element.tagName]; - } - /** * Creates zoom buttoms * @private */ createZoomButton_() { - this.zoomButton_ = this.element.ownerDocument.createElement('div'); - this.zoomButton_.classList.add('amp-pan-zoom-in-icon'); - this.zoomButton_.classList.add('amp-pan-zoom-button'); + this.zoomButton_ = htmlFor( + this.element + )`
`; + this.zoomButton_.addEventListener('click', () => { if (this.zoomButton_.classList.contains('amp-pan-zoom-in-icon')) { this.transform(0, 0, this.maxScale_); diff --git a/extensions/amp-springboard-player/0.1/amp-springboard-player.js b/extensions/amp-springboard-player/0.1/amp-springboard-player.js index f0225efa85ee..57116042cafa 100644 --- a/extensions/amp-springboard-player/0.1/amp-springboard-player.js +++ b/extensions/amp-springboard-player/0.1/amp-springboard-player.js @@ -17,6 +17,7 @@ import {PauseHelper} from '../../../src/utils/pause-helper'; import {Services} from '../../../src/services'; import {isLayoutSizeDefined} from '../../../src/layout'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setIsMediaComponent} from '../../../src/video-interface'; import {userAssert} from '../../../src/log'; @@ -154,7 +155,7 @@ class AmpSpringboardPlayer extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.win.document.createElement('img'); - this.propagateAttributes(['aria-label'], placeholder); + propagateAttributes(['aria-label'], this.element, placeholder); this.applyFillContent(placeholder); placeholder.setAttribute('placeholder', ''); placeholder.setAttribute('referrerpolicy', 'origin'); diff --git a/extensions/amp-story/1.0/amp-story-page.js b/extensions/amp-story/1.0/amp-story-page.js index d2f173e7cfe7..4d7d30baded5 100644 --- a/extensions/amp-story/1.0/amp-story-page.js +++ b/extensions/amp-story/1.0/amp-story-page.js @@ -77,6 +77,7 @@ import {isPrerenderActivePage} from './prerender-active-page'; import {listen, listenOnce} from '../../../src/event-helper'; import {CSS as pageAttachmentCSS} from '../../../build/amp-story-open-page-attachment-0.1.css'; import {prefersReducedMotion} from '../../../src/utils/media-query-props'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {px, toggle} from '../../../src/style'; import {renderPageAttachmentUI} from './amp-story-open-page-attachment'; import {renderPageDescription} from './semantic-render'; @@ -1897,7 +1898,9 @@ export class AmpStoryPage extends AMP.BaseElement { childImgNode && ampImgNode .getImpl() - .then((ampImg) => ampImg.propagateAttributes('alt', childImgNode)); + .then((impl) => + propagateAttributes('alt', impl.element, childImgNode) + ); } }); } diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 69e6684cd211..7ebebecdfcab 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -41,6 +41,7 @@ import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl import {isLayoutSizeDefined} from '../../../src/layout'; import {listen, listenOncePromise} from '../../../src/event-helper'; import {mutedOrUnmutedEvent} from '../../../src/iframe-video'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import { propagateObjectFitStyles, setImportantStyles, @@ -234,8 +235,9 @@ export class AmpVideo extends AMP.BaseElement { // Disable video preload in prerender mode. this.video_.setAttribute('preload', 'none'); this.checkA11yAttributeText_(); - this.propagateAttributes( + propagateAttributes( ATTRS_TO_PROPAGATE_ON_BUILD, + this.element, this.video_, /* opt_removeMissingAttrs */ true ); @@ -313,13 +315,18 @@ export class AmpVideo extends AMP.BaseElement { if (mutations['src']) { const urlService = this.getUrlService_(); urlService.assertHttpsUrl(element.getAttribute('src'), element); - this.propagateAttributes(['src'], dev().assertElement(this.video_)); + propagateAttributes( + ['src'], + this.element, + dev().assertElement(this.video_) + ); } const attrs = ATTRS_TO_PROPAGATE.filter( (value) => mutations[value] !== undefined ); - this.propagateAttributes( + propagateAttributes( attrs, + this.element, dev().assertElement(this.video_), /* opt_removeMissingAttrs */ true ); @@ -357,8 +364,9 @@ export class AmpVideo extends AMP.BaseElement { return Promise.resolve(); } - this.propagateAttributes( + propagateAttributes( ATTRS_TO_PROPAGATE_ON_LAYOUT, + this.element, dev().assertElement(this.video_), /* opt_removeMissingAttrs */ true ); @@ -539,7 +547,11 @@ export class AmpVideo extends AMP.BaseElement { // If the `src` of `amp-video` itself is NOT cached, set it on video if (element.hasAttribute('src') && !isCachedByCdn(element)) { urlService.assertHttpsUrl(element.getAttribute('src'), element); - this.propagateAttributes(['src'], dev().assertElement(this.video_)); + propagateAttributes( + ['src'], + this.element, + dev().assertElement(this.video_) + ); } sources.forEach((source) => { diff --git a/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js b/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js index f9dd4502e1c2..06a531f7762d 100644 --- a/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js +++ b/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js @@ -31,6 +31,7 @@ import { import {getData, listen} from '../../../src/event-helper'; import {getIframe} from '../../../src/3p-frame'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; +import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; const TAG = 'amp-viqeo-player'; @@ -205,7 +206,7 @@ class AmpViqeoPlayer extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.element.ownerDocument.createElement('img'); - this.propagateAttributes(['aria-label'], placeholder); + propagateAttributes(['aria-label'], this.element, placeholder); this.applyFillContent(placeholder); placeholder.setAttribute('loading', 'lazy'); placeholder.setAttribute('placeholder', ''); diff --git a/extensions/amp-youtube/0.1/amp-youtube.js b/extensions/amp-youtube/0.1/amp-youtube.js index 95a4a335a804..510e1ea06159 100644 --- a/extensions/amp-youtube/0.1/amp-youtube.js +++ b/extensions/amp-youtube/0.1/amp-youtube.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'; import {setStyles} from '../../../src/style'; const TAG = 'amp-youtube'; @@ -497,7 +498,7 @@ class AmpYoutube extends AMP.BaseElement { // the object-fit: cover. 'visibility': 'hidden', }); - this.propagateAttributes(['aria-label'], imgPlaceholder); + propagateAttributes(['aria-label'], this.element, imgPlaceholder); // TODO(mkhatib): Maybe add srcset to allow the browser to // load the needed size or even better match YTPlayer logic for loading // player thumbnails for different screen sizes for a cache win! diff --git a/src/base-element.js b/src/base-element.js index 44eca66dc87e..fa61df7fa4a6 100644 --- a/src/base-element.js +++ b/src/base-element.js @@ -742,29 +742,6 @@ export class BaseElement { } } - /** - * Utility method that propagates attributes from this element - * to the given element. - * If `opt_removeMissingAttrs` is true, then also removes any specified - * attributes that are missing on this element from the target element. - * @param {string|!Array} attributes - * @param {!Element} element - * @param {boolean=} opt_removeMissingAttrs - * @public @final - */ - propagateAttributes(attributes, element, opt_removeMissingAttrs) { - attributes = isArray(attributes) ? attributes : [attributes]; - for (let i = 0; i < attributes.length; i++) { - const attr = attributes[i]; - const val = this.element.getAttribute(attr); - if (null !== val) { - element.setAttribute(attr, val); - } else if (opt_removeMissingAttrs) { - element.removeAttribute(attr); - } - } - } - /** * Utility method that forwards the given list of non-bubbling events * from the given element to this element as custom events with the same name. diff --git a/src/core/dom/propagate-attributes.js b/src/core/dom/propagate-attributes.js new file mode 100644 index 000000000000..26640f7102c9 --- /dev/null +++ b/src/core/dom/propagate-attributes.js @@ -0,0 +1,44 @@ +/** + * Copyright 2021 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {arrayOrSingleItemToArray} from '../types/array'; + +/** + * Utility method that propagates attributes from a source element + * to an updateable element. + * If `opt_removeMissingAttrs` is true, then also removes any specified + * attributes that are missing on the source element from the updateable element. + * @param {string|!Array} attributes + * @param {!Element} sourceElement + * @param {!Element} updateElement + * @param {boolean=} opt_removeMissingAttrs + */ +export function propagateAttributes( + attributes, + sourceElement, + updateElement, + opt_removeMissingAttrs +) { + const attrs = arrayOrSingleItemToArray(attributes); + for (const attr of attrs) { + const val = sourceElement.getAttribute(attr); + if (null !== val) { + updateElement.setAttribute(attr, val); + } else if (opt_removeMissingAttrs) { + updateElement.removeAttribute(attr); + } + } +} diff --git a/src/iframe-video.js b/src/iframe-video.js index b7ae5e1be0a5..7936e6e6ed66 100644 --- a/src/iframe-video.js +++ b/src/iframe-video.js @@ -19,6 +19,7 @@ import {dev} from './log'; import {dispatchCustomEvent} from './dom'; import {htmlFor} from './static-template'; import {isArray, isObject} from './core/types'; +import {propagateAttributes} from './core/dom/propagate-attributes'; import {tryParseJson} from './core/types/object/json'; /** @enum {string} */ @@ -90,7 +91,7 @@ export function createFrameFor(video, src, opt_name, opt_sandbox) { // Will propagate for every component, but only validation rules will actually // allow the attribute to be set. - video.propagateAttributes(['referrerpolicy'], frame); + propagateAttributes(['referrerpolicy'], video.element, frame); frame.src = Services.urlForDoc(element).assertHttpsUrl(src, element); diff --git a/test/fixtures/amp-pan-zoom.html b/test/fixtures/amp-pan-zoom.html index 065ea7154968..adfff013fe32 100644 --- a/test/fixtures/amp-pan-zoom.html +++ b/test/fixtures/amp-pan-zoom.html @@ -11,7 +11,7 @@ diff --git a/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html b/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html index a53dcaf7a300..b151b2aab2c7 100644 --- a/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html +++ b/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html @@ -43,6 +43,18 @@ layout="responsive" sizes="(min-width: 500px) 450px, 100vw" > + amp #0 + + + + + + + + + diff --git a/test/manual/amp-image-slider.amp.html b/test/manual/amp-image-slider.amp.html index d5c20389e929..63f06a893f3d 100644 --- a/test/manual/amp-image-slider.amp.html +++ b/test/manual/amp-image-slider.amp.html @@ -12,6 +12,12 @@ margin: 0 auto; } + img.custom-fill { + object-fit: cover; + width: 100%; + height: 100%; + } + .flex { display: flex; justify-content: space-between @@ -104,30 +110,60 @@

P0 TESTS

NO labels; NO whatever modifications

- - + + + + +

NO labels; NO whatever modifications using ImageElements

+ + +

Labels with NO positioning rules specified (default to both top left)

- - + + +
BEFORE
+
AFTER
+ + +

Labels with NO positioning rules specified (default to both top left) using ImageElements

+ + +
BEFORE
AFTER

Labels with positioning rules specified (both should be at the center)

- - + + +
BEFORE
+
AFTER
+ + +

Labels with positioning rules specified (both should be at the center) using ImageElements

+ + +
BEFORE
AFTER

Has alt on amp-imgs. ARIA order: [label content], [alt text if present], left/right image

- - + + +
BEFORE
+
AFTER
+ + +

Has alt on amp-imgs. ARIA order: [label content], [alt text if present], left/right image using ImageElements

+ + Mountain + Ocean
BEFORE
AFTER
 @@ -136,14 +172,28 @@

Customizable Hints

Disable default viewport behavior (hint would NOT reappear after the slider interacted, out of viewport, and then back into viewport)

- - + + + + +

Disable default viewport behavior (hint would NOT reappear after the slider interacted, out of viewport, and then back into viewport) using ImageElements

+ + Mountain + Ocean

Fully customized hint (replace with triangles)

- - + + +
BEFORE
+
AFTER
+ + +

Fully customized hint (replace with triangles) using ImageElements

+ + +
BEFORE
AFTER
 @@ -152,8 +202,8 @@

Actions

seekTo test (currently, seekTo is not considered as user interaction)

- - + +
@@ -161,18 +211,41 @@

Actions

+

seekTo test (currently, seekTo is not considered as user interaction) using ImageElements

+ + + + +
+ + + +
+

Customization

Set initial-slider-position to 0.3

- - + + + + +

Set initial-slider-position to 0.3 using ImageElements

+ + +

Set step-size to 0.2

- - + + + + +

Set step-size to 0.2 using ImageElements

+ + + diff --git a/test/manual/amp-lightbox-gallery.amp.html b/test/manual/amp-lightbox-gallery.amp.html index 76339ac6fc54..5ba66b715e88 100644 --- a/test/manual/amp-lightbox-gallery.amp.html +++ b/test/manual/amp-lightbox-gallery.amp.html @@ -10,15 +10,15 @@