From a7c7fdf71e3d6a4c7f83d28130c094c195986560 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Thu, 27 May 2021 15:13:43 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"=E2=99=BB=EF=B8=8F=20Support=20img=20?= =?UTF-8?q?element=20(#34028)"=20(#34589)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 5a0936f052897b784ae7cef29ac77d8e0b57280d. (cherry picked from commit 3e8f1fc2c0e13e6debc75030cb82aa9234307efd) (cherry picked from commit f0b55df25cce96a8779a06524ff3f7754e12daea) --- build-system/test-configs/forbidden-terms.js | 2 - builtins/amp-img/amp-img.js | 6 +- examples/amp-lightbox.amp.html | 2 - examples/image-lightbox.amp.html | 7 - extensions/amp-anim/0.1/amp-anim.js | 6 +- extensions/amp-audio/0.1/amp-audio.js | 10 +- .../0.1/amp-auto-lightbox.js | 70 ++++---- .../0.1/test/test-amp-auto-lightbox.js | 61 ++----- .../amp-brid-player/0.1/amp-brid-player.js | 3 +- extensions/amp-gfycat/0.1/amp-gfycat.js | 3 +- .../0.1/amp-google-document-embed.js | 10 +- extensions/amp-iframe/0.1/amp-iframe.js | 5 +- .../0.1/amp-image-lightbox.js | 47 +++--- .../0.1/test/test-amp-image-lightbox.js | 75 ++------- .../amp-image-slider/0.1/amp-image-slider.js | 157 +++++++++--------- .../test/integration/test-amp-image-slider.js | 80 +-------- .../amp-image-viewer/0.1/amp-image-viewer.js | 32 ++-- .../0.1/amp-inline-gallery-thumbnails.js | 3 +- extensions/amp-jwplayer/0.1/amp-jwplayer.js | 3 +- .../0.1/amp-kaltura-player.js | 3 +- .../0.1/amp-lightbox-gallery.js | 24 +-- .../0.1/service/lightbox-manager-impl.js | 41 ++--- extensions/amp-pan-zoom/0.1/amp-pan-zoom.js | 40 +++-- .../0.1/amp-springboard-player.js | 3 +- extensions/amp-story/1.0/amp-story-page.js | 5 +- extensions/amp-video/0.1/amp-video.js | 22 +-- .../amp-viqeo-player/0.1/amp-viqeo-player.js | 3 +- extensions/amp-youtube/0.1/amp-youtube.js | 3 +- src/base-element.js | 23 +++ src/core/dom/propagate-attributes.js | 44 ----- src/iframe-video.js | 3 +- test/fixtures/amp-pan-zoom.html | 4 +- .../amp-auto-lightbox/amp-auto-lightbox.html | 12 -- test/manual/amp-carousel.amp.html | 9 - test/manual/amp-image-slider.amp.html | 109 ++---------- test/manual/amp-lightbox-gallery.amp.html | 32 ++-- .../manual/amp-pan-zoom/amp-pan-zoom.amp.html | 4 - test/manual/img/animated.gif | Bin 541343 -> 0 bytes .../core/dom/test-propagate-attributes.js | 75 --------- test/unit/test-amp-img.js | 6 +- test/unit/test-base-element.js | 31 ++++ 41 files changed, 354 insertions(+), 724 deletions(-) delete mode 100644 src/core/dom/propagate-attributes.js delete mode 100644 test/manual/img/animated.gif delete mode 100644 test/unit/core/dom/test-propagate-attributes.js diff --git a/build-system/test-configs/forbidden-terms.js b/build-system/test-configs/forbidden-terms.js index a435bd89735c7..ce6ab4e782f4d 100644 --- a/build-system/test-configs/forbidden-terms.js +++ b/build-system/test-configs/forbidden-terms.js @@ -868,8 +868,6 @@ const forbiddenTermsSrcInclusive = { 'extensions/amp-analytics/0.1/transport.js', 'extensions/amp-web-push/0.1/iframehost.js', 'extensions/amp-recaptcha-input/0.1/amp-recaptcha-service.js', - 'extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js', - 'extensions/amp-image-slider/0.1/amp-image-slider.js', ], }, '\\.getTime\\(\\)': { diff --git a/builtins/amp-img/amp-img.js b/builtins/amp-img/amp-img.js index b405a2e131479..dc6cd6518e2cf 100644 --- a/builtins/amp-img/amp-img.js +++ b/builtins/amp-img/amp-img.js @@ -21,7 +21,6 @@ import {Services} from '../../src/services'; import {dev} from '../../src/log'; import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../../src/utils/img'; import {listen} from '../../src/event-helper'; -import {propagateAttributes} from '../../src/core/dom/propagate-attributes'; import {propagateObjectFitStyles, setImportantStyles} from '../../src/style'; import {registerElement} from '../../src/service/custom-element-registry'; import {removeElement, scopedQuerySelector} from '../../src/dom'; @@ -131,9 +130,8 @@ export class AmpImg extends BaseElement { this.element ); } - propagateAttributes( + this.propagateAttributes( attrs, - this.element, this.img_, /* opt_removeMissingAttrs */ true ); @@ -218,7 +216,7 @@ export class AmpImg extends BaseElement { // It is important to call this before setting `srcset` attribute. this.maybeGenerateSizes_(/* sync setAttribute */ true); - propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, this.img_); + this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_); this.propagateDataset(this.img_); if (!IS_ESM) { guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_); diff --git a/examples/amp-lightbox.amp.html b/examples/amp-lightbox.amp.html index 30c236916a3c2..0dcfa9652c180 100644 --- a/examples/amp-lightbox.amp.html +++ b/examples/amp-lightbox.amp.html @@ -63,8 +63,6 @@

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.

- -

- -
- `, - tagName: 'AMP-IMG', - }, - { - markup: html` -
-
- -
-
- `, - tagName: 'AMP-IMG', - }, - { - markup: html` `, - tagName: 'IMG', - }, - { - markup: html` -
- -
- `, - tagName: 'IMG', - }, - { - markup: html` + html` `, + html` +
+ +
+ `, + html` +
-
- -
+
- `, - tagName: 'IMG', - }, +
+ `, ].forEach((unwrapped) => { - maybeMutate(unwrapped.markup); + maybeMutate(unwrapped); - const scenario = maybeWrap(unwrapped.markup); + const scenario = maybeWrap(unwrapped); const candidate = firstElementLeaf(scenario); env.win.document.body.appendChild(scenario); expect(candidate).to.be.ok; - expect(candidate.tagName).to.equal(unwrapped.tagName); + expect(candidate.tagName).to.equal('AMP-IMG'); expect( Criteria.meetsTreeShapeCriteria(candidate), 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 5716e8ecb40a8..4b29157fd4dd6 100644 --- a/extensions/amp-brid-player/0.1/amp-brid-player.js +++ b/extensions/amp-brid-player/0.1/amp-brid-player.js @@ -42,7 +42,6 @@ import {getData, listen} from '../../../src/event-helper'; import {htmlFor} from '../../../src/static-template'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isLayoutSizeDefined} from '../../../src/layout'; -import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; const TAG = 'amp-brid-player'; @@ -248,7 +247,7 @@ class AmpBridPlayer extends AMP.BaseElement { `; - propagateAttributes(['aria-label'], this.element, placeholder); + this.propagateAttributes(['aria-label'], 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 b2f6f8d2858fc..33d55715deed4 100644 --- a/extensions/amp-gfycat/0.1/amp-gfycat.js +++ b/extensions/amp-gfycat/0.1/amp-gfycat.js @@ -26,7 +26,6 @@ import { import {getData, listen} from '../../../src/event-helper'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isLayoutSizeDefined} from '../../../src/layout'; -import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; const TAG = 'amp-gfycat'; @@ -91,7 +90,7 @@ class AmpGfycat extends AMP.BaseElement { const placeholder = this.win.document.createElement('img'); const videoid = dev().assertString(this.videoid_); this.applyFillContent(placeholder); - propagateAttributes(['alt', 'aria-label'], this.element, placeholder); + this.propagateAttributes(['alt', 'aria-label'], placeholder); placeholder.setAttribute('loading', 'lazy'); placeholder.setAttribute('placeholder', ''); placeholder.setAttribute('referrerpolicy', 'origin'); 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 6ccaecc02c630..45ba247a20821 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,7 +31,6 @@ 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'; @@ -91,7 +90,7 @@ export class AmpDriveViewer extends AMP.BaseElement { iframe.setAttribute('frameborder', '0'); iframe.setAttribute('allowfullscreen', ''); - propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, iframe); + this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, iframe); iframe.src = this.getSrc_(this.element.getAttribute('src')); @@ -106,12 +105,7 @@ export class AmpDriveViewer extends AMP.BaseElement { (value) => mutations[value] !== undefined ); const iframe = dev().assertElement(this.iframe_); - propagateAttributes( - attrs, - this.element, - iframe, - /* opt_removeMissingAttrs */ true - ); + this.propagateAttributes(attrs, 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 d649923560aac..a4ca12952ed72 100644 --- a/extensions/amp-iframe/0.1/amp-iframe.js +++ b/extensions/amp-iframe/0.1/amp-iframe.js @@ -36,7 +36,6 @@ 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'; @@ -415,7 +414,7 @@ export class AmpIframe extends AMP.BaseElement { setStyle(iframe, 'zIndex', -1); } - propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.element, iframe); + this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, iframe); // TEMPORARY: disable `allow=autoplay` // This is a workaround for M72-M74 user-activation breakage. @@ -620,7 +619,7 @@ export class AmpIframe extends AMP.BaseElement { } if (this.iframe_ && mutations['title']) { // only propagating title because propagating all causes e2e error: - propagateAttributes(['title'], this.element, this.iframe_); + this.propagateAttributes(['title'], 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 805044e113702..8f40d2771b6c2 100644 --- a/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js +++ b/extensions/amp-image-lightbox/0.1/amp-image-lightbox.js @@ -38,15 +38,17 @@ import { layoutRectFromDomRect, layoutRectLtwh, moveLayoutRect, -} from '../../../src/layout-rect'; -import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; +} from '../../../src/core/math/layout-rect'; import {setStyles, toggle} from '../../../src/style'; import {srcsetFromElement} from '../../../src/srcset'; const TAG = 'amp-image-lightbox'; -/** @private @const {!Set} */ -const SUPPORTED_ELEMENTS_ = new Set(['amp-img', 'amp-anim', 'img']); +/** @private @const {!Object} */ +const SUPPORTED_ELEMENTS_ = { + 'amp-img': true, + 'amp-anim': true, +}; /** @private @const */ const ARIA_ATTRIBUTES = ['aria-label', 'aria-describedby', 'aria-labelledby']; @@ -251,15 +253,9 @@ export class ImageViewer { this.setSourceDimensions_(sourceElement, sourceImage); this.srcset_ = srcsetFromElement(sourceElement); - if (sourceElement.tagName.toLowerCase() === 'img') { - propagateAttributes(ARIA_ATTRIBUTES, sourceElement, this.image_); - } else { - sourceElement - .getImpl() - .then((impl) => - propagateAttributes(ARIA_ATTRIBUTES, impl.element, this.image_) - ); - } + sourceElement.getImpl().then((elem) => { + elem.propagateAttributes(ARIA_ATTRIBUTES, this.image_); + }); if (sourceImage && isLoaded(sourceImage) && sourceImage.src) { // Set src provisionally to the known loaded value for fast display. @@ -612,9 +608,12 @@ export class ImageViewer { const newPosX = this.boundX_(this.startX_ + deltaX * newScale, false); const newPosY = this.boundY_(this.startY_ + deltaY * newScale, false); - return /** @type {!Promise|undefined} */ ( - this.set_(newScale, newPosX, newPosY, animate) - ); + return /** @type {!Promise|undefined} */ (this.set_( + newScale, + newPosX, + newPosY, + animate + )); } /** @@ -787,11 +786,9 @@ class AmpImageLightbox extends AMP.BaseElement { /** @override */ buildCallback() { /** If the element is in an email document, allow its `open` action. */ - Services.actionServiceForDoc(this.element).addToAllowlist( - 'AMP-IMAGE-LIGHTBOX', - 'open', - ['email'] - ); + Services.actionServiceForDoc( + this.element + ).addToAllowlist('AMP-IMAGE-LIGHTBOX', 'open', ['email']); } /** @@ -827,8 +824,9 @@ class AmpImageLightbox extends AMP.BaseElement { this.container_.appendChild(this.captionElement_); // Invisible close button at the end of lightbox for screen-readers. - const screenReaderCloseButton = - this.element.ownerDocument.createElement('button'); + const screenReaderCloseButton = this.element.ownerDocument.createElement( + 'button' + ); // TODO(aghassemi, #4146) i18n const ariaLabel = this.element.getAttribute('data-close-button-aria-label') || @@ -873,9 +871,8 @@ class AmpImageLightbox extends AMP.BaseElement { this.buildLightbox_(); const source = invocation.caller; - const tagName = source.tagName.toLowerCase(); userAssert( - source && SUPPORTED_ELEMENTS_.has(tagName), + source && SUPPORTED_ELEMENTS_[source.tagName.toLowerCase()], '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 367f2473ef208..b3b42f79ee234 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,13 +65,19 @@ describes.realWin( const impl = await lightbox.getImpl(false); const noop = () => {}; - impl.getViewport = () => ({ - onChanged: noop, - enterLightboxMode: noop, - }); - impl.getHistory_ = () => ({ - push: () => Promise.resolve(), - }); + impl.getViewport = () => { + return { + onChanged: noop, + enterLightboxMode: noop, + }; + }; + impl.getHistory_ = () => { + return { + push: () => { + return Promise.resolve(); + }, + }; + }; impl.enter_ = noop; const ampImage = doc.createElement('amp-img'); @@ -81,69 +87,23 @@ describes.realWin( 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.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; + expect(container).to.not.equal(null); const caption = container.querySelector( '.i-amphtml-image-lightbox-caption' ); - expect(caption).to.not.be.null; + expect(caption).to.not.equal(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; + expect(viewer).to.not.equal(null); const image = viewer.querySelector( '.i-amphtml-image-lightbox-viewer-image' ); - expect(image).to.not.be.null; + expect(image).to.not.equal(null); // Very important. Image must have transform-origin=50% 50%. const win = image.ownerDocument.defaultView; @@ -349,7 +309,6 @@ 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 1b9cf26c8de71..4641468401cb0 100644 --- a/extensions/amp-image-slider/0.1/amp-image-slider.js +++ b/extensions/amp-image-slider/0.1/amp-image-slider.js @@ -22,16 +22,13 @@ 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, loadPromise} from '../../../src/event-helper'; +import {listen} from '../../../src/event-helper'; import { observeWithSharedInOb, unobserveWithSharedInOb, } from '../../../src/viewport-observer'; -import {setStyle, setStyles} from '../../../src/style'; - -const VALID_IMAGE_TAGNAMES = new Set(['AMP-IMG', 'IMG']); +import {setStyles} from '../../../src/style'; export class AmpImageSlider extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -45,11 +42,10 @@ export class AmpImageSlider extends AMP.BaseElement { this.container_ = null; /** @private {?Element} */ - this.leftImage_ = null; + this.leftAmpImage_ = null; + /** @private {?Element} */ - this.rightImage_ = null; - /** @private {boolean} */ - this.containsAmpImages_ = false; + this.rightAmpImage_ = null; /** @private {?Element} */ this.leftLabelWrapper_ = null; @@ -63,12 +59,16 @@ 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,21 +112,22 @@ export class AmpImageSlider extends AMP.BaseElement { buildCallback() { const children = this.getRealChildren(); - for (const child of children) { - if (VALID_IMAGE_TAGNAMES.has(child.tagName)) { + for (let i = 0; i < children.length; i++) { + const child = children[i]; + if (child.tagName.toLowerCase() === 'amp-img') { // First encountered = left image // Second encountered = right image - if (!this.leftImage_) { - this.leftImage_ = child; - } else if (!this.rightImage_) { - this.rightImage_ = child; + if (!this.leftAmpImage_) { + this.leftAmpImage_ = child; + } else if (!this.rightAmpImage_) { + this.rightAmpImage_ = child; } else { user().error( 'AMP-IMAGE-SLIDER', - 'Should not contain more than 2 images.' + 'Should not contain more than 2 s.' ); } - } else if (child.tagName === 'DIV') { + } else if (child.tagName.toLowerCase() === 'div') { if (child.hasAttribute('first')) { this.leftLabel_ = child; } else if (child.hasAttribute('second')) { @@ -142,25 +143,18 @@ export class AmpImageSlider extends AMP.BaseElement { } userAssert( - this.leftImage_ && this.rightImage_, - '2 images must be provided for comparison' + this.leftAmpImage_ && this.rightAmpImage_, + '2 s must be provided for comparison' ); // see comment in layoutCallback // When layers not enabled const owners = Services.ownersForDoc(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; - } + owners.setOwner(dev().assertElement(this.leftAmpImage_), this.element); + owners.setOwner(dev().assertElement(this.rightAmpImage_), this.element); - this.container_ = htmlFor( - this.doc_ - )`
`; + this.container_ = this.doc_.createElement('div'); + this.container_.classList.add('i-amphtml-image-slider-container'); this.buildImageWrappers_(); this.buildBar_(); @@ -193,8 +187,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.leftImage_); - this.rightMask_.appendChild(this.rightImage_); + this.leftMask_.appendChild(this.leftAmpImage_); + this.rightMask_.appendChild(this.rightAmpImage_); // Set initial positioning if (initialPositionString) { const initialPosition = Number(initialPositionString); @@ -202,7 +196,9 @@ export class AmpImageSlider extends AMP.BaseElement { } // Prevent Edge horizontal swipe for go back/forward if (this.isEdge_) { - setStyle(this.element, 'touch-action', 'pan-y'); // allow browser only default y behavior + setStyles(this.element, { + 'touch-action': 'pan-y', // allow browser only default y behavior + }); } }); } @@ -230,7 +226,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.rightImage_.classList.add('i-amphtml-image-slider-push-left'); + this.rightAmpImage_.classList.add('i-amphtml-image-slider-push-left'); if (this.rightLabel_) { this.rightLabelWrapper_ = this.doc_.createElement('div'); this.rightLabelWrapper_.classList.add( @@ -247,11 +243,14 @@ export class AmpImageSlider extends AMP.BaseElement { * @private */ buildBar_() { - this.bar_ = htmlFor( - this.doc_ - )`
-
-
`; + 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.container_.appendChild(this.bar_); } @@ -296,13 +295,8 @@ export class AmpImageSlider extends AMP.BaseElement { * @private */ checkARIA_() { - 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_); + const leftAmpImage = dev().assertElement(this.leftAmpImage_); + const rightAmpImage = dev().assertElement(this.rightAmpImage_); leftAmpImage .signals() .whenSignal(CommonSignals.LOAD_END) @@ -672,7 +666,7 @@ export class AmpImageSlider extends AMP.BaseElement { this.updateTranslateX_(this.bar_, percentFromLeft); this.updateTranslateX_(this.rightMask_, percentFromLeft); - this.updateTranslateX_(this.rightImage_, -percentFromLeft); + this.updateTranslateX_(this.rightAmpImage_, -percentFromLeft); const adjustedDeltaFromLeft = percentFromLeft - 0.5; this.updateTranslateX_(this.hintLeftBody_, adjustedDeltaFromLeft); this.updateTranslateX_(this.hintRightBody_, adjustedDeltaFromLeft); @@ -714,44 +708,45 @@ export class AmpImageSlider extends AMP.BaseElement { observeWithSharedInOb(this.element, (inViewport) => this.viewportCallback_(inViewport) ); - - const appendHints = () => { - this.container_.appendChild(this.hintLeftBody_); - this.container_.appendChild(this.hintRightBody_); - }; + // 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_) + ); 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([ - loadPromise(this.leftImage_), - loadPromise(this.rightImage_), - ]).then(appendHints, appendHints); + 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_); + } + ); } /** @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 9f9e13a38e599..7afec4dc61bd4 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,30 +25,18 @@ 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 = /* HTML */ ` - + const sliderBody = ` +
BEFORE
AFTER
- +
BEFORE
@@ -57,32 +45,6 @@ t.run('amp-image-slider', {}, function () {

HUGE PADDING

- - - - -
BEFORE
-
AFTER
-
- `; const css = ` @@ -99,11 +61,6 @@ 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']; @@ -122,7 +79,6 @@ 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; @@ -712,24 +668,6 @@ 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( @@ -872,17 +810,15 @@ 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 && !!slider3.signals; + !!slider1.signals && !!slider2.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 @@ -908,7 +844,6 @@ 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 @@ -962,7 +897,6 @@ 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 a468ddeddce8d..bc8b7dab00543 100644 --- a/extensions/amp-image-viewer/0.1/amp-image-viewer.js +++ b/extensions/amp-image-viewer/0.1/amp-image-viewer.js @@ -46,7 +46,6 @@ 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'; @@ -55,7 +54,10 @@ const TAG = 'amp-image-viewer'; const ARIA_ATTRIBUTES = ['aria-label', 'aria-describedby', 'aria-labelledby']; const DEFAULT_MAX_SCALE = 2; -const ELIGIBLE_TAGS = new Set(['AMP-IMG', 'AMP-ANIM', 'IMG']); +const ELIGIBLE_TAGS = { + 'amp-img': true, + 'amp-anim': true, +}; export class AmpImageViewer extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -120,7 +122,7 @@ export class AmpImageViewer extends AMP.BaseElement { this.motion_ = null; /** @private {?Element} */ - this.sourceImage_ = null; + this.sourceAmpImage_ = null; /** @private {?Promise} */ this.loadPromise_ = null; @@ -145,9 +147,9 @@ export class AmpImageViewer extends AMP.BaseElement { TAG ); - this.sourceImage_ = children[0]; + this.sourceAmpImage_ = children[0]; Services.ownersForDoc(this.element).setOwner( - this.sourceImage_, + this.sourceAmpImage_, this.element ); } @@ -175,14 +177,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 img = dev().assertElement(this.sourceImage_); + const ampImg = dev().assertElement(this.sourceAmpImage_); const haveImg = !!this.image_; const laidOutPromise = haveImg ? Promise.resolve() - : img.signals().whenSignal(CommonSignals.LOAD_END); + : ampImg.signals().whenSignal(CommonSignals.LOAD_END); if (!haveImg) { - Services.ownersForDoc(this.element).scheduleLayout(this.element, img); + Services.ownersForDoc(this.element).scheduleLayout(this.element, ampImg); } this.loadPromise_ = laidOutPromise @@ -267,7 +269,7 @@ export class AmpImageViewer extends AMP.BaseElement { * @private */ elementIsSupported_(element) { - return ELIGIBLE_TAGS.has(element.tagName); + return ELIGIBLE_TAGS[element.tagName.toLowerCase()]; } /** @@ -303,9 +305,9 @@ export class AmpImageViewer extends AMP.BaseElement { this.image_ = this.element.ownerDocument.createElement('img'); this.image_.classList.add('i-amphtml-image-viewer-image'); - const img = dev().assertElement(this.sourceImage_); - this.setSourceDimensions_(img); - this.srcset_ = srcsetFromElement(img); + const ampImg = dev().assertElement(this.sourceAmpImage_); + this.setSourceDimensions_(ampImg); + this.srcset_ = srcsetFromElement(ampImg); observeContentSize(this.element, this.onResize_); @@ -316,10 +318,10 @@ export class AmpImageViewer extends AMP.BaseElement { width: 0, height: 0, }); - st.toggle(img, false); + st.toggle(ampImg, false); this.element.appendChild(this.image_); - return img.getImpl().then((impl) => { - propagateAttributes(ARIA_ATTRIBUTES, impl.element, this.image_); + return ampImg.getImpl().then((ampImg) => { + ampImg.propagateAttributes(ARIA_ATTRIBUTES, 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 d7664df81d101..2115ec6e93213 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,7 +21,6 @@ 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'; /** @@ -247,7 +246,7 @@ export class AmpInlineGalleryThumbnails extends AMP.BaseElement { // We create with loop defaulting to false above, and allow it to be // overwriten. - propagateAttributes(['loop'], this.element, this.carousel_); + this.propagateAttributes(['loop'], 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 d516759974569..ff86779e816f1 100644 --- a/extensions/amp-jwplayer/0.1/amp-jwplayer.js +++ b/extensions/amp-jwplayer/0.1/amp-jwplayer.js @@ -42,7 +42,6 @@ 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, @@ -350,7 +349,7 @@ class AmpJWPlayer extends AMP.BaseElement { return; } const placeholder = this.win.document.createElement('img'); - propagateAttributes(['aria-label'], this.element, placeholder); + this.propagateAttributes(['aria-label'], 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 6d984925ca434..ce704b25b6f4b 100644 --- a/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js +++ b/extensions/amp-kaltura-player/0.1/amp-kaltura-player.js @@ -20,7 +20,6 @@ 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'; @@ -126,7 +125,7 @@ class AmpKaltura extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.win.document.createElement('img'); - propagateAttributes(['aria-label'], this.element, placeholder); + this.propagateAttributes(['aria-label'], 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 eb8a43b70a538..6ff5492bc4466 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) { - if (element.classList.contains('amp-notsupported')) { - const fallback = element.getFallback(); - if (!!fallback) { - element = fallback; - } + const fallback = element.getFallback(); + const shouldCloneFallback = + element.classList.contains('amp-notsupported') && !!fallback; + if (shouldCloneFallback) { + 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.has(clonedNode.tagName)) { + if (ELIGIBLE_TAP_TAGS[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.has(element.tagName)) { + if (!ELIGIBLE_TAP_TAGS[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 012c05248fd05..de867e549d252 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,16 +38,21 @@ import {map} from '../../../../src/core/types/object'; import {srcsetFromElement, srcsetFromSrc} from '../../../../src/srcset'; import {toArray} from '../../../../src/core/types/array'; -const LIGHTBOX_ELIGIBLE_TAGS = new Set(['AMP-IMG', 'IMG']); +const LIGHTBOX_ELIGIBLE_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 ELIGIBLE_TAP_TAGS = { + 'AMP-IMG': true, +}; -// eslint-disable-next-line local/no-export-side-effect -export const VIDEO_TAGS = new Set(['AMP-YOUTUBE', 'AMP-VIDEO']); +export const VIDEO_TAGS = { + 'AMP-YOUTUBE': true, + 'AMP-VIDEO': true, +}; const GALLERY_TAG = 'amp-lightbox-gallery'; -const CAROUSEL_TAGS = new Set(['AMP-CAROUSEL', 'AMP-BASE-CAROUSEL']); +const CAROUSEL_TAGS = ['AMP-CAROUSEL', 'AMP-BASE-CAROUSEL']; const FIGURE_TAG = 'FIGURE'; const SLIDE_SELECTOR = '.amp-carousel-slide, .i-amphtml-carousel-slotted'; @@ -121,9 +126,9 @@ export class LightboxManager { // mapped unique ids. /** * List of lightbox elements that have already been scanned. - * @private {!Set} + * @private {!Array} */ - this.seen_ = new Set(); + this.seen_ = []; } /** @@ -161,9 +166,7 @@ export class LightboxManager { */ scanLightboxables_() { return this.ampdoc_.whenReady().then(() => { - const matches = this.ampdoc_ - .getRootNode() - .querySelectorAll('[lightbox],[data-lightbox]'); + const matches = this.ampdoc_.getRootNode().querySelectorAll('[lightbox]'); const processLightboxElement = this.processLightboxElement_.bind(this); iterateCursor(matches, processLightboxElement); }); @@ -176,7 +179,7 @@ export class LightboxManager { * @private */ baseElementIsSupported_(element) { - return LIGHTBOX_ELIGIBLE_TAGS.has(element.tagName); + return LIGHTBOX_ELIGIBLE_TAGS[element.tagName]; } /** @@ -200,11 +203,11 @@ export class LightboxManager { return; } const baseElement = getBaseElementForSlide(slide); - if (this.seen_.has(baseElement)) { + if (this.seen_.includes(baseElement)) { return; } baseElement.setAttribute('lightbox', lightboxGroupId); - this.seen_.add(baseElement); + this.seen_.push(baseElement); this.processBaseLightboxElement_(baseElement, lightboxGroupId); }); }); @@ -215,11 +218,11 @@ export class LightboxManager { * @private */ processLightboxElement_(element) { - if (this.seen_.has(element)) { + if (this.seen_.includes(element)) { return; } - this.seen_.add(element); - if (CAROUSEL_TAGS.has(element.tagName)) { + this.seen_.push(element); + if (CAROUSEL_TAGS.includes(element.tagName)) { this.processLightboxCarousel_(element); } else { const lightboxGroupId = element.getAttribute('lightbox') || 'default'; @@ -402,7 +405,7 @@ export class LightboxManager { if (element.hasAttribute('lightbox-thumbnail-id')) { const thumbnailId = element.getAttribute('lightbox-thumbnail-id'); const thumbnailImage = this.ampdoc_.getElementById(thumbnailId); - if (LIGHTBOX_ELIGIBLE_TAGS.has(thumbnailImage?.tagName)) { + if (thumbnailImage && thumbnailImage.tagName == 'AMP-IMG') { return srcsetFromElement(thumbnailImage); } } @@ -416,7 +419,7 @@ export class LightboxManager { * @private */ getUserPlaceholderSrcset_(element) { - if (LIGHTBOX_ELIGIBLE_TAGS.has(element.tagName)) { + if (element.tagName == 'AMP-IMG') { 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 4aa7e5044374d..c9e6904526520 100644 --- a/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js +++ b/extensions/amp-pan-zoom/0.1/amp-pan-zoom.js @@ -33,8 +33,10 @@ 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 { + layoutRectFromDomRect, + layoutRectLtwh, +} from '../../../src/core/math/layout-rect'; import {numeric} from '../../../src/transition'; import { observeContentSize, @@ -47,14 +49,13 @@ const TAG = 'amp-pan-zoom'; const DEFAULT_MAX_SCALE = 3; const MAX_ANIMATION_DURATION = 250; -const ELIGIBLE_TAGS = new Set([ - 'svg', - 'DIV', - 'AMP-IMG', - 'AMP-LAYOUT', - 'AMP-SELECTOR', - 'IMG', -]); +const ELIGIBLE_TAGS = { + 'svg': true, + 'DIV': true, + 'AMP-IMG': true, + 'AMP-LAYOUT': true, + 'AMP-SELECTOR': true, +}; /** * @extends {AMP.BaseElement} @@ -176,7 +177,7 @@ export class AmpPanZoom extends AMP.BaseElement { TAG ); userAssert( - ELIGIBLE_TAGS.has(children[0].tagName), + this.elementIsSupported_(children[0]), '%s is not supported by %s', children[0].tagName, TAG @@ -267,15 +268,24 @@ 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_ = htmlFor( - this.element - )`
`; - + 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_.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 57116042cafaf..f0225efa85eed 100644 --- a/extensions/amp-springboard-player/0.1/amp-springboard-player.js +++ b/extensions/amp-springboard-player/0.1/amp-springboard-player.js @@ -17,7 +17,6 @@ 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'; @@ -155,7 +154,7 @@ class AmpSpringboardPlayer extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.win.document.createElement('img'); - propagateAttributes(['aria-label'], this.element, placeholder); + this.propagateAttributes(['aria-label'], 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 fb0c406c3b98e..28adfd72fb5ca 100644 --- a/extensions/amp-story/1.0/amp-story-page.js +++ b/extensions/amp-story/1.0/amp-story-page.js @@ -77,7 +77,6 @@ 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'; @@ -1898,9 +1897,7 @@ export class AmpStoryPage extends AMP.BaseElement { childImgNode && ampImgNode .getImpl() - .then((impl) => - propagateAttributes('alt', impl.element, childImgNode) - ); + .then((ampImg) => ampImg.propagateAttributes('alt', childImgNode)); } }); } diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 7ebebecdfcab4..69e6684cd2112 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -41,7 +41,6 @@ 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, @@ -235,9 +234,8 @@ export class AmpVideo extends AMP.BaseElement { // Disable video preload in prerender mode. this.video_.setAttribute('preload', 'none'); this.checkA11yAttributeText_(); - propagateAttributes( + this.propagateAttributes( ATTRS_TO_PROPAGATE_ON_BUILD, - this.element, this.video_, /* opt_removeMissingAttrs */ true ); @@ -315,18 +313,13 @@ export class AmpVideo extends AMP.BaseElement { if (mutations['src']) { const urlService = this.getUrlService_(); urlService.assertHttpsUrl(element.getAttribute('src'), element); - propagateAttributes( - ['src'], - this.element, - dev().assertElement(this.video_) - ); + this.propagateAttributes(['src'], dev().assertElement(this.video_)); } const attrs = ATTRS_TO_PROPAGATE.filter( (value) => mutations[value] !== undefined ); - propagateAttributes( + this.propagateAttributes( attrs, - this.element, dev().assertElement(this.video_), /* opt_removeMissingAttrs */ true ); @@ -364,9 +357,8 @@ export class AmpVideo extends AMP.BaseElement { return Promise.resolve(); } - propagateAttributes( + this.propagateAttributes( ATTRS_TO_PROPAGATE_ON_LAYOUT, - this.element, dev().assertElement(this.video_), /* opt_removeMissingAttrs */ true ); @@ -547,11 +539,7 @@ 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); - propagateAttributes( - ['src'], - this.element, - dev().assertElement(this.video_) - ); + this.propagateAttributes(['src'], 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 06a531f7762d7..f9dd4502e1c2d 100644 --- a/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js +++ b/extensions/amp-viqeo-player/0.1/amp-viqeo-player.js @@ -31,7 +31,6 @@ 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'; @@ -206,7 +205,7 @@ class AmpViqeoPlayer extends AMP.BaseElement { /** @override */ createPlaceholderCallback() { const placeholder = this.element.ownerDocument.createElement('img'); - propagateAttributes(['aria-label'], this.element, placeholder); + this.propagateAttributes(['aria-label'], 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 510e1ea061597..95a4a335a8042 100644 --- a/extensions/amp-youtube/0.1/amp-youtube.js +++ b/extensions/amp-youtube/0.1/amp-youtube.js @@ -42,7 +42,6 @@ import {getData, listen} from '../../../src/event-helper'; import {htmlFor} from '../../../src/static-template'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isLayoutSizeDefined} from '../../../src/layout'; -import {propagateAttributes} from '../../../src/core/dom/propagate-attributes'; import {setStyles} from '../../../src/style'; const TAG = 'amp-youtube'; @@ -498,7 +497,7 @@ class AmpYoutube extends AMP.BaseElement { // the object-fit: cover. 'visibility': 'hidden', }); - propagateAttributes(['aria-label'], this.element, imgPlaceholder); + this.propagateAttributes(['aria-label'], 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 fa61df7fa4a61..44eca66dc87e7 100644 --- a/src/base-element.js +++ b/src/base-element.js @@ -742,6 +742,29 @@ 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 deleted file mode 100644 index 26640f7102c98..0000000000000 --- a/src/core/dom/propagate-attributes.js +++ /dev/null @@ -1,44 +0,0 @@ -/** - * 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 7936e6e6ed668..b7ae5e1be0a5d 100644 --- a/src/iframe-video.js +++ b/src/iframe-video.js @@ -19,7 +19,6 @@ 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} */ @@ -91,7 +90,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. - propagateAttributes(['referrerpolicy'], video.element, frame); + video.propagateAttributes(['referrerpolicy'], 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 adfff013fe323..065ea71549689 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 b151b2aab2c76..a53dcaf7a300e 100644 --- a/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html +++ b/test/fixtures/e2e/amp-auto-lightbox/amp-auto-lightbox.html @@ -43,18 +43,6 @@ 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 63f06a893f3de..d5c20389e9298 100644 --- a/test/manual/amp-image-slider.amp.html +++ b/test/manual/amp-image-slider.amp.html @@ -12,12 +12,6 @@ margin: 0 auto; } - img.custom-fill { - object-fit: cover; - width: 100%; - height: 100%; - } - .flex { display: flex; justify-content: space-between @@ -110,60 +104,30 @@

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
@@ -172,28 +136,14 @@

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
@@ -202,8 +152,8 @@

Actions

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

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

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 5ba66b715e88d..76339ac6fc540 100644 --- a/test/manual/amp-lightbox-gallery.amp.html +++ b/test/manual/amp-lightbox-gallery.amp.html @@ -10,15 +10,15 @@