Experiment to remove adsense/doubleclick centering css (#27816)

rebase
ampproject · Apr 24, 2020 · 1afb254 · 1afb254
1 parent 90f1f7f
commit 1afb254
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 31 deletions.
diff --git a/ads/google/a4a/utils.js b/ads/google/a4a/utils.js
@@ -47,6 +47,13 @@ const AmpAdImplementation = {
   AMP_AD_IFRAME_GET: '5',
 };
 
+/** @const {!{id: string, control: string, experiment: string}} */
+export const AMP_AD_NO_CENTER_CSS_EXP = {
+  id: 'amp-ad-no-center-css',
+  control: '21065897',
+  experiment: '21065898',
+};
+
 /** @const {!Object} */
 export const ValidAdContainerTypes = {
   'AMP-CAROUSEL': 'ac',

diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
@@ -21,11 +21,8 @@
 // extensions/amp-ad-network-${NETWORK_NAME}-impl directory.
 
 import {EXPERIMENT_INFO_MAP as AMPDOC_FIE_EXPERIMENT_INFO_MAP} from '../../../src/ampdoc-fie';
-import {AdsenseSharedState} from './adsense-shared-state';
-import {AmpA4A} from '../../amp-a4a/0.1/amp-a4a';
-import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
-import {Navigation} from '../../../src/service/navigation';
 import {
+  AMP_AD_NO_CENTER_CSS_EXP,
   QQID_HEADER,
   SANDBOX_HEADER,
   ValidAdContainerTypes,
@@ -41,6 +38,10 @@ import {
   isReportingEnabled,
   maybeAppendErrorParameter,
 } from '../../../ads/google/a4a/utils';
+import {AdsenseSharedState} from './adsense-shared-state';
+import {AmpA4A} from '../../amp-a4a/0.1/amp-a4a';
+import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
+import {Navigation} from '../../../src/service/navigation';
 import {ResponsiveState} from './responsive-state';
 import {Services} from '../../../src/services';
 import {
@@ -55,6 +56,7 @@ import {getData} from '../../../src/event-helper';
 import {getDefaultBootstrapBaseUrl} from '../../../src/3p-frame';
 import {
   getExperimentBranch,
+  isExperimentOn,
   randomlySelectUnsetExperiments,
 } from '../../../src/experiments';
 import {getMode} from '../../../src/mode';
@@ -221,6 +223,13 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
           Number(this.element.getAttribute('height')) > 0,
         branches: ['21062003', '21062004'],
       },
+      [AMP_AD_NO_CENTER_CSS_EXP.id]: {
+        isTrafficEligible: () => true,
+        branches: [
+          AMP_AD_NO_CENTER_CSS_EXP.control,
+          AMP_AD_NO_CENTER_CSS_EXP.experiment,
+        ],
+      },
       ...AMPDOC_FIE_EXPERIMENT_INFO_MAP,
     });
     const setExps = randomlySelectUnsetExperiments(this.win, experimentInfoMap);
@@ -487,6 +496,19 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
       width: `${this.size_.width}px`,
       height: `${this.size_.height}px`,
     });
+
+    // Set the centering CSS if the experiment is off
+    if (
+      !isExperimentOn(this.win, 'amp-ad-no-center-css') ||
+      getExperimentBranch(this.win, AMP_AD_NO_CENTER_CSS_EXP.id) ===
+        AMP_AD_NO_CENTER_CSS_EXP.control
+    ) {
+      setStyles(this.iframe, {
+        top: '50%',
+        left: '50%',
+        transform: 'translate(-50%, -50%)',
+      });
+    }
     if (this.qqid_) {
       this.element.setAttribute('data-google-query-id', this.qqid_);
     }

diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
@@ -488,8 +488,6 @@ describes.realWin(
       function verifyCss(iframe) {
         expect(iframe).to.be.ok;
         const style = win.getComputedStyle(iframe);
-        expect(style.top).to.equal('50%');
-        expect(style.left).to.equal('50%');
         // We expect these set, but the exact dimensions will be determined by the
         // IOb.
         expect(style.width).to.be.ok;
@@ -498,9 +496,7 @@ describes.realWin(
         // as this can vary depending on whether we use the height/width
         // attributes, or the actual size of the frame. To make this less of a
         // hassle, we'll just match against regexp.
-        expect(style.transform).to.match(
-          new RegExp('matrix\\(1, 0, 0, 1, -[0-9]+, -[0-9]+\\)')
-        );
+        expect(style.transform).to.equal('none');
       }
 
       it('centers iframe in slot when height && width', () => {

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
@@ -23,12 +23,7 @@
 import '../../amp-a4a/0.1/real-time-config-manager';
 import {EXPERIMENT_INFO_MAP as AMPDOC_FIE_EXPERIMENT_INFO_MAP} from '../../../src/ampdoc-fie';
 import {
-  AmpA4A,
-  DEFAULT_SAFEFRAME_VERSION,
-  XORIGIN_MODE,
-  assignAdUrlToError,
-} from '../../amp-a4a/0.1/amp-a4a';
-import {
+  AMP_AD_NO_CENTER_CSS_EXP,
   AmpAnalyticsConfigDef,
   QQID_HEADER,
   SANDBOX_HEADER,
@@ -48,6 +43,12 @@ import {
   maybeAppendErrorParameter,
   truncAndTimeUrl,
 } from '../../../ads/google/a4a/utils';
+import {
+  AmpA4A,
+  DEFAULT_SAFEFRAME_VERSION,
+  XORIGIN_MODE,
+  assignAdUrlToError,
+} from '../../amp-a4a/0.1/amp-a4a';
 import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
 import {Deferred} from '../../../src/utils/promise';
 import {
@@ -88,6 +89,11 @@ import {
   isInManualExperiment,
 } from '../../../ads/google/a4a/traffic-experiments';
 import {getCryptoRandomBytesArray, utf8Decode} from '../../../src/utils/bytes';
+import {
+  getExperimentBranch,
+  isExperimentOn,
+  randomlySelectUnsetExperiments,
+} from '../../../src/experiments';
 import {getMode} from '../../../src/mode';
 import {getMultiSizeDimensions} from '../../../ads/google/utils';
 import {getOrCreateAdCid} from '../../../src/ad-cid';
@@ -98,10 +104,6 @@ import {
 } from '../../amp-ad/0.1/concurrent-load';
 import {insertAnalyticsElement} from '../../../src/extension-analytics';
 import {isCancellation} from '../../../src/error';
-import {
-  isExperimentOn,
-  randomlySelectUnsetExperiments,
-} from '../../../src/experiments';
 import {
   lineDelimitedStreamer,
   metaJsonCreativeGrouper,
@@ -430,6 +432,13 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
           RANDOM_SUBDOMAIN_SAFEFRAME_BRANCHES.EXPERIMENT,
         ],
       },
+      [AMP_AD_NO_CENTER_CSS_EXP.id]: {
+        isTrafficEligible: () => true,
+        branches: [
+          [AMP_AD_NO_CENTER_CSS_EXP.control],
+          [AMP_AD_NO_CENTER_CSS_EXP.experiment],
+        ],
+      },
       ...AMPDOC_FIE_EXPERIMENT_INFO_MAP,
     });
     const setExps = this.randomlySelectUnsetExperiments_(experimentInfoMap);
@@ -1144,6 +1153,20 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
       height: `${size.height}px`,
       position: isMultiSizeFluid ? 'relative' : null,
     });
+
+    // Set the centering CSS if the experiment is off
+    if (
+      !isExperimentOn(this.win, 'amp-ad-no-center-css') ||
+      getExperimentBranch(this.win, AMP_AD_NO_CENTER_CSS_EXP.id) ===
+        AMP_AD_NO_CENTER_CSS_EXP.control
+    ) {
+      setStyles(this.iframe, {
+        top: '50%',
+        left: '50%',
+        transform: 'translate(-50%, -50%)',
+      });
+    }
+
     if (this.qqid_) {
       this.element.setAttribute('data-google-query-id', this.qqid_);
     }

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
@@ -1546,17 +1546,13 @@ describes.realWin(
       function verifyCss(iframe, expectedSize) {
         expect(iframe).to.be.ok;
         const style = env.win.getComputedStyle(iframe);
-        expect(style.top).to.equal('50%');
-        expect(style.left).to.equal('50%');
         expect(style.width).to.equal(expectedSize.width);
         expect(style.height).to.equal(expectedSize.height);
         // We don't know the exact values by which the frame will be
         // translated, as this can vary depending on whether we use the
         // height/width attributes, or the actual size of the frame. To make
         // this less of a hassle, we'll just match against regexp.
-        expect(style.transform).to.match(
-          new RegExp('matrix\\(1, 0, 0, 1, -[0-9]+, -[0-9]+\\)')
-        );
+        expect(style.transform).to.equal('none');
       }
 
       afterEach(() => env.win.document.body.removeChild(impl.element));

diff --git a/extensions/amp-ad/0.1/amp-ad.css b/extensions/amp-ad/0.1/amp-ad.css
@@ -50,13 +50,6 @@ amp-embed iframe {
   border: 1px, solid, #696969;
 }
 
-amp-ad[data-a4a-upgrade-type="amp-ad-network-doubleclick-impl"] > iframe,
-amp-ad[type="adsense"] > iframe {
-  top: 50% !important;
-  left: 50% !important;
-  transform: translate(-50%, -50%);
-}
-
 amp-ad[type="adsense"], amp-ad[type="doubleclick"] {
   direction: ltr;
 }

diff --git a/tools/experiments/experiments-config.js b/tools/experiments/experiments-config.js
@@ -321,4 +321,9 @@ export const EXPERIMENTS = [
     spec: 'TODO: add before merge',
     cleanupIssue: 'TODO: create issue before merge',
   },
+  {
+    id: 'amp-ad-no-center-css',
+    name: 'Removing the centering css rule for amp-ad',
+    spec: 'https://github.com/ampproject/amphtml/issues/27095',
+  },
 ];