🐛 Add 'no-fill' attribute from AdsenseNetworkConfig.

extensions/amp-auto-ads/0.1/amp-auto-ads.js

@@ -21,9 +21,9 @@ import {
   getExistingAds,
 } from './ad-tracker';
 import {AnchorAdStrategy} from './anchor-ad-strategy';
+import {Attributes, getAttributesFromConfigObj} from './attributes';
 import {Services} from '../../../src/services';
 import {getAdNetworkConfig} from './ad-network-config';
-import {getAttributesFromConfigObj} from './attributes';
 import {getPlacementsFromConfigObj} from './placement';
 import {isExperimentOn} from '../../../src/experiments';
 import {userAssert} from '../../../src/log';
@@ -75,7 +75,7 @@ export class AmpAutoAds extends AMP.BaseElement {
         const placements = getPlacementsFromConfigObj(ampdoc, configObj);
         const attributes = /** @type {!JsonObject} */ (Object.assign(
           adNetwork.getAttributes(),
-          getAttributesFromConfigObj(configObj)
+          getAttributesFromConfigObj(configObj, Attributes.BASE_ATTRIBUTES)
         ));
         const sizing = adNetwork.getSizing();
         const adConstraints =
@@ -89,7 +89,11 @@ export class AmpAutoAds extends AMP.BaseElement {
           adTracker,
           adNetwork.isResponsiveEnabled()
         ).run();
-        new AnchorAdStrategy(ampdoc, attributes, configObj).run();
+        const stickyAdAttributes = /** @type {!JsonObject} */ (Object.assign(
+          attributes,
+          getAttributesFromConfigObj(configObj, Attributes.STICKY_AD_ATTRIBUTES)
+        ));
+        new AnchorAdStrategy(ampdoc, stickyAdAttributes, configObj).run();
       });
   }
 

extensions/amp-auto-ads/0.1/anchor-ad-strategy.js

@@ -13,14 +13,14 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import {OptInStatus} from './opt-in-status';
 import {Services} from '../../../src/services';
 import {createElementWithAttributes} from '../../../src/dom';
 import {dict} from '../../../src/utils/object';
 import {user} from '../../../src/log';
 
 const TAG = 'amp-auto-ads';
 const STICKY_AD_TAG = 'amp-sticky-ad';
