Deduplicate amp-story-page ids.

ampproject · Sep 24, 2019 · 365b04b · 365b04b
1 parent 2e9a940
commit 365b04b
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 0 deletions.
diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js
@@ -415,6 +415,7 @@ export class AmpStory extends AMP.BaseElement {
     this.initializeStyles_();
     this.initializeListeners_();
     this.initializeListenersForDev_();
+    this.initializePageIds_();
 
     this.storeService_.dispatch(Action.TOGGLE_UI, this.getUIType_());
 
@@ -520,6 +521,25 @@ export class AmpStory extends AMP.BaseElement {
     });
   }
 
+  /**
+   * Initializes page ids by deduplicating them.
+   * @private
+   */
+  initializePageIds_() {
+    const pageEls = this.element.querySelectorAll('amp-story-page');
+    const pageIds = Array.prototype.map.call(pageEls, el => el.id || 'default');
+    for (let i = 0; i < pageIds.length; i++) {
+      let counter = 0;
+      let index;
+      while ((index = pageIds.indexOf(pageIds[i], i + 1)) !== -1) {
+        user().error(TAG, `Duplicate amp-story-page ID ${pageIds[index]}`);
+        const newId = `${pageIds[index]}__${++counter}`;
+        pageEls[index].id = newId;
+        pageIds[index] = newId;
+      }
+    }
+  }
+
   /**
    * @param {!Element} styleEl
    * @private
@@ -607,6 +627,9 @@ export class AmpStory extends AMP.BaseElement {
   buildSystemLayer_(initialPageId) {
     this.updateAudioIcon_();
 
+    // TODO(gmajoulet): cache the page ids in the store from the
+    // initializePageIds_ method to remove this block. Requires refactoring in
+    // test-amp-story to avoid console.errors.
     let pageIds;
     if (this.pages_.length) {
       pageIds = this.pages_.map(page => page.element.id);

diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js
@@ -540,6 +540,54 @@ describes.realWin(
         });
     });
 
+    it('should deduplicate amp-story-page ids', async () => {
+      createPages(story.element, 6, [
+        'cover',
+        'page-1',
+        'cover',
+        'page-1',
+        'page-1',
+        'page-2',
+      ]);
+
+      allowConsoleError(() => story.buildCallback());
+      await story.layoutCallback();
+
+      const pages = story.element.querySelectorAll('amp-story-page');
+      const pageIds = Array.prototype.map.call(pages, page => page.id);
+      expect(pageIds).to.deep.equal([
+        'cover',
+        'page-1',
+        'cover__1',
+        'page-1__1',
+        'page-1__2',
+        'page-2',
+      ]);
+    });
+
+    it('should deduplicate amp-story-page ids and cache them', async () => {
+      createPages(story.element, 6, [
+        'cover',
+        'page-1',
+        'cover',
+        'page-1',
+        'page-1',
+        'page-2',
+      ]);
+
+      allowConsoleError(() => story.buildCallback());
+      await story.layoutCallback();
+
+      expect(story.storeService_.get(StateProperty.PAGE_IDS)).to.deep.equal([
+        'cover',
+        'page-1',
+        'cover__1',
+        'page-1__1',
+        'page-1__2',
+        'page-2',
+      ]);
+    });
+
     describe('amp-story consent', () => {
       it('should pause the story if there is a consent', () => {
         sandbox