A4A: Do not set visibility hidden for iframe attached via A4A flow (#…

…4215)

* Do not set visibility hidden when rendering via iframe from A4A

* Fixes related to review

* Do not set visibility hidden when rendering via iframe from A4A

* Fixes related to review

* fix test failure

* Add missing semi-colon

extensions/amp-a4a/0.1/amp-a4a.js

@@ -624,7 +624,7 @@ export class AmpA4A extends AMP.BaseElement {
    * @private
    */
   renderViaIframe_(opt_isNonAmpCreative) {
-    user.assert(this.adUrl_, 'creative missing in renderViaIframe_?');
+    user.assert(this.adUrl_, 'adUrl missing in renderViaIframe_?');
     const iframe = this.element.ownerDocument.createElement('iframe');
     iframe.setAttribute('height', this.element.getAttribute('height'));
     iframe.setAttribute('width', this.element.getAttribute('width'));
@@ -638,7 +638,10 @@ export class AmpA4A extends AMP.BaseElement {
       // TODO(keithwrightbos): noContentCallback?
       this.apiHandler_ = new AmpAdApiHandler(this, this.element);
       // TODO(keithwrightbos): startup returns load event, do we need to wait?
-      this.apiHandler_.startUp(iframe, opt_isNonAmpCreative);
+      // Set opt_defaultVisible to true as 3p draw code never executed causing
+      // render-start event never to fire which will remove visiblity hidden.
+      this.apiHandler_.startUp(
+        iframe, /* is3p */opt_isNonAmpCreative, /* opt_defaultVisible */true);
     });
   }
 

extensions/amp-a4a/0.1/test/test-amp-a4a.js

@@ -399,6 +399,34 @@ describe('amp-a4a', () => {
           JSON.stringify(offsets) + '</script>' +
           baseTestDoc.slice(splicePoint));
     }
+    it('should not render AMP natively', () => {
+      return createAdTestingIframePromise().then(fixture => {
+        const doc = fixture.doc;
+        const a4aElement = doc.createElement('amp-a4a');
+        const a4a = new AmpA4A(a4aElement);
+        a4a.adUrl_ = 'http://foo.com';
+        a4a.maybeRenderAmpAd_ = function() { return Promise.resolve(false); };
+        return a4a.maybeRenderAmpAd_().then(rendered => {
+          // Force vsync system to run all queued tasks, so that DOM mutations
+          // are actually completed before testing.
+          a4a.vsync_.runScheduledTasks_();
+          expect(rendered).to.be.false;
+          expect(a4aElement.shadowRoot).to.be.null;
+          expect(a4a.rendered_).to.be.false;
+          // Force layout callback which will cause iframe to be attached
+          a4a.adPromise_ = Promise.resolve(false);
+          return a4a.layoutCallback().then(() => {
+            a4a.vsync_.runScheduledTasks_();
+            // Verify iframe presence and lack of visibility hidden
+            expect(a4aElement.children.length).to.equal(1);
+            const iframe = a4aElement.children[0];
+            expect(iframe.tagName).to.equal('IFRAME');
+            expect(iframe.src.indexOf('http://foo.com')).to.equal(0);
+            expect(iframe.style.visibility).to.equal('');
+          });
+        });
+      });
+    });
     it('should render AMP natively', () => {
       return createAdTestingIframePromise().then(fixture => {
         const doc = fixture.doc;

extensions/amp-ad/0.1/amp-ad-api-handler.js

@@ -63,11 +63,14 @@ export class AmpAdApiHandler {
   }
 
   /**
+   * Sets up listeners and iframe state for iframe containing ad creative.
    * @param {!Element} iframe
-   * @param {boolean} is3p
+   * @param {boolean} is3p whether iframe was loaded via 3p.
+   * @param {boolean} opt_defaultVisible when true, visibility hidden is NOT
+   *    set on the iframe element (remains visible
    * @return {!Promise} awaiting load event for ad frame
    */
-  startUp(iframe, is3p) {
+  startUp(iframe, is3p, opt_defaultVisible) {
     user.assert(
       !this.iframe, 'multiple invocations of startup without destroy!');
     this.iframe_ = iframe;
@@ -110,7 +113,7 @@ export class AmpAdApiHandler {
         this.updateSize_(newHeight, newWidth);
       }
     }, this.is3p_));
-    if (this.is3p_) {
+    if (!opt_defaultVisible) {
       // NOTE(tdrl,keithwrightbos): This will not work for A4A with an AMP
       // creative as it will not expect having to send the render-start message.
       this.iframe_.style.visibility = 'hidden';