Skip to content

Commit

Permalink
fix: googletag.display errors when ad slot not in DOM (#142)
Browse files Browse the repository at this point in the history 
## Description
This PR fixes an issue where GPT complains if googletag.displaySlots()
is called for a defined ad slot that is not yet in the DOM, i.e.
```
GPT] Error in googletag.display: could not find div with id "my-ad-id" in DOM for slot: /my/ad/unit/path
``` 

- The fix is to defer the calling of `Advertising.displaySlots()`. So
instead calling this method in `Advertising.setupGpt()`, we are now
calling it at the end of `Advertising.active()`.
- I've also refactored displaySlots() to accept an id and to only call
`window.googletag.display(id)` for this single `id`, instead of
iterating through all the slots
- `Advertising.active()` gets called from the `AdvertisingSlot`
component, so now Advertising.displaySlots() will not fire for defined
ad slots that are not yet in the DOM.
- The solution was based on a suggestion from @Trav84, as outlined in:
https://github.com/orgs/KijijiCA/discussions/141

---------

Co-authored-by: Stephen Gill <stephen.gill@adevinta.com>
  • Loading branch information
@caffeinated-pixels
caffeinated-pixels and Stephen Gill authored Jan 2, 2025
1 parent bb3073b commit 3e9615b
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 136 deletions.
22 changes: 6 additions & 16 deletions src/Advertising.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ export default class Advertising {
typeof this.config.useAPS === 'undefined'
? typeof window.apstag !== 'undefined'
: this.config.useAPS;
this.apsSlotType = this.config.aps && this.config.aps.simplerGPT ? 'gpt' : 'aps';
this.apsSlotType =
this.config.aps && this.config.aps.simplerGPT ? 'gpt' : 'aps';
this.executePlugins('setup');
const { slots, outOfPageSlots, queue, isPrebidUsed, isAPSUsed } = this;
this.setupCustomEvents();
Expand Down Expand Up @@ -194,6 +195,8 @@ export default class Advertising {
this.onError
);
}

this.displaySlots(id);
}

isConfigReady() {
Expand Down Expand Up @@ -367,20 +370,9 @@ export default class Advertising {
}
}

