From df04d5498f1050fcba7cfdf6d86ed5a705a9f631 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin <5449100+Enriqe@users.noreply.github.com> Date: Mon, 6 Apr 2020 18:36:34 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8[amp-story-player]=20Upgrading=20amp-s?= =?UTF-8?q?tory-player=20to=20CE=20(#27320)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * upgrade to CEv1, move ctor to init, change from onload to load listener * add viewpoer meta tag to sample * failing test * fix tests * expand comment on tests * call connectedCallback() instead of init * fix test * change to suffix underscore for convention * rely on connectedCallback rather than calling build + layout manaully * prettier parens * Revert "change to suffix underscore for convention" This reverts commit 3118d00c06085d325f1c40d5a3d462e3f24e70e9. * call buildcallback & layoutWHenVisible from connectedCallback, making each player responsible to call them individually. --- examples/amp-story/player.html | 1 + src/amp-story-player-impl.js | 45 ++++++++--------- src/amp-story-player-manager.js | 19 +------- src/amp-story-player.js | 9 ++-- test/unit/test-amp-story-player.js | 78 +++++++++++++++++------------- 5 files changed, 72 insertions(+), 80 deletions(-) diff --git a/examples/amp-story/player.html b/examples/amp-story/player.html index 8b308a55bb93..bf4ae9a2b17f 100644 --- a/examples/amp-story/player.html +++ b/examples/amp-story/player.html @@ -32,6 +32,7 @@ text-shadow: 3px 3px #000; } +

This is a NON-AMP page that embeds a story below:

diff --git a/src/amp-story-player-impl.js b/src/amp-story-player-impl.js index e35831352dbe..5cda5170d529 100644 --- a/src/amp-story-player-impl.js +++ b/src/amp-story-player-impl.js @@ -14,18 +14,19 @@ * limitations under the License. */ +import {IframePool} from './amp-story-player-iframe-pool'; import {Messaging} from '@ampproject/viewer-messaging'; +import {VisibilityState} from './visibility-state'; import { addParamsToUrl, getFragment, parseUrlWithA, removeFragment, } from './url'; +import {applySandbox} from './3p-frame'; import {dict, map} from './utils/object'; // Source for this constant is css/amp-story-player-iframe.css -import {IframePool} from './amp-story-player-iframe-pool'; -import {VisibilityState} from './visibility-state'; -import {applySandbox} from './3p-frame'; +import {AmpStoryPlayerManager} from './amp-story-player-manager'; import {cssText} from '../build/amp-story-player-iframe.css'; import {resetStyles, setStyle, setStyles} from './style'; import {toArray} from './types'; @@ -66,29 +67,21 @@ export const IFRAME_IDX = '__AMP_IFRAME_IDX__'; * Note that this is a vanilla JavaScript class and should not depend on AMP * services, as v0.js is not expected to be loaded in this context. */ -export class AmpStoryPlayer { +export class AmpStoryPlayer extends HTMLElement { /** - * @param {!Window} win - * @param {!Element} element - * @constructor + * Player is appended in the document. */ - constructor(win, element) { - console./*OK*/ assert( - element.childElementCount > 0, - 'Missing configuration.' - ); + connectedCallback() { + console./*OK*/ assert(this.childElementCount > 0, 'Missing configuration.'); /** @private {!Window} */ - this.win_ = win; + this.win_ = self; /** @private {!Array} */ this.iframes_ = []; - /** @private {!Element} */ - this.element_ = element; - /** @private {!Document} */ - this.doc_ = win.document; + this.doc_ = this.win_.document; /** @private {!Element} */ this.cachedA_ = this.doc_.createElement('a'); @@ -121,6 +114,10 @@ export class AmpStoryPlayer { lastX: 0, isSwipeX: null, }; + + this.buildCallback_(); + const manager = new AmpStoryPlayerManager(self); + manager.layoutWhenVisible(this); } /** @@ -128,12 +125,12 @@ export class AmpStoryPlayer { * @return {!Element} */ getElement() { - return this.element_; + return this; } - /** @public */ - buildCallback() { - this.stories_ = toArray(this.element_.querySelectorAll('a')); + /** @private */ + buildCallback_() { + this.stories_ = toArray(this.querySelectorAll('a')); this.initializeShadowRoot_(); this.initializeIframes_(); @@ -158,7 +155,7 @@ export class AmpStoryPlayer { this.rootEl_ = this.doc_.createElement('main'); // Create shadow root - const shadowRoot = this.element_.attachShadow({mode: 'open'}); + const shadowRoot = this.attachShadow({mode: 'open'}); // Inject default styles const styleEl = this.doc_.createElement('style'); @@ -250,12 +247,12 @@ export class AmpStoryPlayer { iframeEl.onload = () => { this.rootEl_.classList.remove(LoadStateClass.LOADING); this.rootEl_.classList.add(LoadStateClass.LOADED); - this.element_.classList.add(LoadStateClass.LOADED); + this.classList.add(LoadStateClass.LOADED); }; iframeEl.onerror = () => { this.rootEl_.classList.remove(LoadStateClass.LOADING); this.rootEl_.classList.add(LoadStateClass.ERROR); - this.element_.classList.add(LoadStateClass.ERROR); + this.classList.add(LoadStateClass.ERROR); }; } diff --git a/src/amp-story-player-manager.js b/src/amp-story-player-manager.js index a248fd260af6..5350167b6e4b 100644 --- a/src/amp-story-player-manager.js +++ b/src/amp-story-player-manager.js @@ -14,7 +14,6 @@ * limitations under the License. */ -import {AmpStoryPlayer} from './amp-story-player-impl'; import {throttle} from './utils/rate-limit'; /** @const {string} */ @@ -33,9 +32,8 @@ export class AmpStoryPlayerManager { /** * Calls layoutCallback on the player when it is close to the viewport. * @param {!AmpStoryPlayer} playerImpl - * @private */ - layoutPlayer_(playerImpl) { + layoutWhenVisible(playerImpl) { if (!this.win_.IntersectionObserver || this.win_ !== this.win_.parent) { this.layoutFallback_(playerImpl); return; @@ -90,19 +88,4 @@ export class AmpStoryPlayerManager { playerImpl.layoutCallback(); } } - - /** - * Builds and layouts the players when appropiate. - * @public - */ - loadPlayers() { - const doc = this.win_.document; - const players = doc.getElementsByTagName('amp-story-player'); - for (let i = 0; i < players.length; i++) { - const playerEl = players[i]; - const player = new AmpStoryPlayer(this.win_, playerEl); - player.buildCallback(); - this.layoutPlayer_(player); - } - } } diff --git a/src/amp-story-player.js b/src/amp-story-player.js index 875c41a2c8b6..c2d11457ab2a 100644 --- a/src/amp-story-player.js +++ b/src/amp-story-player.js @@ -14,9 +14,8 @@ * limitations under the License. */ -import {AmpStoryPlayerManager} from './amp-story-player-manager'; +import {AmpStoryPlayer} from './amp-story-player-impl'; -self.onload = () => { - const manager = new AmpStoryPlayerManager(self); - manager.loadPlayers(); -}; +self.addEventListener('load', () => { + customElements.define('amp-story-player', AmpStoryPlayer); +}); diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js index 352a208f6435..de705d4a931a 100644 --- a/test/unit/test-amp-story-player.js +++ b/test/unit/test-amp-story-player.js @@ -14,35 +14,30 @@ * limitations under the License. */ -import {AmpStoryPlayerManager} from '../../src/amp-story-player-manager'; -import {IFRAME_IDX} from '../../src/amp-story-player-impl'; +import {AmpStoryPlayer, IFRAME_IDX} from '../../src/amp-story-player-impl'; import {Messaging} from '@ampproject/viewer-messaging'; import {toArray} from '../../src/types'; describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { let win; let playerEl; - let url; - let manager; + const fireHandler = []; + const DEFAULT_URL = + 'https://www-washingtonpost-com.cdn.ampproject.org/v/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/'; let fakeMessaging; let messagingMock; + let oldPrototype; - function buildStoryPlayer(numStories = 1) { + function buildStoryPlayer(numStories = 1, url = DEFAULT_URL) { playerEl = win.document.createElement('amp-story-player'); for (let i = 0; i < numStories; i++) { const storyAnchor = win.document.createElement('a'); - url = - 'https://www-washingtonpost-com.cdn.ampproject.org/v/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/'; storyAnchor.setAttribute('href', url); playerEl.appendChild(storyAnchor); } - win.document.body.appendChild(playerEl); - manager = new AmpStoryPlayerManager(win); - env.sandbox - .stub(Messaging, 'waitForHandshakeFromDocument') - .resolves(fakeMessaging); + win.document.body.appendChild(playerEl); } function swipeLeft() { @@ -69,6 +64,24 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { beforeEach(() => { win = env.win; + + // By the time this code runs, the outer window has already defined + // AmpStoryPlayer in the import, which still references the HTMLElement + // native function it extends. Then the `custom-elements.js` polyfill + // replaces it in the inner window `realWin`, but by this point it's no good + // because it's already been accessed, throwing an error. + // + // By using `Object.setPrototypeOf(AmpStoryPlayer, win.HTMLElement)`, we + // make `AmpStoryPlayer` native to the current inner `realWin` window, + // making sure it uses the polyfill and not the native function. + // + // Doing this causes the class to be poisoned, so we reset it on every test + // run. + oldPrototype = Object.getPrototypeOf(AmpStoryPlayer); + Object.setPrototypeOf(AmpStoryPlayer, win.HTMLElement); + Object.setPrototypeOf(AmpStoryPlayer.prototype, win.HTMLElement.prototype); + win.customElements.define('amp-story-player', AmpStoryPlayer); + fakeMessaging = { setDefaultHandler: () => {}, sendRequest: () => {}, @@ -78,47 +91,51 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { }, }; messagingMock = env.sandbox.mock(fakeMessaging); + env.sandbox + .stub(Messaging, 'waitForHandshakeFromDocument') + .resolves(fakeMessaging); }); afterEach(() => { messagingMock.verify(); + Object.setPrototypeOf(AmpStoryPlayer, oldPrototype); + Object.setPrototypeOf(AmpStoryPlayer.prototype, oldPrototype.prototype); }); it('should build an iframe for each story', () => { buildStoryPlayer(); - manager.loadPlayers(); expect(playerEl.shadowRoot.querySelector('iframe')).to.exist; }); it('should correctly append params at the end of the story url', () => { buildStoryPlayer(); - manager.loadPlayers(); + const storyIframe = playerEl.shadowRoot.querySelector('iframe'); expect(storyIframe.getAttribute('src')).to.equals( - url + - '?amp_js_v=0.1#visibilityState=visible&origin=about%3Asrcdoc&showStoryUrlInfo=0&storyPlayer=v0&cap=swipe' + DEFAULT_URL + + '?amp_js_v=0.1#visibilityState=visible&origin=http%3A%2F%2Flocalhost' + + '%3A9876&showStoryUrlInfo=0&storyPlayer=v0&cap=swipe' ); }); - it('should correctly append params at the end of a story url with existing params', () => { - buildStoryPlayer(); - url += '?testParam=true#myhash=hashValue'; - playerEl.firstElementChild.setAttribute('href', url); + it('should correctly append params at the end of a story url with existing params', async () => { + const existingParams = '?testParam=true#myhash=hashValue'; + buildStoryPlayer(1, DEFAULT_URL + existingParams); - manager.loadPlayers(); const storyIframe = playerEl.shadowRoot.querySelector('iframe'); expect(storyIframe.getAttribute('src')).to.equals( - url + - '&_js_v=0.1#visibilityState=visible&origin=about%3Asrcdoc&showStoryUrlInfo=0&storyPlayer=v0&cap=swipe' + DEFAULT_URL + + existingParams + + '&_js_v=0.1#visibilityState=visible&origin=http%3A%2F%2Flocalhost' + + '%3A9876&showStoryUrlInfo=0&storyPlayer=v0&cap=swipe' ); }); it('should set first story as visible', () => { buildStoryPlayer(3); - manager.loadPlayers(); const storyIframes = playerEl.shadowRoot.querySelectorAll('iframe'); expect(storyIframes[0].getAttribute('src')).to.include( @@ -128,7 +145,6 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { it('should prerender next stories', () => { buildStoryPlayer(3); - manager.loadPlayers(); const storyIframes = playerEl.shadowRoot.querySelectorAll('iframe'); expect(storyIframes[1].getAttribute('src')).to.include( @@ -143,7 +159,7 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { buildStoryPlayer(4); const stories = toArray(playerEl.querySelectorAll('a')); - await manager.loadPlayers(); + await Promise.resolve(); // Microtask tick. swipeLeft(); expect(stories[0][IFRAME_IDX]).to.eql(0); @@ -162,7 +178,7 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { buildStoryPlayer(4); const stories = toArray(playerEl.querySelectorAll('a')); - await manager.loadPlayers(); + await Promise.resolve(); // Microtask tick. swipeLeft(); swipeLeft(); @@ -181,15 +197,11 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { messagingMock.expects('registerHandler').withArgs('touchmove'); messagingMock.expects('registerHandler').withArgs('touchend'); messagingMock.expects('setDefaultHandler'); - - await manager.loadPlayers(); }); it('should navigate to next story when the last page of a story is tapped', async () => { buildStoryPlayer(2); - await manager.loadPlayers(); - const fakeData = {next: true}; fireHandler['selectDocument']('selectDocument', fakeData); @@ -202,7 +214,7 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { it('should navigate when swiping', async () => { buildStoryPlayer(4); - await manager.loadPlayers(); + await Promise.resolve(); // Microtask tick. swipeLeft(); @@ -215,7 +227,7 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { it('should not navigate when swiping last story', async () => { buildStoryPlayer(2); - await manager.loadPlayers(); + await Promise.resolve(); // Microtask tick. swipeLeft(); swipeLeft();