Skip to content

Commit

Permalink
🐛 Bug fix: Limit the Full-Width Responsive upgrade in Auto Ads to mo…
Browse files Browse the repository at this point in the history
…bile users only. (#28376)

* 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.
  • Loading branch information
Jiaming-X committed May 13, 2020
1 parent 7593fed commit e7a08bb
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 10 deletions.
26 changes: 22 additions & 4 deletions extensions/amp-auto-ads/0.1/placement.js
Expand Up @@ -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.
Expand Down Expand Up @@ -296,7 +300,7 @@ export class Placement {
* @return {!Element}
* @private
*/
createResponsiveAdElement_(baseAttributes) {
createFullWidthResponsiveAdElement_(baseAttributes) {
const attributes = /** @type {!JsonObject} */ (Object.assign(
dict({
'width': '100vw',
Expand All @@ -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;
}
}

/**
Expand Down
69 changes: 63 additions & 6 deletions extensions/amp-auto-ads/0.1/test/test-placement.js
Expand Up @@ -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: [
Expand Down Expand Up @@ -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';
Expand Down

0 comments on commit e7a08bb

Please sign in to comment.