🚀 🐛 [amp-story-player] show() improvements (#30085)

* simplify and improve show() * leverage new loading strategy * update todo * restore todos * update tests * sort storyIdsWithIframe_ on each rotation, clean ups
ampproject · Sep 24, 2020 · 1e5d976 · 1e5d976
1 parent e24849f
commit 1e5d976
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 91 deletions.
diff --git a/src/amp-story-player/amp-story-player-iframe-pool.js b/src/amp-story-player/amp-story-player-iframe-pool.js
@@ -55,8 +55,7 @@ export class IframePool {
   rotateFirst(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.shift();
     this.storyIdsWithIframe_.push(nextStoryIdx);
-
-    this.iframePool_.push(this.iframePool_.shift());
+    this.storyIdsWithIframe_.sort();
 
     return detachedStoryIdx;
   }
@@ -71,30 +70,12 @@ export class IframePool {
    */
   rotateLast(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.pop();
-    this.storyIdsWithIframe_.unshift(nextStoryIdx);
-
-    this.iframePool_.unshift(this.iframePool_.pop());
+    this.storyIdsWithIframe_.push(nextStoryIdx);
+    this.storyIdsWithIframe_.sort();
 
     return detachedStoryIdx;
   }
 
-  /**
-   * Clears stored story indices.
-   * @return {!Array<number>}
-   */
-  evictStories() {
-    const evictedStories = this.storyIdsWithIframe_;
-    this.storyIdsWithIframe_ = [];
-    return evictedStories;
-  }
-
-  /**
-   * @return {!Array<number>}
-   */
-  getAvailableIframeIdx() {
-    return this.iframePool_;
-  }
-
   /**
    * Finds adjacent iframe indices given an index.
    * Examples of resulting adjacent arrays:
@@ -112,8 +93,11 @@ export class IframePool {
     // If index is at rightmost part of the array, adjacent iframes will all be
     // at the left.
     let cursor = storyIdx + 1 > maxIdx ? storyIdx - 2 : storyIdx - 1;
+
+    // Place current story index first to prioritize loading over adjacent ones.
+    adjacent.push(storyIdx);
     while (adjacent.length < this.iframePool_.length) {
-      if (cursor < 0) {
+      if (cursor < 0 || cursor === storyIdx) {
         ++cursor;
         continue;
       }

diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js
@@ -607,7 +607,7 @@ export class AmpStoryPlayer {
     // TODO(enriqe): sanitize URLs for matching.
     const storyIdx = findIndex(this.stories_, ({href}) => href === storyUrl);
 
-    // TODO(proyectoramirez): replace for add() once implemented.
+    // TODO(#28987): replace for add() once implemented.
     if (!this.stories_[storyIdx]) {
       throw new Error(`Story URL not found in the player: ${storyUrl}`);
     }
@@ -616,11 +616,42 @@ export class AmpStoryPlayer {
       return;
     }
 
-    this.currentIdx_ = storyIdx;
+    const adjacentStoriesIdx = this.iframePool_.findAdjacent(
+      storyIdx,
+      this.stories_.length - 1
+    );
 
-    this.evictStoriesFromIframes_();
-    this.assignIframesForStoryIdx_(storyIdx);
+    adjacentStoriesIdx.forEach((idx) => {
+      const story = this.stories_[idx];
+      let iframeIdx = story[IFRAME_IDX];
+
+      if (iframeIdx === undefined) {
+        const visibilityState =
+          idx === storyIdx
+            ? VisibilityState.VISIBLE
+            : VisibilityState.PRERENDER;
+        this.allocateIframeForStory_(
+          idx,
+          storyIdx < this.currentIdx_ /** reverse */,
+          visibilityState
+        );
+        iframeIdx = story[IFRAME_IDX];
+      }
 
+      let iframePosition;
+      if (idx === storyIdx) {
+        iframePosition = IframePosition.CURRENT;
+        this.updateVisibilityState_(iframeIdx, VisibilityState.VISIBLE);
+        tryFocus(this.iframes_[iframeIdx]);
+      } else {
+        iframePosition =
+          idx > storyIdx ? IframePosition.NEXT : IframePosition.PREVIOUS;
+      }
+
+      this.updateIframePosition_(iframeIdx, iframePosition);
+    });
+
+    this.currentIdx_ = storyIdx;
     this.signalNavigation_();
   }
 
