From 7088ff998d421bd4047e6c6caa319425d91280a1 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 25 Jan 2021 12:16:29 -0500 Subject: [PATCH 1/9] introduce main render method --- src/amp-story-player/amp-story-player-impl.js | 256 ++++++++---------- 1 file changed, 117 insertions(+), 139 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index ab9709ba2178..515d351dd082 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -224,9 +224,6 @@ export class AmpStoryPlayer { /** @private {?Deferred} */ this.currentStoryLoadDeferred_ = null; - /** @private {!Deferred} */ - this.prerenderFirstStoryDeferred_ = new Deferred(); - /** @private {!Deferred} */ this.visibleDeferred_ = new Deferred(); @@ -265,39 +262,34 @@ export class AmpStoryPlayer { /** * Adds stories to the player. Additionally, creates or assigns * iframes to those that are close to the current playing story. - * @param {!Array} stories + * @param {!Array} newStories * @public */ - add(stories) { - if (stories.length <= 0) { + add(newStories) { + if (newStories.length <= 0) { return; } const isStoryDef = (story) => story && story.href; - if (!Array.isArray(stories) || !stories.every(isStoryDef)) { + if (!Array.isArray(newStories) || !newStories.every(isStoryDef)) { throw new Error('"stories" parameter has the wrong structure'); } - for (let i = 0; i < stories.length; i++) { - const story = stories[i]; + const renderStartingIdx = this.stories_.length; + + for (let i = 0; i < newStories.length; i++) { + const story = newStories[i]; story.idx = this.stories_.push(story) - 1; story.distance = story.idx - this.currentIdx_; this.build_(story); - - if (story.distance === 0) { - this.appendToDom_(story); - this.updateCurrentStory_(story); - continue; - } - - this.setSrc_(story); - - if (story.distance === 1) { - this.appendToDom_(story); - this.updatePosition_(story, StoryPosition.NEXT); - } } + + this.render_( + /* addingStories */ true, + /* startingIdx */ renderStartingIdx, + /* limit */ newStories.length + ); } /** @@ -392,9 +384,6 @@ export class AmpStoryPlayer { buildStories_() { this.stories_.forEach((story) => { this.build_(story); - if (story.distance <= 1) { - this.appendToDom_(story); - } }); } @@ -646,33 +635,6 @@ export class AmpStoryPlayer { }; } - /** @private */ - prerenderStories_() { - this.stories_.forEach((story) => { - this.setSrc_(story).then(() => - this.prerenderFirstStoryDeferred_.resolve() - ); - }); - - // Unblock layoutCallback when there are no stories initially. - if (this.stories_.length === 0) { - this.prerenderFirstStoryDeferred_.resolve(); - } - } - - /** @private */ - initializeVisibleIO_() { - const visibleCb = () => { - this.prerenderFirstStoryDeferred_.promise.then(() => - this.visibleDeferred_.resolve() - ); - }; - - new AmpStoryPlayerViewportObserver(this.win_, this.element_, () => - visibleCb() - ); - } - /** * @public */ @@ -680,14 +642,12 @@ export class AmpStoryPlayer { if (this.isLaidOut_) { return; } - this.prerenderStories_(); - this.initializeVisibleIO_(); - this.visibleDeferred_.promise.then(() => { - if (this.stories_[0]) { - this.updateVisibilityState_(this.stories_[0], VisibilityState.VISIBLE); - } - }); + new AmpStoryPlayerViewportObserver(this.win_, this.element_, () => + this.visibleDeferred_.resolve() + ); + + this.render_(/* addingStories */ true); this.isLaidOut_ = true; } @@ -727,7 +687,7 @@ export class AmpStoryPlayer { * @param {!StoryDef} story * @private */ - waitForStoryToLoadPromise_(story) { + initStoryContentLoadedPromise_(story) { this.currentStoryLoadDeferred_ = new Deferred(); story.messagingPromise.then((messaging) => @@ -754,13 +714,9 @@ export class AmpStoryPlayer { } if (storyIdx !== this.currentIdx_) { - const story = this.stories_[storyIdx]; this.currentIdx_ = storyIdx; - this.updateDistances_(storyIdx /* startingIdx */); - this.updateVisibilityState_(story, VisibilityState.VISIBLE); - - tryFocus(story.iframe); + this.render_(); this.onNavigation_(); } @@ -903,14 +859,8 @@ export class AmpStoryPlayer { } this.currentIdx_++; + this.render_(); - const previousStory = this.stories_[this.currentIdx_ - 1]; - this.updatePreviousStory_(previousStory, StoryPosition.PREVIOUS); - - const currentStory = this.stories_[this.currentIdx_]; - this.updateCurrentStory_(currentStory); - - this.updateDistances_(); this.onNavigation_(); } @@ -935,14 +885,8 @@ export class AmpStoryPlayer { } this.currentIdx_--; + this.render_(); - const previousStory = this.stories_[this.currentIdx_ + 1]; - this.updatePreviousStory_(previousStory, StoryPosition.NEXT); - - const currentStory = this.stories_[this.currentIdx_]; - this.updateCurrentStory_(currentStory); - - this.updateDistances_(); this.onNavigation_(); } @@ -980,76 +924,104 @@ export class AmpStoryPlayer { } /** - * Updates story to the `previous` state. + * Updates story position. * @param {!StoryDef} story - * @param {!StoryPosition} position * @private */ - updatePreviousStory_(story, position) { - this.updateVisibilityState_(story, VisibilityState.INACTIVE); - this.updatePosition_(story, position); - } + updatePosition_(story) { + const position = + story.distance === 0 + ? StoryPosition.CURRENT + : story.idx > this.currentIdx_ + ? StoryPosition.NEXT + : StoryPosition.PREVIOUS; - /** - * Updates a story to the `current` state. - * @param {!StoryDef} story - * @private - */ - updateCurrentStory_(story) { - // setSrc() must be called first to cancel previous story load if needed. - this.setSrc_(story).then(() => { - this.updateVisibilityState_(story, VisibilityState.VISIBLE); - this.updatePosition_(story, StoryPosition.CURRENT); - tryFocus(story.iframe); + requestAnimationFrame(() => { + const {iframe} = story; + resetStyles(iframe, ['transform', 'transition']); + iframe.setAttribute('i-amphtml-iframe-position', position); }); } /** - * Updates story position. + * Returns a promise that makes sure current story gets loaded first before + * others. * @param {!StoryDef} story - * @param {!StoryPosition} position + * @return {!Promise} * @private */ - updatePosition_(story, position) { - requestAnimationFrame(() => { - const {iframe} = story; - resetStyles(iframe, ['transform', 'transition']); - iframe.setAttribute('i-amphtml-iframe-position', position); - }); + currentStoryFirstPromise_(story) { + if (story.distance !== 0) { + return this.currentStoryLoadDeferred_.promise; + } + + if (this.currentStoryLoadDeferred_) { + // Cancel previous story load promise. + this.currentStoryLoadDeferred_.reject( + 'Cancelling previous story load promise.' + ); + } + this.initStoryContentLoadedPromise_(story); + return Promise.resolve(); } /** - * Updates distances of the stories. Appends to the DOM if new distance is - * <= 1 and removes from DOM if new distance > 1. It also positions the iframe - * for those appended to the DOM. + * - Updates distances of the stories. + * - Appends / removes from the DOM depending on distances. + * - Sets visibility state. + * - Loads iframe N+1 when N is ready. + * - Positions iframes depending on distance. + * @param {boolean=} addingStories Whether new stories are being added or not. * @param {number=} startingIdx + * @param {number=} limit * @private */ - updateDistances_(startingIdx = 0) { - for (let i = 0; i < this.stories_.length; i++) { + render_( + addingStories = false, + startingIdx = this.currentIdx_, + limit = this.stories_.length + ) { + for (let i = 0; i < limit; i++) { const story = this.stories_[(i + startingIdx) % this.stories_.length]; const oldDistance = story.distance; story.distance = Math.abs(this.currentIdx_ - story.idx); + // 1. Determine whether iframe should be in DOM tree or not. if (oldDistance <= 1 && story.distance > 1) { this.removeFromDom_(story); } - if (story.distance <= 1) { - if (oldDistance > 1) { - this.appendToDom_(story); - } + if ((oldDistance > 1 || addingStories) && story.distance <= 1) { + this.appendToDom_(story); + } - const position = - story.distance === 0 - ? StoryPosition.CURRENT - : story.idx > this.currentIdx_ - ? StoryPosition.NEXT - : StoryPosition.PREVIOUS; + // 2. Sets src to iframe. + this.setSrc_(story) + .then(() => { + // 3. Waits for player to be visible before moving on. + return this.visibleDeferred_.promise; + }) + .then(() => { + // 4. Update the visibility state of the story. + if (story.distance === 0) { + return this.updateVisibilityState_(story, VisibilityState.VISIBLE); + } - this.updatePosition_(story, position); - } + if (oldDistance === 0 && story.distance === 1) { + return this.updateVisibilityState_(story, VisibilityState.INACTIVE); + } + }) + .then(() => { + // 5. Finally update the story position. + if (story.distance <= 1) { + this.updatePosition_(story); + } + + if (story.distance === 0) { + tryFocus(story.iframe); + } + }); } } @@ -1062,6 +1034,24 @@ export class AmpStoryPlayer { this.setUpMessagingForStory_(story); } + /** + * @param {!StoryDef} story + * @private + */ + removeFromDom_(story) { + story.iframe.setAttribute('src', ''); + story.iframe.remove(); + } + + /** + * @param {!StoryDef} story + * @private + */ + appendToDom_(story) { + this.rootEl_.appendChild(story.iframe); + this.setUpMessagingForStory_(story); + } + /** * @param {!StoryDef} story * @private @@ -1085,25 +1075,12 @@ export class AmpStoryPlayer { return Promise.resolve(); } - let navigationPromise; - if (story.distance === 0) { - if (this.currentStoryLoadDeferred_) { - // Cancel previous story load promise. - this.currentStoryLoadDeferred_.reject( - 'Cancelling previous story load promise.' - ); - } - navigationPromise = Promise.resolve(); - this.waitForStoryToLoadPromise_(story); - } else { - navigationPromise = this.currentStoryLoadDeferred_.promise; - } - - return navigationPromise.then(() => { + this.currentStoryFirstPromise_(story).then(() => { const {href} = this.getEncodedLocation_( storyUrl, VisibilityState.PRERENDER ); + iframe.setAttribute('src', href); if (story.title) { iframe.setAttribute('title', story.title); @@ -1203,12 +1180,13 @@ export class AmpStoryPlayer { * Updates the visibility state of the story inside the iframe. * @param {!StoryDef} story * @param {!VisibilityState} visibilityState + * @return {!Promise} * @private */ updateVisibilityState_(story, visibilityState) { - story.messagingPromise.then((messaging) => { - messaging.sendRequest('visibilitychange', {state: visibilityState}, true); - }); + return story.messagingPromise.then((messaging) => + messaging.sendRequest('visibilitychange', {state: visibilityState}, true) + ); } /** From 11f7cd337b126c03365e9a5399d7a7f47e3238e0 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 25 Jan 2021 12:26:59 -0500 Subject: [PATCH 2/9] dont block on visiblityState change --- src/amp-story-player/amp-story-player-impl.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index 515d351dd082..646424d2db25 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -1005,11 +1005,11 @@ export class AmpStoryPlayer { .then(() => { // 4. Update the visibility state of the story. if (story.distance === 0) { - return this.updateVisibilityState_(story, VisibilityState.VISIBLE); + this.updateVisibilityState_(story, VisibilityState.VISIBLE); } if (oldDistance === 0 && story.distance === 1) { - return this.updateVisibilityState_(story, VisibilityState.INACTIVE); + this.updateVisibilityState_(story, VisibilityState.INACTIVE); } }) .then(() => { @@ -1180,11 +1180,10 @@ export class AmpStoryPlayer { * Updates the visibility state of the story inside the iframe. * @param {!StoryDef} story * @param {!VisibilityState} visibilityState - * @return {!Promise} * @private */ updateVisibilityState_(story, visibilityState) { - return story.messagingPromise.then((messaging) => + story.messagingPromise.then((messaging) => messaging.sendRequest('visibilitychange', {state: visibilityState}, true) ); } From 2bd706dfc26b11dd8570f8c3ca348769af0d506f Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 25 Jan 2021 17:06:52 -0500 Subject: [PATCH 3/9] add sync property for storyContentLoaded, extract logic from setSrc, wait on changing story before changing page --- src/amp-story-player/amp-story-player-impl.js | 138 +++++++++++------- 1 file changed, 85 insertions(+), 53 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index 646424d2db25..ad9f371f77d7 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -127,7 +127,8 @@ let DocumentStateTypeDef; * iframe: ?Element, * messagingPromise: ?Promise, * title: (?string), - * poster: (?string) + * poster: (?string), + * storyContentLoaded: ?boolean * }} */ let StoryDef; @@ -692,6 +693,9 @@ export class AmpStoryPlayer { story.messagingPromise.then((messaging) => messaging.registerHandler('storyContentLoaded', () => { + // Stories that already loaded won't dispatch a `storyContentLoaded` + // event anymore, which is why we need this sync property. + story.storyContentLoaded = true; this.currentStoryLoadDeferred_.resolve(); }) ); @@ -701,6 +705,7 @@ export class AmpStoryPlayer { * Shows the story provided by the URL in the player and go to the page if provided. * @param {?string} storyUrl * @param {string=} pageId + * @return {!Promise} */ show(storyUrl, pageId = null) { // TODO(enriqe): sanitize URLs for matching. @@ -713,16 +718,19 @@ export class AmpStoryPlayer { throw new Error(`Story URL not found in the player: ${storyUrl}`); } + let renderPromise = Promise.resolve(); if (storyIdx !== this.currentIdx_) { this.currentIdx_ = storyIdx; - this.render_(); + renderPromise = this.render_(); this.onNavigation_(); } if (pageId != null) { - this.goToPageId_(pageId); + return renderPromise.then(() => this.goToPageId_(pageId)); } + + return renderPromise; } /** Sends a message muting the current story. */ @@ -916,11 +924,14 @@ export class AmpStoryPlayer { this.stories_.length ]; + let showPromise = Promise.resolve(); if (this.currentIdx_ !== newStory.idx) { - this.show(newStory.href); + showPromise = this.show(newStory.href); } - this.selectPage_(pageDelta); + showPromise.then(() => { + this.selectPage_(pageDelta); + }); } /** @@ -951,6 +962,10 @@ export class AmpStoryPlayer { * @private */ currentStoryFirstPromise_(story) { + if (this.stories_[this.currentIdx_].storyContentLoaded) { + return Promise.resolve(); + } + if (story.distance !== 0) { return this.currentStoryLoadDeferred_.promise; } @@ -961,6 +976,7 @@ export class AmpStoryPlayer { 'Cancelling previous story load promise.' ); } + this.initStoryContentLoadedPromise_(story); return Promise.resolve(); } @@ -969,11 +985,12 @@ export class AmpStoryPlayer { * - Updates distances of the stories. * - Appends / removes from the DOM depending on distances. * - Sets visibility state. - * - Loads iframe N+1 when N is ready. + * - Loads story N+1 when N is ready. * - Positions iframes depending on distance. * @param {boolean=} addingStories Whether new stories are being added or not. * @param {number=} startingIdx * @param {number=} limit + * @return {!Promise} * @private */ render_( @@ -981,6 +998,7 @@ export class AmpStoryPlayer { startingIdx = this.currentIdx_, limit = this.stories_.length ) { + const renderPromises = []; for (let i = 0; i < limit; i++) { const story = this.stories_[(i + startingIdx) % this.stories_.length]; @@ -996,33 +1014,59 @@ export class AmpStoryPlayer { this.appendToDom_(story); } - // 2. Sets src to iframe. - this.setSrc_(story) - .then(() => { - // 3. Waits for player to be visible before moving on. - return this.visibleDeferred_.promise; - }) - .then(() => { - // 4. Update the visibility state of the story. - if (story.distance === 0) { - this.updateVisibilityState_(story, VisibilityState.VISIBLE); - } - - if (oldDistance === 0 && story.distance === 1) { - this.updateVisibilityState_(story, VisibilityState.INACTIVE); - } - }) - .then(() => { - // 5. Finally update the story position. - if (story.distance <= 1) { - this.updatePosition_(story); - } - - if (story.distance === 0) { - tryFocus(story.iframe); - } - }); + renderPromises.push( + // 1. Wait for current story to load before moving on. + this.currentStoryFirstPromise_(story) + .then(() => { + if (story.distance <= 1) { + return this.maybeGetCacheUrl_(story.href); + } + }) + // 2. Set iframe src when appropiate + .then((storyUrl) => { + if (!storyUrl) { + return; + } + const urlsAreEqual = this.sanitizedUrlsAreEquals_( + storyUrl, + story.iframe.src + ); + if (story.distance <= 1 && !urlsAreEqual) { + return this.setSrc_(story); + } + }) + .then(() => { + // 3. Waits for player to be visible before updating visibility + // state. + return this.visibleDeferred_.promise; + }) + .then(() => { + // 4. Update the visibility state of the story. + if (story.distance === 0) { + this.updateVisibilityState_(story, VisibilityState.VISIBLE); + } + + if (oldDistance === 0 && story.distance === 1) { + this.updateVisibilityState_(story, VisibilityState.INACTIVE); + } + }) + .then(() => { + // 5. Finally update the story position. + if (story.distance <= 1) { + this.updatePosition_(story); + } + + if (story.distance === 0) { + tryFocus(story.iframe); + } + }) + .catch((reason) => { + console /*OK*/ + .log({reason}); + }) + ); } + return Promise.all(renderPromises); } /** @@ -1039,6 +1083,7 @@ export class AmpStoryPlayer { * @private */ removeFromDom_(story) { + story.storyContentLoaded = false; story.iframe.setAttribute('src', ''); story.iframe.remove(); } @@ -1069,28 +1114,15 @@ export class AmpStoryPlayer { */ setSrc_(story) { const {iframe} = story; - return this.maybeGetCacheUrl_(story.href) - .then((storyUrl) => { - if (this.sanitizedUrlsAreEquals_(storyUrl, iframe.src)) { - return Promise.resolve(); - } - - this.currentStoryFirstPromise_(story).then(() => { - const {href} = this.getEncodedLocation_( - storyUrl, - VisibilityState.PRERENDER - ); + const {href} = this.getEncodedLocation_( + story.href, + VisibilityState.PRERENDER + ); - iframe.setAttribute('src', href); - if (story.title) { - iframe.setAttribute('title', story.title); - } - }); - }) - .catch((reason) => { - console /*OK*/ - .log({reason}); - }); + iframe.setAttribute('src', href); + if (story.title) { + iframe.setAttribute('title', story.title); + } } /** From ee8b93ba1297e8c6076c0cc87b2f67e805aa1bb0 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 25 Jan 2021 18:49:01 -0500 Subject: [PATCH 4/9] add tests for new show() functionality --- test/unit/test-amp-story-player.js | 33 +++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js index cfe10c4894ba..f7e05a17bf1c 100644 --- a/test/unit/test-amp-story-player.js +++ b/test/unit/test-amp-story-player.js @@ -524,7 +524,11 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { await player.load(); - await player.show('https://example.com/story3.html'); + player.show('https://example.com/story3.html'); + await nextTick(); + + fireHandler['storyContentLoaded']('storyContentLoaded', {}); + await nextTick(); const stories = playerEl.getStories(); @@ -535,6 +539,33 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { expect(playerEl.contains(stories[4].iframe)).to.be.true; }); + it('show() callback should prerender next story after current one is loaded', async () => { + const playerEl = win.document.createElement('amp-story-player'); + appendStoriesToPlayer(playerEl, 5); + + const player = new AmpStoryPlayer(win, playerEl); + + await player.load(); + + player.show('https://example.com/story3.html'); + await nextTick(); + + fireHandler['storyContentLoaded']('storyContentLoaded', {}); + await nextTick(); + + const storyIframes = playerEl.querySelectorAll('iframe'); + + expect(storyIframes[0].getAttribute('src')).to.include( + 'https://example.com/story3.html#visibilityState=prerender' + ); + expect(storyIframes[1].getAttribute('src')).to.include( + 'https://example.com/story4.html#visibilityState=prerender' + ); + expect(storyIframes[2].getAttribute('src')).to.include( + 'https://example.com/story2.html#visibilityState=prerender' + ); + }); + // TODO(proyectoramirez): delete once add() is implemented. it('show callback should throw when story is not found', async () => { const playerEl = win.document.createElement('amp-story-player'); From f82b3bdd527b7bcdc4ce56e41785dd2c87a4ef9f Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 25 Jan 2021 18:49:19 -0500 Subject: [PATCH 5/9] renames --- src/amp-story-player/amp-story-player-impl.js | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index ad9f371f77d7..2e4fffe5a47e 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -961,7 +961,7 @@ export class AmpStoryPlayer { * @return {!Promise} * @private */ - currentStoryFirstPromise_(story) { + currentStoryPromise_(story) { if (this.stories_[this.currentIdx_].storyContentLoaded) { return Promise.resolve(); } @@ -1016,7 +1016,7 @@ export class AmpStoryPlayer { renderPromises.push( // 1. Wait for current story to load before moving on. - this.currentStoryFirstPromise_(story) + this.currentStoryPromise_(story) .then(() => { if (story.distance <= 1) { return this.maybeGetCacheUrl_(story.href); @@ -1032,7 +1032,7 @@ export class AmpStoryPlayer { story.iframe.src ); if (story.distance <= 1 && !urlsAreEqual) { - return this.setSrc_(story); + this.setSrc_(story, storyUrl); } }) .then(() => { @@ -1106,18 +1106,15 @@ export class AmpStoryPlayer { } /** - * Sets the story src. It waits for first story before setting it to - * neighboring stories. + * Sets the story src to the iframe. * @param {!StoryDef} story + * @param {string} url * @return {!Promise} * @private */ - setSrc_(story) { + setSrc_(story, url) { const {iframe} = story; - const {href} = this.getEncodedLocation_( - story.href, - VisibilityState.PRERENDER - ); + const {href} = this.getEncodedLocation_(url, VisibilityState.PRERENDER); iframe.setAttribute('src', href); if (story.title) { From 4ade27265a2043d1ccf54b5d61a3f853bf982509 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 29 Jan 2021 18:32:49 -0500 Subject: [PATCH 6/9] reduce args in render(), fix names, fix errors logs --- src/amp-story-player/amp-story-player-impl.js | 136 ++++++++---------- 1 file changed, 61 insertions(+), 75 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index 2e4fffe5a47e..04a360a6d8b2 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -127,7 +127,7 @@ let DocumentStateTypeDef; * iframe: ?Element, * messagingPromise: ?Promise, * title: (?string), - * poster: (?string), + * posterImage: (?string), * storyContentLoaded: ?boolean * }} */ @@ -165,6 +165,11 @@ export let ViewerControlDef; /** @type {string} */ const TAG = 'amp-story-player'; +/** @enum {string} */ +const LOG_TYPE = { + DEV: 'amp-story-player-dev', +}; + /** * 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. @@ -286,11 +291,7 @@ export class AmpStoryPlayer { this.build_(story); } - this.render_( - /* addingStories */ true, - /* startingIdx */ renderStartingIdx, - /* limit */ newStories.length - ); + this.render_(renderStartingIdx); } /** @@ -367,7 +368,7 @@ export class AmpStoryPlayer { href: anchorEl.href, distance: idx, title: (anchorEl.textContent && anchorEl.textContent.trim()) || null, - poster: anchorEl.getAttribute('data-poster-portrait-src'), + posterImage: anchorEl.getAttribute('data-poster-portrait-src'), idx, }) ); @@ -449,6 +450,15 @@ export class AmpStoryPlayer { return this.playerConfig_; } + const ampCache = this.element_.getAttribute('amp-cache'); + if (ampCache && !SUPPORTED_CACHES.includes(ampCache)) { + console /*OK*/ + .error( + `[${TAG}]`, + `Unsupported cache specified, use one of following: ${SUPPORTED_CACHES}` + ); + } + const scriptTag = this.element_.querySelector('script'); if (!scriptTag) { return null; @@ -476,8 +486,8 @@ export class AmpStoryPlayer { */ build_(story) { const iframeEl = this.doc_.createElement('iframe'); - if (story.poster) { - setStyle(iframeEl, 'backgroundImage', story.poster); + if (story.posterImage) { + setStyle(iframeEl, 'backgroundImage', story.posterImage); } iframeEl.classList.add('story-player-iframe'); iframeEl.setAttribute('allow', 'autoplay'); @@ -574,7 +584,7 @@ export class AmpStoryPlayer { }, (err) => { console /*OK*/ - .log({err}); + .error(`[${TAG}]`, err); } ); }); @@ -606,15 +616,21 @@ export class AmpStoryPlayer { * @private */ initializeHandshake_(story, iframeEl) { - return this.maybeGetCacheUrl_(story.href).then((url) => { - return Messaging.waitForHandshakeFromDocument( + return this.maybeGetCacheUrl_(story.href).then((url) => + Messaging.waitForHandshakeFromDocument( this.win_, iframeEl.contentWindow, +<<<<<<< HEAD this.getEncodedLocation_(url).origin, /*opt_token*/ null, urls.cdnProxyRegex ); }); +======= + this.getEncodedLocation_(url).origin + ) + ); +>>>>>>> 10ef1c670 (reduce args in render(), fix names, fix errors logs) } /** @@ -648,7 +664,7 @@ export class AmpStoryPlayer { this.visibleDeferred_.resolve() ); - this.render_(/* addingStories */ true); + this.render_(); this.isLaidOut_ = true; } @@ -678,7 +694,7 @@ export class AmpStoryPlayer { .then((response) => response.json()) .catch((reason) => { console /*OK*/ - .error(`[${TAG}] `, reason); + .error(`[${TAG}]`, reason); }); } @@ -811,7 +827,7 @@ export class AmpStoryPlayer { }) .catch((reason) => { console /*OK*/ - .error(`[${TAG}] `, reason); + .error(`[${TAG}]`, reason); }); } } @@ -973,7 +989,7 @@ export class AmpStoryPlayer { if (this.currentStoryLoadDeferred_) { // Cancel previous story load promise. this.currentStoryLoadDeferred_.reject( - 'Cancelling previous story load promise.' + `[${LOG_TYPE.DEV}] Cancelling previous story load promise.` ); } @@ -987,19 +1003,14 @@ export class AmpStoryPlayer { * - Sets visibility state. * - Loads story N+1 when N is ready. * - Positions iframes depending on distance. - * @param {boolean=} addingStories Whether new stories are being added or not. * @param {number=} startingIdx - * @param {number=} limit * @return {!Promise} * @private */ - render_( - addingStories = false, - startingIdx = this.currentIdx_, - limit = this.stories_.length - ) { + render_(startingIdx = this.currentIdx_) { const renderPromises = []; - for (let i = 0; i < limit; i++) { + + for (let i = 0; i < this.stories_.length; i++) { const story = this.stories_[(i + startingIdx) % this.stories_.length]; const oldDistance = story.distance; @@ -1010,38 +1021,30 @@ export class AmpStoryPlayer { this.removeFromDom_(story); } - if ((oldDistance > 1 || addingStories) && story.distance <= 1) { + if (story.distance <= 1 && !story.iframe.isConnected) { this.appendToDom_(story); } + // Only create renderPromises for neighbor stories. + if (story.distance > 1) { + continue; + } + renderPromises.push( - // 1. Wait for current story to load before moving on. + // 1. Wait for current story to load before evaluating neighbor stories. this.currentStoryPromise_(story) - .then(() => { - if (story.distance <= 1) { - return this.maybeGetCacheUrl_(story.href); - } - }) + .then(() => this.maybeGetCacheUrl_(story.href)) // 2. Set iframe src when appropiate .then((storyUrl) => { - if (!storyUrl) { - return; - } - const urlsAreEqual = this.sanitizedUrlsAreEquals_( - storyUrl, - story.iframe.src - ); - if (story.distance <= 1 && !urlsAreEqual) { + if (!this.sanitizedUrlsAreEquals_(storyUrl, story.iframe.src)) { this.setSrc_(story, storyUrl); } }) + // 3. Waits for player to be visible before updating visibility + // state. + .then(() => this.visibleDeferred_.promise) + // 4. Update the visibility state of the story. .then(() => { - // 3. Waits for player to be visible before updating visibility - // state. - return this.visibleDeferred_.promise; - }) - .then(() => { - // 4. Update the visibility state of the story. if (story.distance === 0) { this.updateVisibilityState_(story, VisibilityState.VISIBLE); } @@ -1050,22 +1053,24 @@ export class AmpStoryPlayer { this.updateVisibilityState_(story, VisibilityState.INACTIVE); } }) + // 5. Finally update the story position. .then(() => { - // 5. Finally update the story position. - if (story.distance <= 1) { - this.updatePosition_(story); - } + this.updatePosition_(story); if (story.distance === 0) { tryFocus(story.iframe); } }) - .catch((reason) => { + .catch((err) => { + if (err.includes(LOG_TYPE.DEV)) { + return; + } console /*OK*/ - .log({reason}); + .error(`[${TAG}]`, err); }) ); } + return Promise.all(renderPromises); } @@ -1088,23 +1093,6 @@ export class AmpStoryPlayer { story.iframe.remove(); } - /** - * @param {!StoryDef} story - * @private - */ - appendToDom_(story) { - this.rootEl_.appendChild(story.iframe); - this.setUpMessagingForStory_(story); - } - - /** - * @param {!StoryDef} story - * @private - */ - removeFromDom_(story) { - story.iframe.remove(); - } - /** * Sets the story src to the iframe. * @param {!StoryDef} story @@ -1149,16 +1137,14 @@ export class AmpStoryPlayer { maybeGetCacheUrl_(url) { const ampCache = this.element_.getAttribute('amp-cache'); - if (!ampCache || isProxyOrigin(url)) { + if ( + !ampCache || + isProxyOrigin(url) || + !SUPPORTED_CACHES.includes(ampCache) + ) { return Promise.resolve(url); } - if (!SUPPORTED_CACHES.includes(ampCache)) { - throw new Error( - `Unsupported cache, use one of following: ${SUPPORTED_CACHES}` - ); - } - return ampToolboxCacheUrl .createCacheUrl(ampCache, url, 'viewer' /** servingType */) .then((cacheUrl) => { From 6166bc1135c1dffbf0cfa8fa552a748fc126ef21 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 29 Jan 2021 18:33:01 -0500 Subject: [PATCH 7/9] fix test --- test/unit/test-amp-story-player.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js index f7e05a17bf1c..9b1083f0b9bb 100644 --- a/test/unit/test-amp-story-player.js +++ b/test/unit/test-amp-story-player.js @@ -112,6 +112,10 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { .resolves(fakeMessaging); }); + afterEach(() => { + console.error.restore(); + }); + it('should build an iframe for each story', async () => { buildStoryPlayer(); await manager.loadPlayers(); @@ -449,10 +453,17 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => { }); it('should throw error when invalid url is provided', async () => { - buildStoryPlayer(1, DEFAULT_ORIGIN_URL, 'www.invalid.org'); + console.error.restore(); + env.sandbox.spy(console, 'error'); + + buildStoryPlayer(1, DEFAULT_ORIGIN_URL, 'www.tacos.org'); + + await manager.loadPlayers(); + await nextTick(); - return expect(() => manager.loadPlayers()).to.throw( - /Unsupported cache, use one of following: cdn.ampproject.org,www.bing-amp.com/ + expect(console.error).to.be.calledWithMatch( + /\[amp-story-player\]/, + /Unsupported cache specified, use one of following: cdn.ampproject.org,www.bing-amp.com/ ); }); }); From 21e1ea20868c3e4ff48fb3a9f6764e0c65903f22 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 29 Jan 2021 18:47:30 -0500 Subject: [PATCH 8/9] merge conflict --- src/amp-story-player/amp-story-player-impl.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index 04a360a6d8b2..62b2b082858f 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -620,17 +620,11 @@ export class AmpStoryPlayer { Messaging.waitForHandshakeFromDocument( this.win_, iframeEl.contentWindow, -<<<<<<< HEAD this.getEncodedLocation_(url).origin, /*opt_token*/ null, urls.cdnProxyRegex - ); - }); -======= - this.getEncodedLocation_(url).origin ) ); ->>>>>>> 10ef1c670 (reduce args in render(), fix names, fix errors logs) } /** From 249d389e889e656a5e4f382859e38fd361a5d2d4 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 1 Feb 2021 16:11:31 -0500 Subject: [PATCH 9/9] update e2e test --- test/e2e/test-amp-story-player-prerender.js | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/test/e2e/test-amp-story-player-prerender.js b/test/e2e/test-amp-story-player-prerender.js index 615f2505c11c..d76d0eadf9a1 100644 --- a/test/e2e/test-amp-story-player-prerender.js +++ b/test/e2e/test-amp-story-player-prerender.js @@ -41,35 +41,34 @@ describes.endtoend( beforeEach(async () => { controller = env.controller; - player = await controller.findElement( - 'amp-story-player.i-amphtml-story-player-loaded' - ); + player = await controller.findElement('amp-story-player'); await expect(player); }); - it('player builds the iframe when below the fold', async () => { + it('player builds the shadow DOM container when far from the viewport', async () => { const shadowHost = await controller.findElement( 'div.i-amphtml-story-player-shadow-root-intermediary' ); - await controller.switchToShadowRoot(shadowHost); - - const iframe = await controller.findElement('iframe'); - - await expect(iframe); + await expect(shadowHost); }); - it('when player is far from viewport, no stories are loaded in the iframes', async () => { + it('when player is far from viewport and user has not scrolled, no stories are loaded in the iframes', async () => { const shadowHost = await controller.findElement( 'div.i-amphtml-story-player-shadow-root-intermediary' ); await controller.switchToShadowRoot(shadowHost); + const iframeContainer = await controller.findElement( + '.i-amphtml-story-player-main-container' + ); - const iframe = await controller.findElement('iframe'); - const iframeSrc = await controller.getElementAttribute(iframe, 'src'); + const count = await controller.getElementProperty( + iframeContainer, + 'childElementCount' + ); - await expect(iframeSrc).to.not.exist; + await expect(count).to.eql(0); }); it('when player comes close to the viewport, iframe loads first story in prerender', async () => {