🚀 Make sure amp-story ad is loaded before display (#13618)

* check if ad loaded * add created count
ampproject · Feb 23, 2018 · 3a2d476 · 3a2d476
1 parent 19e6e78
commit 3a2d476
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 25 deletions.
diff --git a/extensions/amp-story/0.1/amp-story-auto-ads.js b/extensions/amp-story/0.1/amp-story-auto-ads.js
@@ -14,21 +14,26 @@
  * limitations under the License.
  */
 
+import {CommonSignals} from '../../../src/common-signals';
+import {Services} from '../../../src/services';
 import {StateChangeType} from './navigation-state';
+import {createElementWithAttributes} from '../../../src/dom';
 import {dev, user} from '../../../src/log';
+import {dict} from '../../../src/utils/object';
 
-// temp before config in passed in
+
+// TODO(ccordry) replace these constants with user config
 /** @const */
 const MIN_INTERVAL = 3;
 
 /** @const */
 const MAX_NUMBER = 2;
 
 /** @const */
-// const EXPERIMENT = 'amp-story-auto-ad';
+const TAG = 'amp-story-auto-ads';
 
 /** @const */
-const TAG = 'amp-story-auto-ads';
+const AD_TAG = 'amp-ad';
 
 export class AmpStoryAutoAds extends AMP.BaseElement {
 
@@ -46,10 +51,16 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
     this.interactions_ = 0;
 
     /** @private {!Array} */
-    this.adElements_ = [];
+    this.adPageEls_ = [];
 
     /** @private {number} */
     this.adsPlaced_ = 0;
+
+    /** @private {number} */
+    this.adPagesCreated_ = 0;
+
+    /** @private {boolean} */
+    this.isCurrentAdLoaded_ = false;
   }
 
   /** @override */
@@ -58,6 +69,10 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
     user().assert(ampStoryElement.tagName === 'AMP-STORY',
         `<${TAG}> should be child of <amp-story>`);
 
+    const ampdoc = this.getAmpDoc();
+    Services.extensionsFor(this.win)./*OK*/installExtensionForDoc(
+        ampdoc, AD_TAG);
+
     ampStoryElement.getImpl().then(impl => {
       this.ampStory_ = impl;
       this.navigationState_ = this.ampStory_.getNavigationState();
@@ -78,43 +93,84 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
     return Promise.resolve();
   }
 
+
   /**
    * build page and start preloading
    * @private
    */
   schedulePage_() {
-    const page = this.makeMockPage();
-    this.adElements_.push(page);
+    const page = this.createAdPage_();
+    this.adPageEls_.push(page);
 
     this.ampStory_.element.appendChild(page);
 
-    // TODO(ccordry): need to fake distance from page to force load
-
     page.getImpl().then(impl => {
       this.ampStory_.addPage(impl);
     });
   }
 
+
   /**
-   * temporary to be replaced with real fetching
+   * create an `amp-story-page` containing an `amp-ad`
+   * @private
    */
-  makeMockPage() {
-    const ampStoryAdPage = document.createElement('amp-story-page');
-    const id = this.adsPlaced_ + 1;
-    ampStoryAdPage.id = `i-amphtml-ad-page-${id}`;
-    ampStoryAdPage.setAttribute('ad', '');
-    ampStoryAdPage./*OK*/innerHTML = `
-      <amp-story-grid-layer template="vertical">
-        <h1>Ad Page #${id}</h1>
-        <p>This is ad #${id} shown in this story.</p>
-      </amp-story-grid-layer>
-      `;
+  createAdPage_() {
+    // TODO(ccordry) add new <amp-story-cta-layer>
+    const ampStoryAdPage = this.createPageElement_();
+    const ampAd = this.createAdElement_();
+
+    ampStoryAdPage.appendChild(ampAd);
+
+    this.isCurrentAdLoaded_ = false;
+
+    // set up listener for ad-loaded event
+    ampAd.getImpl().then(impl => {
+      const signals = impl.signals();
+      return signals.whenSignal(CommonSignals.INI_LOAD);
+    }).then(() => {
+      this.isCurrentAdLoaded_ = true;
+    });
+
     return ampStoryAdPage;
   }
 
 
   /**
-   * Get array containing all pages of associated story
+   * @return {!Element}
+   * @private
+   */
+  createPageElement_() {
+    const id = ++this.adPagesCreated_;
+    const attributes = dict({
+      'id': `i-amphtml-ad-page-${id}`,
+      'ad': '',
+      'distance': '1',
+    });
+
+    return createElementWithAttributes(
+        document, 'amp-story-page', attributes);
+  }
+
+
+  /**
+   * @return {!Element}
+   * @private
+   */
+  createAdElement_() {
+    // TODO(ccordry) get this info (e.g. source) from config
+    const attributes = dict({
+      'id': 'i-amphtml-demo-ad',
+      'height': '300',
+      'src': '/extensions/amp-ad-network-fake-impl/0.1/data/fake_amp.json',
+      'type': 'fake',
+    });
+
+    return createElementWithAttributes(
+        document, 'amp-ad', attributes);
+  }
+
+
+  /**
    * @private
    */
   handleStateChange_(stateChangeEvent) {
@@ -158,14 +214,13 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
    * @private
    */
   placeAdAfterPage_(currentPageId) {
-    // TODO(ccordry) make sure ad is loaded
-    const nextAdElement = this.adElements_[this.adElements_.length - 1];
+    const nextAdPageEl = this.adPageEls_[this.adPageEls_.length - 1];
 
-    if (!nextAdElement) {
+    if (!nextAdPageEl || !this.isCurrentAdLoaded_) {
       return;
     }
 
-    this.ampStory_.insertPage(currentPageId, nextAdElement.id);
+    this.ampStory_.insertPage(currentPageId, nextAdPageEl.id);
     this.adsPlaced_++;
 
     if (!this.allAdsPlaced_()) {

diff --git a/extensions/amp-story/0.1/amp-story-page.js b/extensions/amp-story/0.1/amp-story-page.js
@@ -701,6 +701,11 @@ export class AmpStoryPage extends AMP.BaseElement {
     });
   }
 
+
+  /**
+   * check to see if this page is a wrapper for an ad
+   * @return {boolean}
+   */
   isAd() {
     return this.element.hasAttribute(ADVERTISEMENT_ATTR_NAME);
   }