@@ -651,65 +682,6 @@ export class AmpStoryPlayer {
     }
   }
 
-  /**
-   * Evicts stories from iframes.
-   * @private
-   */
-  evictStoriesFromIframes_() {
-    const evictedStories = this.iframePool_.evictStories();
-
-    evictedStories.forEach((storyIdx) => {
-      const story = this.stories_[storyIdx];
-      this.messagingPromises_[story[IFRAME_IDX]].then((messaging) => {
-        messaging.unregisterHandler('documentStateUpdate');
-        messaging.unregisterHandler('selectDocument');
-      });
-      story[IFRAME_IDX] = undefined;
-    });
-  }
-
-  /**
-   * Sets up new iframe arrangement given a story index. The adjacent stories
-   * will be prerendered and positioned accordingly. All messaging will be
-   * setup.
-   * @param {number} storyIdx
-   * @private
-   */
-  assignIframesForStoryIdx_(storyIdx) {
-    const availableIframeIdx = this.iframePool_.getAvailableIframeIdx();
-    const adjacentStoriesIdx = this.iframePool_.findAdjacent(
-      storyIdx,
-      this.stories_.length - 1
-    );
-
-    for (let i = 0; i < adjacentStoriesIdx.length; i++) {
-      const story = this.stories_[adjacentStoriesIdx[i]];
-      story[IFRAME_IDX] = availableIframeIdx[i];
-      this.iframePool_.addStoryIdx(adjacentStoriesIdx[i]);
-
-      const iframe = this.iframes_[story[IFRAME_IDX]];
-
-      this.layoutIframe_(
-        story,
-        iframe,
-        adjacentStoriesIdx[i] === storyIdx
-          ? VisibilityState.VISIBLE
-          : VisibilityState.PRERENDER
-      );
-
-      this.updateIframePosition_(
-        availableIframeIdx[i],
-        adjacentStoriesIdx[i] === storyIdx
-          ? IframePosition.CURRENT
-          : adjacentStoriesIdx[i] > storyIdx
-          ? IframePosition.NEXT
-          : IframePosition.PREVIOUS
-      );
-
-      this.setUpMessagingForIframe_(story, iframe);
-    }
-  }
-
   /**
    * Indicates the player changed story.
    * @private
@@ -879,9 +851,14 @@ export class AmpStoryPlayer {
    * navigating and allocates it to a story that the user is close to seeing.
    * @param {number} nextStoryIdx
    * @param {boolean} reverse
+   * @param {VisibilityState=} visibilityState
    * @private
    */
-  allocateIframeForStory_(nextStoryIdx, reverse = false) {
+  allocateIframeForStory_(
+    nextStoryIdx,
+    reverse = false,
+    visibilityState = VisibilityState.PRERENDER
+  ) {
     const detachedStoryIdx = reverse
       ? this.iframePool_.rotateLast(nextStoryIdx)
       : this.iframePool_.rotateFirst(nextStoryIdx);
@@ -898,7 +875,7 @@ export class AmpStoryPlayer {
     detachedStory[IFRAME_IDX] = undefined;
 
     const nextIframe = this.iframes_[nextStory[IFRAME_IDX]];
-    this.layoutIframe_(nextStory, nextIframe, VisibilityState.PRERENDER);
+    this.layoutIframe_(nextStory, nextIframe, visibilityState);
     this.updateIframePosition_(
       nextStory[IFRAME_IDX],
       reverse ? IframePosition.PREVIOUS : IframePosition.NEXT

diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js
@@ -512,9 +512,9 @@ describes.realWin('AmpStoryPlayer', {amp: false}, (env) => {
 
       expect(stories[0][IFRAME_IDX]).to.eql(undefined);
       expect(stories[1][IFRAME_IDX]).to.eql(undefined);
-      expect(stories[2][IFRAME_IDX]).to.eql(0);
-      expect(stories[3][IFRAME_IDX]).to.eql(1);
-      expect(stories[4][IFRAME_IDX]).to.eql(2);
+      expect(stories[2][IFRAME_IDX]).to.eql(2);
+      expect(stories[3][IFRAME_IDX]).to.eql(0);
+      expect(stories[4][IFRAME_IDX]).to.eql(1);
     });
 
     // TODO(proyectoramirez): delete once add() is implemented.