+const OPT_IN_STATUS_ANCHOR_ADS = 2;
 
 export class AnchorAdStrategy {
   /**
@@ -44,25 +44,15 @@ export class AnchorAdStrategy {
    * @return {!Promise<boolean>} Resolves when the strategy is complete.
    */
   run() {
-    const filledAnchorEnabled = this.isFilledAnchorAdEnabled_();
-    const noFillAnchorEnabled = this.isNoFillAnchorAdEnabled_();
-
     if (this.hasExistingStickyAd_()) {
-      if (noFillAnchorEnabled) {
-        user().warn(
-          TAG,
-          'Auto ads may not work because of already existing <amp-sticky-ad>.'
-        );
-      } else {
-        user().warn(
-          TAG,
-          'Auto Ads is unable to create an anchor ad because of already existing <amp-sticky-ad>.'
-        );
-      }
+      user().warn(
+        TAG,
+        'Auto ads may not work because of already existing <amp-sticky-ad>.'
+      );
       return Promise.resolve(false);
     }
 
-    if (!filledAnchorEnabled && !noFillAnchorEnabled) {
+    if (!this.isAnchorAdEnabled_()) {
       return Promise.resolve(false);
     }
 
@@ -71,7 +61,7 @@ export class AnchorAdStrategy {
       STICKY_AD_TAG,
       '1.0'
     );
-    this.placeStickyAd_(noFillAnchorEnabled && !filledAnchorEnabled);
+    this.placeStickyAd_();
     return Promise.resolve(true);
   }
 
@@ -87,27 +77,16 @@ export class AnchorAdStrategy {
    * @return {boolean}
    * @private
    */
-  isFilledAnchorAdEnabled_() {
-    return user()
-      .assertArray(this.configObj_['optInStatus'] || [])
-      .includes(OptInStatus.OPT_IN_STATUS_ANCHOR_ADS);
-  }
-
-  /**
-   * @return {boolean}
-   * @private
-   */
-  isNoFillAnchorAdEnabled_() {
+  isAnchorAdEnabled_() {
     return user()
       .assertArray(this.configObj_['optInStatus'] || [])
-      .includes(OptInStatus.OPT_IN_STATUS_NO_FILL_ANCHOR_ADS);
+      .includes(OPT_IN_STATUS_ANCHOR_ADS);
   }
 
   /**
-   * @param {boolean} noFill
    * @private
    */
-  placeStickyAd_(noFill) {
+  placeStickyAd_() {
     const viewportWidth = Services.viewportForDoc(this.ampdoc).getWidth();
     const attributes = /** @type {!JsonObject} */ (Object.assign(
       dict(),
@@ -124,7 +103,6 @@ export class AnchorAdStrategy {
       'amp-sticky-ad',
       dict({
         'layout': 'nodisplay',
-        'data-no-fill': String(noFill),
       })
     );
     stickyAd.appendChild(ampAd);

extensions/amp-auto-ads/0.1/attributes.js

@@ -29,19 +29,31 @@ const NON_DATA_ATTRIBUTE_WHITELIST = {
   'type': true,
 };
 
+/**
+ * Indicates attributes from config object for different ad formats.
+ * @enum {string}
+ */
+export const Attributes = {
+  // Attributes from config object which should be added on any ads.
+  BASE_ATTRIBUTES: 'attributes',
+  // Attributes from config object which should be added on anchor ads.
+  STICKY_AD_ATTRIBUTES: 'stickyAdAttributes',
+};
+
 /**
  * @param {!JsonObject} configObj
+ * @param {!Attributes} attributes
  * @return {!JsonObject<string, string>}
  */
-export function getAttributesFromConfigObj(configObj) {
-  if (!configObj['attributes']) {
+export function getAttributesFromConfigObj(configObj, attributes) {
+  if (!configObj[attributes]) {
     return dict();
   }
-  if (!isObject(configObj['attributes']) || isArray(configObj['attributes'])) {
-    user().warn(TAG, 'attributes property not an object');
+  if (!isObject(configObj[attributes]) || isArray(configObj[attributes])) {
+    user().warn(TAG, attributes + ' property not an object');
     return dict();
   }
-  return parseAttributes(configObj['attributes']);
+  return parseAttributes(configObj[attributes]);
 }
 
 /**

extensions/amp-auto-ads/0.1/placement.js

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import {Attributes, getAttributesFromConfigObj} from './attributes';
 import {
   LayoutMarginsChangeDef,
   cloneLayoutMarginsChangeDef,
@@ -29,7 +30,6 @@ import {
 import {computedStyle} from '../../../src/style';
 import {dev, user} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
-import {getAttributesFromConfigObj} from './attributes';
 
 /** @const */
 const TAG = 'amp-auto-ads';
@@ -376,7 +376,10 @@ function getPlacementsFromObject(ampdoc, placementObj, placements) {
     if (!isPositionValid(anchorElement, placementObj['pos'])) {
       return;
     }
-    const attributes = getAttributesFromConfigObj(placementObj);
+    const attributes = getAttributesFromConfigObj(
+      placementObj,
+      Attributes.BASE_ATTRIBUTES
+    );
     placements.push(
       new Placement(
         ampdoc,

extensions/amp-auto-ads/0.1/test/test-amp-auto-ads.js

@@ -404,6 +404,32 @@ describes.realWin(
           });
         });
       });
+
+      it('should insert anchor anchor ad with provided anchor ad attributes.', () => {
+        configObj = {
+          optInStatus: [2],
+        };
+        configObj.stickyAdAttributes = {
+          'data-no-fill': 'true',
+        };
+
+        return getAmpAutoAds().then(() => {
+          return new Promise(resolve => {
+            waitForChild(
+              env.win.document.body,
+              parent => {
+                return parent.firstChild.tagName == 'AMP-STICKY-AD';
+              },
+              () => {
+                const stickyAd = env.win.document.body.firstChild;
+                const ampAd = stickyAd.firstChild;
+                expect(ampAd.getAttribute('data-no-fill')).to.equal('true');
+                resolve();
+              }
+            );
+          });
+        });
+      });
     });
 
     describe('ad constraints', () => {

extensions/amp-auto-ads/0.1/test/test-anchor-ad-strategy.js

@@ -48,6 +48,7 @@ describes.realWin(
       attributes = {
         'data-ad-client': 'ca-pub-test',
         'type': 'adsense',
+        'data-no-fill': 'true',
       };
     });
 
@@ -81,6 +82,7 @@ describes.realWin(
               expect(ampAd.getAttribute('data-ad-client')).to.equal(
                 'ca-pub-test'
               );
+              expect(ampAd.getAttribute('data-no-fill')).to.equal('true');
               resolve();
             }
           );
@@ -89,7 +91,7 @@ describes.realWin(
         return Promise.all([strategyPromise, expectPromise]);
       });
 
-      it('should not insert sticky ad if not opted in anchor ad or no fill anchor ad', () => {
+      it('should not insert sticky ad if not opted in anchor ad', () => {
         const anchorAdStrategy = new AnchorAdStrategy(
           env.ampdoc,
           attributes,
@@ -141,100 +143,6 @@ describes.realWin(
 
         return Promise.all([strategyPromise, expectPromise]);
       });
-
-      it('should set data-no-fill to false if opted in for anchor ads', () => {
-        configObj['optInStatus'].push(2);
-
-        const anchorAdStrategy = new AnchorAdStrategy(
-          env.ampdoc,
-          attributes,
-          configObj
-        );
-
-        const strategyPromise = anchorAdStrategy.run().then(placed => {
-          expect(placed).to.equal(true);
-        });
-
-        const expectPromise = new Promise(resolve => {
-          waitForChild(
-            env.win.document.body,
-            parent => {
-              return parent.firstChild.tagName == 'AMP-STICKY-AD';
-            },
-            () => {
-              const stickyAd = env.win.document.body.firstChild;
-              expect(stickyAd.getAttribute('layout')).to.equal('nodisplay');
-              expect(stickyAd.getAttribute('data-no-fill')).to.equal('false');
-              resolve();
-            }
-          );
-        });
-
-        return Promise.all([strategyPromise, expectPromise]);
-      });
-
-      it('should set data-no-fill to true if opted in only for no fill anchor ads', () => {
-        configObj['optInStatus'].push(4);
-
-        const anchorAdStrategy = new AnchorAdStrategy(
-          env.ampdoc,
-          attributes,
-          configObj
-        );
-
-        const strategyPromise = anchorAdStrategy.run().then(placed => {
-          expect(placed).to.equal(true);
-        });
-
-        const expectPromise = new Promise(resolve => {
-          waitForChild(
-            env.win.document.body,
-            parent => {
-              return parent.firstChild.tagName == 'AMP-STICKY-AD';
-            },
-            () => {
-              const stickyAd = env.win.document.body.firstChild;
-              expect(stickyAd.getAttribute('layout')).to.equal('nodisplay');
-              expect(stickyAd.getAttribute('data-no-fill')).to.equal('true');
-              resolve();
-            }
-          );
-        });
-
-        return Promise.all([strategyPromise, expectPromise]);
-      });
-
-      it('should set data-no-fill to false if opted in for both filled anchor ads and no fill anchor ads', () => {
-        configObj['optInStatus'].push(2);
-        configObj['optInStatus'].push(4);
-
-        const anchorAdStrategy = new AnchorAdStrategy(
-          env.ampdoc,
-          attributes,
-          configObj
-        );
-
-        const strategyPromise = anchorAdStrategy.run().then(placed => {
-          expect(placed).to.equal(true);
-        });
-
-        const expectPromise = new Promise(resolve => {
-          waitForChild(
-            env.win.document.body,
-            parent => {
-              return parent.firstChild.tagName == 'AMP-STICKY-AD';
-            },
-            () => {
-              const stickyAd = env.win.document.body.firstChild;
-              expect(stickyAd.getAttribute('layout')).to.equal('nodisplay');
-              expect(stickyAd.getAttribute('data-no-fill')).to.equal('false');
-              resolve();
-            }
-          );
-        });
-
-        return Promise.all([strategyPromise, expectPromise]);
-      });
     });
   }
 );