wait for document visible (#14656)

ampproject · Apr 17, 2018 · f238db5 · f238db5
1 parent 0549d07
commit f238db5
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 10 deletions.
diff --git a/examples/amp-consent.amp.html b/examples/amp-consent.amp.html
@@ -168,14 +168,19 @@ <h3>Image that is NOT blocked by consent</h3>
             "checkConsentHref": "http://localhost:8000/get-consent-v1",
             "promptUI": "ui1"
           }
-        }
+        },
+        "postPromptUI": "postPromptUI"
       }</script>
       <div id="ui1">
         Can I load the image??
         <button on="tap:ABC.accept" role="button">Accept</button>
         <button on="tap:ABC.reject" role="button">Reject</button>
         <button on="tap:ABC.dismiss" role="button">Dismiss</button>
       </div>
+      <div id="postPromptUI">
+        Post Prompt UI
+        <button on="tap:ABC.prompt" role="button">Manage</button>
+      </div>
     </amp-consent>
     <article>
 

diff --git a/extensions/amp-consent/0.1/amp-consent.js b/extensions/amp-consent/0.1/amp-consent.js
@@ -153,7 +153,6 @@ export class AmpConsent extends AMP.BaseElement {
             .then(manager => {
               this.notificationUiManager_ = manager;
             });
-
     Promise.all([consentStateManagerPromise, notificationUiManagerPromise])
         .then(() => {
           this.init_();
@@ -285,20 +284,20 @@ export class AmpConsent extends AMP.BaseElement {
    */
   init_() {
     const instanceKeys = Object.keys(this.consentConfig_);
+    const initPromptPromises = [];
     for (let i = 0; i < instanceKeys.length; i++) {
       const instanceId = instanceKeys[i];
       this.consentStateManager_.registerConsentInstance(instanceId);
-      this.getConsentRemote_(instanceId).then(response => {
+      const promise = this.getConsentRemote_(instanceId).then(response => {
         this.parseConsentResponse_(instanceId, response);
         this.handlePromptUI_(instanceId);
       }).catch(unusedError => {
         // TODO: Handle errors
       });
+      initPromptPromises.push(promise);
     }
 
-    // TODO(@zhouyx): Use setTimeout to make sure we handle postPromptUI
-    // after all prompt UI registerd. Make handle PromptUI a promise instead.
-    this.win.setTimeout(() => {
+    Promise.all(initPromptPromises).then(() => {
       this.handlePostPromptUI_();
     });
 
@@ -344,9 +343,12 @@ export class AmpConsent extends AMP.BaseElement {
     const href =
         this.consentConfig_[instanceId]['checkConsentHref'];
     assertHttpsUrl(href, this.element);
-    return Services.xhrFor(this.win)
-        .fetchJson(href, init)
-        .then(res => res.json());
+    const viewer = Services.viewerForDoc(this.getAmpDoc());
+    return viewer.whenFirstVisible().then(() => {
+      return Services.xhrFor(this.win)
+          .fetchJson(href, init)
+          .then(res => res.json());
+    });
   }
 
 

diff --git a/extensions/amp-consent/0.1/test/test-amp-consent.js b/extensions/amp-consent/0.1/test/test-amp-consent.js
@@ -337,13 +337,13 @@ describes.realWin('amp-consent', {
       });
 
       it('handle postPromptUI', function* () {
-        yield macroTask();
         expect(ampConsent.postPromptUI_).to.not.be.null;
         expect(computedStyle(ampConsent.win, ampConsent.element)['display'])
             .to.equal('none');
         expect(computedStyle(ampConsent.win, ampConsent.postPromptUI_)
             ['display']).to.equal('none');
         yield macroTask();
+
         expect(computedStyle(ampConsent.win, ampConsent.element)['display'])
             .to.not.equal('none');
         expect(ampConsent.element.classList.contains('amp-active')).to.be.true;