displaySlots() {
displaySlots(id) {
this.executePlugins('displaySlots');
this.config.slots.forEach(({ id }) => {
window.googletag.display(id);
});
}

displayOutOfPageSlots() {
this.executePlugins('displayOutOfPageSlot');
if (this.config.outOfPageSlots) {
this.config.outOfPageSlots.forEach(({ id }) => {
window.googletag.display(id);
});
}
window.googletag.display(id);
}

refreshInterstitialSlot() {
Expand Down Expand Up @@ -425,8 +417,6 @@ export default class Advertising {
pubads.enableSingleRequest();

window.googletag.enableServices();
this.displaySlots();
this.displayOutOfPageSlots();
this.refreshInterstitialSlot();
}

Expand Down
48 changes: 22 additions & 26 deletions src/Advertising.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,17 +367,10 @@ describe('When I instantiate an advertising main module, with Prebid', () => {
void it('are enabled', () =>
expect(global.googletag.enableServices).toHaveBeenCalledTimes(1)));
//----------------------------------------------------------------------------------------------------
describe('the display method of GPT', () => {
it('is called for each slot', () =>
expect(global.googletag.display).toHaveBeenCalledTimes(2));
it('is called with the DIV ID of the “foo” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_FOO));
it('is called with the DIV ID of the “bar” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_BAR));
});
describe('the slots of the advertising module instance', () =>
void it('are correct', () =>
expect(advertising.slots).toMatchSnapshot()));
void it('are correct', () => {
expect(advertising.slots).toMatchSnapshot();
}));
describe('the GPT size mappings of the advertising module instance', () =>
void it('are correct', () =>
expect(advertising.gptSizeMappings).toMatchSnapshot()));
Expand Down Expand Up @@ -411,6 +404,13 @@ describe('When I instantiate an advertising main module, with Prebid', () => {
expect(
global.googletag.pubads().refresh.mock.calls
).toMatchSnapshot()));

describe('the display method of GPT', () => {
it('is called once', () =>
expect(global.googletag.display).toHaveBeenCalledTimes(1));
it('is called with the DIV ID of the “foo” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_FOO));
});
});
describe('and I activate the “foo” ad with a custom events object to collapse its slot', () => {
let collapse;
Expand Down Expand Up @@ -467,9 +467,12 @@ describe('When I instantiate an advertising main module', () => {
describe('the targeting for asynchronous GPT', () =>
void it('is not set', () =>
expect(global.pbjs.setTargetingForGPTAsync).toHaveBeenCalledTimes(0)));
describe('the ad slot', () =>
describe('the ad slot', () => {
void it('is not refreshed', () =>
expect(global.googletag.pubads().refresh).toHaveBeenCalledTimes(0)));
expect(global.googletag.pubads().refresh).toHaveBeenCalledTimes(0));
void it('is not displayed', () =>
expect(global.googletag.pubads().display).toHaveBeenCalledTimes(0));
});
describe('and I call the setup method', () => {
beforeEach(() => advertising.setup());
describe('a bid', () =>
Expand Down Expand Up @@ -573,12 +576,7 @@ describe('When I instantiate an advertising main module with plugins', () => {
describe("the plugin's hook for GPT teardown", () =>
void it('is not called', () =>
expect(plugins[0].teardownGpt).toHaveBeenCalledTimes(0)));
describe("the plugin's hook for displaying slots", () =>
void it('is called', () =>
expect(plugins[0].displaySlots).toHaveBeenCalled()));
describe("the plugin's hook for displaying outOfPage slots", () =>
void it('is called', () =>
expect(plugins[0].displayOutOfPageSlot).toHaveBeenCalled()));

describe("the plugin's hook for refreshing interstitial slot", () =>
void it('is called', () =>
expect(plugins[0].refreshInterstitialSlot).toHaveBeenCalled()));
Expand Down Expand Up @@ -822,14 +820,6 @@ describe('When I instantiate an advertising main module without Prebid.js or APS
void it('are enabled', () =>
expect(global.googletag.enableServices).toHaveBeenCalledTimes(1)));
//----------------------------------------------------------------------------------------------------
describe('the display method of GPT', () => {
it('is called for each slot', () =>
expect(global.googletag.display).toHaveBeenCalledTimes(2));
it('is called with the DIV ID of the “foo” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_FOO));
it('is called with the DIV ID of the “bar” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_BAR));
});
describe('the slots of the advertising module instance', () =>
void it('are correct', () =>
expect(advertising.slots).toMatchSnapshot()));
Expand All @@ -855,6 +845,12 @@ describe('When I instantiate an advertising main module without Prebid.js or APS
expect(
global.googletag.pubads().refresh.mock.calls
).toMatchSnapshot()));
describe('the display method of GPT', () => {
it('is called for each slot', () =>
expect(global.googletag.display).toHaveBeenCalledTimes(1));
it('is called with the DIV ID of the “foo” ad', () =>
expect(global.googletag.display).toHaveBeenCalledWith(DIV_ID_FOO));
});
});
});
afterEach(() => {
Expand Down
98 changes: 4 additions & 94 deletions src/__snapshots__/Advertising.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,19 +247,12 @@ Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"enableServices": [MockFunction] {
Expand Down Expand Up @@ -1040,26 +1033,7 @@ Object {
},
],
},
"display": [MockFunction] {
"calls": Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"display": [MockFunction],
"enableServices": [MockFunction] {
"calls": Array [
Array [],
Expand Down Expand Up @@ -1688,26 +1662,7 @@ Object {
},
],
},
"display": [MockFunction] {
"calls": Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"display": [MockFunction],
"enableServices": [MockFunction] {
"calls": Array [
Array [],
Expand Down Expand Up @@ -2546,19 +2501,12 @@ Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"enableServices": [MockFunction] {
Expand Down Expand Up @@ -3407,26 +3355,7 @@ Object {
},
],
},
"display": [MockFunction] {
"calls": Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"display": [MockFunction],
"enableServices": [MockFunction] {
"calls": Array [
Array [],
Expand Down Expand Up @@ -4055,26 +3984,7 @@ Object {
},
],
},
"display": [MockFunction] {
"calls": Array [
Array [
"div-gpt-ad-foo",
],
Array [
"div-gpt-ad-bar",
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
Object {
"type": "return",
"value": undefined,
},
],
},
"display": [MockFunction],
"enableServices": [MockFunction] {
"calls": Array [
Array [],
Expand Down

0 comments on commit 3e9615b

Please sign in to comment.