From e7a08bb5e9f7a664399695bf182b5fc590dcf5cc Mon Sep 17 00:00:00 2001
From: Jiaming <jiamingxie@google.com>
Date: Wed, 13 May 2020 22:57:41 +0100
Subject: [PATCH] =?UTF-8?q?=20=F0=9F=90=9B=20Bug=20fix:=20Limit=20the=20Fu?=
 =?UTF-8?q?ll-Width=20Responsive=20upgrade=20in=20Auto=20Ads=20to=20mobile?=
 =?UTF-8?q?=20users=20only.=20(#28376)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Limit the Full-Width Responsive upgrade in Auto Ads to mobile users only.

* Update the unit tests for the Auto-Ad responsive upgrade changes.

* Add comment for the threshold and update the width in the related test cases.
---
 extensions/amp-auto-ads/0.1/placement.js      | 26 +++++--
 .../amp-auto-ads/0.1/test/test-placement.js   | 69 +++++++++++++++++--
 2 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/extensions/amp-auto-ads/0.1/placement.js b/extensions/amp-auto-ads/0.1/placement.js
index 90f236383a53..49a00743f002 100644
--- a/extensions/amp-auto-ads/0.1/placement.js
+++ b/extensions/amp-auto-ads/0.1/placement.js
@@ -199,13 +199,17 @@ export class Placement {
           this.state_ = PlacementState.TOO_NEAR_EXISTING_AD;
           return this.state_;
         }
-        this.adElement_ = isResponsiveEnabled
-          ? this.createResponsiveAdElement_(baseAttributes)
+
+        const shouldUseFullWidthResponsive =
+          isResponsiveEnabled &&
+          this.isLayoutViewportNarrow_(this.anchorElement_);
+        this.adElement_ = shouldUseFullWidthResponsive
+          ? this.createFullWidthResponsiveAdElement_(baseAttributes)
           : this.createAdElement_(baseAttributes, sizing.width);
 
         this.injector_(this.anchorElement_, this.getAdElement());
 
-        if (isResponsiveEnabled) {
+        if (shouldUseFullWidthResponsive) {
           return (
             whenUpgradedToCustomElement(this.getAdElement())
               // Responsive ads set their own size when built.
@@ -296,7 +300,7 @@ export class Placement {
    * @return {!Element}
    * @private
    */
-  createResponsiveAdElement_(baseAttributes) {
+  createFullWidthResponsiveAdElement_(baseAttributes) {
     const attributes = /** @type {!JsonObject} */ (Object.assign(
       dict({
         'width': '100vw',
@@ -315,6 +319,20 @@ export class Placement {
       attributes
     );
   }
+
+  /**
+   * Estimate if the viewport has a narrow layout.
+   * @param {!Element} element
+   * @return {boolean}
+   * @private
+   */
+  isLayoutViewportNarrow_(element) {
+    const viewportSize = Services.viewportForDoc(element).getSize();
+
+    // The threshold aligns with the one for Non-AMP website. Checkout
+    // isLayoutViewportNarrow in responsive_util.js for internal reference.
+    return viewportSize.width < 488;
+  }
 }
 
 /**
diff --git a/extensions/amp-auto-ads/0.1/test/test-placement.js b/extensions/amp-auto-ads/0.1/test/test-placement.js
index 8dd54f4097e5..0475d84ab4b8 100644
--- a/extensions/amp-auto-ads/0.1/test/test-placement.js
+++ b/extensions/amp-auto-ads/0.1/test/test-placement.js
@@ -608,16 +608,18 @@ describes.realWin(
         });
       });
 
-      it('should set the correct attributes for responsive enabled ads using amp-ad responsive', () => {
+      it('should set the full-with responsive attributes for responsive enabled users on narrow viewport.', () => {
         const anchor = doc.createElement('div');
         anchor.id = 'anId';
         container.appendChild(anchor);
 
-        const mutator = Services.mutatorForDoc(anchor);
-        env.sandbox.stub(mutator, 'requestChangeSize').callsFake(() => {
-          return Promise.resolve();
-        });
-        env.sandbox.stub(mutator.viewport_, 'getWidth').callsFake(() => 2000);
+        const viewportMock = env.sandbox.mock(
+          Services.viewportForDoc(env.win.document)
+        );
+        viewportMock
+          .expects('getSize')
+          .returns({width: 487, height: 823})
+          .atLeast(1);
 
         const placements = getPlacementsFromConfigObj(ampdoc, {
           placements: [
@@ -661,6 +663,61 @@ describes.realWin(
           });
       });
 
+      it('should not set the full-with responsive attributes for responsive enabled users on wide viewport.', () => {
+        const anchor = doc.createElement('div');
+        anchor.id = 'anId';
+        container.appendChild(anchor);
+
+        const viewportMock = env.sandbox.mock(
+          Services.viewportForDoc(env.win.document)
+        );
+        viewportMock
+          .expects('getSize')
+          .returns({width: 488, height: 1000})
+          .atLeast(1);
+
+        const placements = getPlacementsFromConfigObj(ampdoc, {
+          placements: [
+            {
+              anchor: {
+                selector: 'DIV#anId',
+              },
+              pos: 2,
+              type: 1,
+            },
+          ],
+        });
+        expect(placements).to.have.lengthOf(1);
+
+        const attributes = {
+          'type': '_ping_',
+        };
+
+        const sizing = {};
+        const adTracker = new AdTracker([], {
+          initialMinSpacing: 0,
+          subsequentMinSpacing: [],
+          maxAdCount: 10,
+        });
+
+        return placements[0]
+          .placeAd(attributes, sizing, adTracker, true)
+          .then((placementState) => {
+            const adElement = anchor.firstChild;
+            expect(adElement.tagName).to.equal('AMP-AD');
+            expect(adElement.getAttribute('type')).to.equal('_ping_');
+            expect(adElement.getAttribute('layout')).to.equal('fixed-height');
+            expect(adElement.getAttribute('height')).to.equal('0');
+            expect(adElement.hasAttribute('data-auto-format')).to.be.false;
+            expect(adElement.hasAttribute('data-full-width')).to.be.false;
+            expect(adElement.style.marginTop).to.equal('');
+            expect(adElement.style.marginBottom).to.equal('');
+            expect(adElement.style.marginLeft).to.equal('');
+            expect(adElement.style.marginRight).to.equal('');
+            expect(placementState).to.equal(PlacementState.PLACED);
+          });
+      });
+
       it('should report placement placed when resize allowed', () => {
         const anchor = doc.createElement('div');
         anchor.id = 'anId';