diff --git a/build-system/test-configs/forbidden-terms.js b/build-system/test-configs/forbidden-terms.js index 9d92d423b90d..dbce427e55ea 100644 --- a/build-system/test-configs/forbidden-terms.js +++ b/build-system/test-configs/forbidden-terms.js @@ -875,10 +875,13 @@ const forbiddenTermsSrcInclusive = { 'src/service/resources-impl.js', 'src/service/variable-source.js', 'src/validator-integration.js', - 'extensions/amp-image-lightbox/0.1/amp-image-lightbox.js', 'extensions/amp-analytics/0.1/transport.js', - 'extensions/amp-web-push/0.1/iframehost.js', + 'extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js', + 'extensions/amp-image-lightbox/0.1/amp-image-lightbox.js', + 'extensions/amp-image-slider/0.1/amp-image-slider.js', + 'extensions/amp-image-viewer/0.1/amp-image-viewer.js', 'extensions/amp-recaptcha-input/0.1/amp-recaptcha-service.js', + 'extensions/amp-web-push/0.1/iframehost.js', ], }, '\\.getTime\\(\\)': { diff --git a/builtins/amp-img/amp-img.js b/builtins/amp-img/amp-img.js index 989d70b9ac27..75bb44231d92 100644 --- a/builtins/amp-img/amp-img.js +++ b/builtins/amp-img/amp-img.js @@ -21,6 +21,7 @@ import {Services} from '#service'; import {dev} from '../../src/log'; import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '#core/dom/img'; import {listen} from '../../src/event-helper'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {propagateObjectFitStyles, setImportantStyles} from '#core/dom/style'; import {registerElement} from '#service/custom-element-registry'; import {removeElement} from '#core/dom'; @@ -33,7 +34,7 @@ const TAG = 'amp-img'; * Attributes to propagate to internal image when changed externally. * @type {!Array} */ -const ATTRIBUTES_TO_PROPAGATE = [ +export const ATTRIBUTES_TO_PROPAGATE = [ 'alt', 'aria-describedby', 'aria-label', @@ -131,8 +132,9 @@ export class AmpImg extends BaseElement { this.element ); } - this.propagateAttributes( + propagateAttributes( attrs, + this.element, this.img_, /* opt_removeMissingAttrs */ true ); @@ -217,7 +219,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 d4dfaa698d51..972a91454552 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 c90cb2aa36e8..022945b1987b 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 fabb0da88ae3..cf2a67705ba3 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 '#core/dom/propagate-attributes'; import {propagateObjectFitStyles} from '#core/dom/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_); 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 e990b09b57f1..92fd5a1ed132 100644 --- a/extensions/amp-audio/0.1/amp-audio.js +++ b/extensions/amp-audio/0.1/amp-audio.js @@ -31,6 +31,7 @@ import { import {dev, user} from '../../../src/log'; import {getMode} from '../../../src/mode'; import {listen} from '../../../src/event-helper'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {setIsMediaComponent} from '../../../src/video-interface'; import {triggerAnalyticsEvent} from '../../../src/analytics'; @@ -88,7 +89,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']; @@ -122,7 +127,7 @@ export class AmpAudio extends AMP.BaseElement { if (src) { assertHttpsUrl(src, this.element); } - this.propagateAttributes( + propagateAttributes( [ 'src', 'preload', @@ -134,6 +139,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 b9caaa3f6cfc..46c5b942a9b0 100644 --- a/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js +++ b/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js @@ -29,6 +29,7 @@ import {Services} from '#service'; import {closestAncestorElementBySelector} from '#core/dom/query'; import {dev} from '../../../src/log'; import {dispatchCustomEvent} from '#core/dom'; +import {loadPromise} from '../../../src/event-helper'; import {measureIntersectionNoRoot} from '../../../src/utils/intersection-no-root'; import {toArray} from '#core/types/array'; import {tryParseJson} from '#core/types/object/json'; @@ -112,33 +113,40 @@ 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, ampdoc) ); } /** * @param {!Element} element + * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc * @return {boolean} */ - static meetsTreeShapeCriteria(element) { + static meetsTreeShapeCriteria(element, ampdoc) { + if ( + element.tagName === 'IMG' && + closestAncestorElementBySelector(element, 'amp-img') + ) { + // Images that are a child of an AMP-IMG do not need additional treatment. + return false; + } + const disabledSelector = `${DISABLED_ANCESTORS},${DISABLED_BY_ATTR}`; const disabledAncestor = closestAncestorElementBySelector( element, @@ -147,22 +155,23 @@ export class Criteria { if (disabledAncestor) { return false; } - const actions = Services.actionServiceForDoc(element); + const actions = Services.actionServiceForDoc(ampdoc || element); return !actions.hasResolvableAction(element, 'tap'); } /** * @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 {naturalHeight, naturalWidth} = getMaxNaturalDimensions( - dev().assertElement(element.querySelector('img')) + dev().assertElement(element.querySelector('img') || element) ); - const viewport = Services.viewportForDoc(element); + const viewport = Services.viewportForDoc(ampdoc); const {height: vh, width: vw} = viewport.getSize(); return meetsSizingCriteria( @@ -271,11 +280,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(','); } /** @@ -283,6 +297,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) ); @@ -296,7 +313,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. @@ -316,7 +333,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'); } @@ -337,7 +354,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']; @@ -367,10 +384,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) @@ -438,13 +453,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 {height, width} = boundingClientRect; - if (!Criteria.meetsAll(candidate, width, height)) { + if (!Criteria.meetsAll(candidate, ampdoc, width, height)) { return; } dev().info(TAG, 'apply', candidate); @@ -473,7 +492,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 98c850d9044b..9ceda4489a77 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,75 @@ describes.realWin( it(`${accepts ? 'accepts' : 'rejects'} ${accepts || rejects}`, () => { [ - html` `, - html` -

- -
- `, - html` -
+ { + markup: html` + + `, + tagName: 'AMP-IMG', + }, + { + markup: html`
- + + +
-
- `, + `, + tagName: 'AMP-IMG', + candidate: '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 candidate = firstElementLeaf(scenario); + const scenario = maybeWrap(unwrapped.markup); + const candidate = unwrapped.candidate + ? scenario.querySelector(unwrapped.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 618fce1f636c..ea0c871242d9 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 { import {getData, listen} from '../../../src/event-helper'; import {htmlFor} from '#core/dom/static-template'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; +import {propagateAttributes} from '#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); 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 969b46373d51..eedb5233dff4 100644 --- a/extensions/amp-gfycat/0.1/amp-gfycat.js +++ b/extensions/amp-gfycat/0.1/amp-gfycat.js @@ -26,6 +26,7 @@ import { } from '#core/dom'; import {getData, listen} from '../../../src/event-helper'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; +import {propagateAttributes} from '#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_); 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 0563580bcb8d..010ce1a323fc 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 '#service'; import {addParamToUrl} from '../../../src/url'; import {applyFillContent, isLayoutSizeDefined} from '#core/dom/layout'; import {dev, userAssert} from '../../../src/log'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {removeElement} from '#core/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 1cf1001573ec..579879972b08 100644 --- a/extensions/amp-iframe/0.1/amp-iframe.js +++ b/extensions/amp-iframe/0.1/amp-iframe.js @@ -40,6 +40,7 @@ import {isAdPositionAllowed} from '../../../src/ad-helper'; import {isExperimentOn} from '#experiments'; import {moveLayoutRect} from '#core/math/layout-rect'; import {parseJson} from '#core/types/object/json'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {removeElement} from '#core/dom'; import {removeFragment} from '../../../src/url'; import {setStyle} from '#core/dom/style'; @@ -418,7 +419,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. @@ -623,7 +624,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 62620dd7cc35..730b8e4b0f60 100644 --- a/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js +++ b/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js @@ -40,16 +40,14 @@ import { layoutRectLtwh, moveLayoutRect, } from '#core/math/layout-rect'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {setStyles, toggle} from '#core/dom/style'; import {srcsetFromElement} from '#core/dom/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']; @@ -254,9 +252,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. @@ -870,8 +874,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 2314f8ac35d7..e09c80c7208a 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 @@ -66,19 +66,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'); @@ -88,23 +82,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; @@ -310,6 +350,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 37f500a7934d..277c616b9fb9 100644 --- a/extensions/amp-image-slider/0.1/amp-image-slider.js +++ b/extensions/amp-image-slider/0.1/amp-image-slider.js @@ -22,14 +22,17 @@ import {Services} from '#service'; import {SwipeXRecognizer} from '../../../src/gesture-recognizers'; import {clamp} from '#core/math'; import {dev, user, userAssert} from '../../../src/log'; +import {htmlFor} from '#core/dom/static-template'; import {isLayoutSizeDefined} from '#core/dom/layout'; -import {listen} from '../../../src/event-helper'; +import {listen, loadPromise} from '../../../src/event-helper'; import { observeWithSharedInOb, unobserveWithSharedInOb, } from '../../../src/viewport-observer'; import {realChildElements} from '#core/dom/query'; -import {setStyles} from '#core/dom/style'; +import {setStyle} from '#core/dom/style'; + +const VALID_IMAGE_TAGNAMES = new Set(['AMP-IMG', 'IMG']); export class AmpImageSlider extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -43,10 +46,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; @@ -60,16 +64,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} */ @@ -113,22 +113,21 @@ export class AmpImageSlider extends AMP.BaseElement { buildCallback() { const children = realChildElements(this.element); - 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')) { @@ -144,18 +143,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_(); @@ -188,8 +194,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); @@ -197,9 +203,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 } }); } @@ -227,7 +231,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( @@ -244,14 +248,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_); } @@ -296,8 +297,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) @@ -667,7 +673,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); @@ -694,9 +700,11 @@ export class AmpImageSlider extends AMP.BaseElement { * @private */ updateTranslateX_(element, percentage) { - setStyles(dev().assertElement(element), { - transform: `translateX(${percentage * 100}%)`, - }); + setStyle( + dev().assertElement(element), + 'transform', + `translateX(${percentage * 100}%)` + ); } /** @override */ @@ -709,45 +717,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 fac0dd2ec4b3..d1e80cee6fbf 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 778f2a6e0f5f..43b744a1a61f 100644 --- a/extensions/amp-image-viewer/0.1/amp-image-viewer.js +++ b/extensions/amp-image-viewer/0.1/amp-image-viewer.js @@ -38,7 +38,7 @@ import { realChildElements, } from '#core/dom/query'; import {continueMotion} from '../../../src/motion'; -import {createCustomEvent} from '../../../src/event-helper'; +import {createCustomEvent, loadPromise} from '../../../src/event-helper'; import {dev, userAssert} from '../../../src/log'; import { expandLayoutRect, @@ -50,6 +50,7 @@ import { observeContentSize, unobserveContentSize, } from '#core/dom/size-observer'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {setStyles} from '#core/dom/style'; import {srcsetFromElement} from '#core/dom/srcset'; @@ -58,10 +59,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 */ @@ -126,7 +124,7 @@ export class AmpImageViewer extends AMP.BaseElement { this.motion_ = null; /** @private {?Element} */ - this.sourceAmpImage_ = null; + this.sourceImage_ = null; /** @private {?Promise} */ this.loadPromise_ = null; @@ -151,9 +149,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 ); } @@ -181,14 +179,16 @@ 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.tagName === 'IMG' + ? loadPromise(img) + : 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 @@ -273,7 +273,7 @@ export class AmpImageViewer extends AMP.BaseElement { * @private */ elementIsSupported_(element) { - return ELIGIBLE_TAGS[element.tagName.toLowerCase()]; + return ELIGIBLE_TAGS.has(element.tagName); } /** @@ -309,9 +309,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_); @@ -322,11 +322,17 @@ 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_); - }); + if (img.tagName === 'IMG') { + propagateAttributes(ARIA_ATTRIBUTES, img, this.image_); + return Promise.resolve(); + } + 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 283229a92dfc..0ff8b15045a1 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 '#core/types/object'; import {htmlFor} from '#core/dom/static-template'; import {isLayoutSizeDefined} from '#core/dom/layout'; import {matches, scopedQuerySelector} from '#core/dom/query'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {setStyle} from '#core/dom/style'; /** @@ -242,11 +243,13 @@ export class AmpInlineGalleryThumbnails extends AMP.BaseElement { > `; - thumbnails.forEach((t) => this.carousel_.appendChild(t)); + for (const thumbnail of thumbnails) { + this.carousel_.appendChild(thumbnail); + } // 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 bcf6fc474d1f..b0999af3e535 100644 --- a/extensions/amp-jwplayer/0.1/amp-jwplayer.js +++ b/extensions/amp-jwplayer/0.1/amp-jwplayer.js @@ -41,6 +41,7 @@ import {getData, listen} from '../../../src/event-helper'; import {getMode} from '../../../src/mode'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; import {once} from '#core/types/function'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; const JWPLAYER_EVENTS = { 'ready': VideoEvents.LOAD, @@ -348,7 +349,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); 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 c1ffed5a4ea8..9f31e3e1545b 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 {applyFillContent, isLayoutSizeDefined} from '#core/dom/layout'; import {dict} from '#core/types/object'; import {getDataParamsFromAttributes} from '#core/dom'; +import {propagateAttributes} from '#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); 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 70d6a1994f57..b209b7d6efb0 100644 --- a/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js +++ b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js @@ -274,11 +274,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); @@ -307,7 +307,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_)` `; @@ -360,8 +360,10 @@ export class AmpLightboxGallery extends AMP.BaseElement { return this.mutateElement(() => { const {length} = this.elementsMetadata_[lightboxGroupId]; this.maybeEnableMultipleItemControls_(length); - const owners = Services.ownersForDoc(this.element); - owners./*OK*/ scheduleUnlayout(this.element, this.carousel_); + Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout( + this.element, + this.carousel_ + ); toggle(this.carousel_, true); }); } @@ -813,7 +815,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'); @@ -933,6 +935,13 @@ export class AmpLightboxGallery extends AMP.BaseElement { }; const mutate = () => { + if (enter) { + Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout( + this.element, + this.carousel_ + ); + } + toggle(carousel, enter); // Undo opacity 0 from `openLightboxGallery_` setStyle(this.element, 'opacity', ''); @@ -1004,6 +1013,10 @@ export class AmpLightboxGallery extends AMP.BaseElement { fade_(fadeIn) { return this.mutateElement(() => { if (fadeIn) { + Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout( + this.element, + this.carousel_ + ); toggle(dev().assertElement(this.carousel_), true); toggle(this.element, true); } @@ -1247,6 +1260,10 @@ export class AmpLightboxGallery extends AMP.BaseElement { closeGallery_() { return this.mutateElement(() => { this.container_.removeAttribute('gallery-view'); + Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout( + this.element, + this.carousel_ + ); toggle(dev().assertElement(this.carousel_), true); this.updateDescriptionBox_(); }); @@ -1334,11 +1351,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 dfd9f3cde96c..d7a6655548f3 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 @@ -39,21 +39,16 @@ import {map} from '#core/types/object'; import {srcsetFromElement, srcsetFromSrc} from '#core/dom/srcset'; import {toArray} from '#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'; @@ -127,9 +122,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(); } /** @@ -167,7 +162,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); }); @@ -180,7 +177,7 @@ export class LightboxManager { * @private */ baseElementIsSupported_(element) { - return LIGHTBOX_ELIGIBLE_TAGS[element.tagName]; + return LIGHTBOX_ELIGIBLE_TAGS.has(element.tagName); } /** @@ -204,11 +201,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); }); }); @@ -219,11 +216,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'; @@ -406,7 +403,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); } } @@ -420,7 +417,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 bf992a9b675d..38b499817b57 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 '#core/types/object'; import {dispatchCustomEvent} from '#core/dom'; +import {htmlFor} from '#core/dom/static-template'; import {layoutRectFromDomRect, layoutRectLtwh} from '#core/math/layout-rect'; import {numeric} from '../../../src/transition'; import { @@ -47,13 +48,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} @@ -175,7 +177,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 @@ -266,24 +268,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 a42909005b7e..b1301d26c732 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 '#core/dom/video/pause-helper'; import {Services} from '#service'; import {applyFillContent, isLayoutSizeDefined} from '#core/dom/layout'; +import {propagateAttributes} from '#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); 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 dc34a2d09cd2..33e240a2149e 100644 --- a/extensions/amp-story/1.0/amp-story-page.js +++ b/extensions/amp-story/1.0/amp-story-page.js @@ -75,6 +75,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 '#core/dom/media-query-props'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {px, toggle} from '#core/dom/style'; import {renderPageAttachmentUI} from './amp-story-open-page-attachment'; import {renderPageDescription} from './semantic-render'; @@ -1910,7 +1911,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 aa21f9627a67..08fd72b74518 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -45,6 +45,7 @@ import {htmlFor} from '#core/dom/static-template'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; import {listen, listenOncePromise} from '../../../src/event-helper'; import {mutedOrUnmutedEvent} from '../../../src/iframe-video'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import { propagateObjectFitStyles, setImportantStyles, @@ -238,8 +239,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 ); @@ -317,13 +319,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 ); @@ -361,8 +368,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 ); @@ -543,7 +551,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 8782cac586f2..bc9586cbe73c 100644 --- a/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js +++ b/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js @@ -28,6 +28,7 @@ import { import {getData, listen} from '../../../src/event-helper'; import {getIframe} from '../../../src/3p-frame'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {redispatch} from '../../../src/iframe-video'; import {removeElement} from '#core/dom'; @@ -204,7 +205,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); 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 c817e057b0c2..19ccc09beb99 100644 --- a/extensions/amp-youtube/0.1/amp-youtube.js +++ b/extensions/amp-youtube/0.1/amp-youtube.js @@ -44,6 +44,7 @@ import { import {getData, listen} from '../../../src/event-helper'; import {htmlFor} from '#core/dom/static-template'; import {installVideoManagerForDoc} from '#service/video-manager-impl'; +import {propagateAttributes} from '#core/dom/propagate-attributes'; import {setStyles} from '#core/dom/style'; const TAG = 'amp-youtube'; @@ -499,7 +500,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 2d20ec25262e..fe6add699b25 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..a115c2c27421 --- /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 '#core/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 a97099c1f302..15581576c2dd 100644 --- a/src/iframe-video.js +++ b/src/iframe-video.js @@ -20,6 +20,7 @@ import {dev} from './log'; import {dispatchCustomEvent} from './core/dom'; import {htmlFor} from './core/dom/static-template'; import {isArray, isObject} from './core/types'; +import {propagateAttributes} from './core/dom/propagate-attributes'; import {tryParseJson} from './core/types/object/json'; /** @enum {string} */ @@ -91,7 +92,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 58c926d4305b..9f85d6823b87 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 d78c198bddc3..eb6a6bce14d8 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 722aaef12bd3..d0115ae52fed 100644 --- a/test/manual/amp-lightbox-gallery.amp.html +++ b/test/manual/amp-lightbox-gallery.amp.html @@ -10,15 +10,15 @@