Skip to content

Select active GPT responsive slots #978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChristianPavilonis wants to merge 1 commit into
base: fix/duplicate-gpt-slots
Choose a base branch
from
+243 −23
Open

Select active GPT responsive slots #978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/trusted-server-core/src/integrations/gpt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,7 +1213,7 @@ mod tests {
"bootstrap should scan ID-bearing elements instead of interpolating div_id into CSS"
);
assert!(
combined.contains(".startsWith(slot.div_id)"),
combined.contains("candidate.id.startsWith(divId)"),

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 seedling — Not for this PR: the resolver now exists twice (gpt_bootstrap.js and gpt/index.ts) and the only thing tying them together is this substring assertion plus the shared it.each table. The assertion pins one identifier, so a divergence in the tiering logic itself (the exact thing this PR changes) would pass. Worth a follow-up to generate the bootstrap from the TS source, or at least to assert on the tier names (activeMatches / prefixMatches) as well.

"bootstrap should match metacharacter-containing div_id prefixes with startsWith"
);
assert!(
Expand Down
79 changes: 61 additions & 18 deletions crates/trusted-server-core/src/integrations/gpt_bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,63 @@
return target && target.id ? target.id : null;
}

function isElementVisible(element) {
var style = window.getComputedStyle(element);
return (
style.display !== "none" &&
style.visibility !== "hidden" &&
style.visibility !== "collapse"
);
}

function slotElementHasLayout(element) {
if (!isElementVisible(element)) return false;
var elementRect = element.getBoundingClientRect();
if (elementRect.width > 0 && elementRect.height > 0) return true;

var container = document.getElementById(element.id + "-container");
if (!container || !isElementVisible(container)) return false;
var containerRect = container.getBoundingClientRect();
return containerRect.width > 0 && containerRect.height > 0;
}

function findSlotElementByDivId(divId) {
if (!divId) return null;
var exact = document.getElementById(divId);
if (exact) return exact;

var idElements = document.querySelectorAll("[id]");
var prefixMatches = [];
for (var i = 0; i < idElements.length; i++) {
var candidate = idElements[i];
if (
candidate.id.startsWith(divId) &&
!candidate.id.endsWith("-container")
) {
prefixMatches.push(candidate);
}
}
// A unique prefix match may be a lazy slot that has not been sized yet.
// Geometry is only needed to disambiguate multiple responsive siblings.
if (prefixMatches.length === 1) return prefixMatches[0];

var activeMatches = prefixMatches.filter(slotElementHasLayout);

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — Geometry is required before any sibling can be picked, so the single visible responsive sibling is skipped whenever it reserves no space.

slotElementHasLayout needs width > 0 && height > 0 on the element or on <id>-container. An empty ad div awaiting its creative commonly computes to height 0, and its empty wrapper does too. With several prefix matches, the active sibling then fails the check and activeMatches.length === 0 → this function returns null → adInit does if (!el) return, so nothing is defined and no ad request is made. Before this PR the first prefix match was defined and requested, so this trades a mis-targeted impression for no impression at all.

Reproduced against both implementations with a throwaway probe — three display:none dupes plus one visible sibling, default 0×0 rects:

GPT slot prefix did not resolve to one active element { divId: 'ad-probe-', prefixMatchCount: 4, activeMatchCount: 0 }
→ defineSlot not called, divToSlotId {}

That is exactly the not-size-a/b/c/d shape this PR targets: inactive siblings are hidden with display:none, so visibility alone already disambiguates them and geometry is only needed when two or more siblings are visible.

Fix — add a visibility tier before the geometry tier:

    var visibleMatches = prefixMatches.filter(isElementVisible);
    if (visibleMatches.length === 1) return visibleMatches[0];

    var activeMatches = visibleMatches.filter(slotElementHasLayout);
    if (activeMatches.length === 1) return activeMatches[0];

Filtering the geometry pass over visibleMatches (not prefixMatches) also keeps the hidden-sibling case unchanged, since slotElementHasLayout already starts with isElementVisible.

if (activeMatches.length === 1) return activeMatches[0];

if (
prefixMatches.length > 1 &&
ts.log &&
typeof ts.log.warn === "function"
) {
ts.log.warn("GPT slot prefix did not resolve to one active element", {

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — This warn can never fire: window.tsjs.log is declared optional on TsjsApi (core/types.ts:97) but is never assigned anywhere in the repo, so ts.log && typeof ts.log.warn === "function" is always false. The probe above confirms it — the bundle path logged, the bootstrap path was silent.

That matters more here than for the pre-existing handoff warn on line 180: the bootstrap is the first-load path, and it is the only place that can report why a placement resolved to nothing. As written, an ambiguous prefix produces a blank slot with no signal anywhere.

Either fall back to console.warn, or record a counter the bundle can surface later, e.g.:

      ts.gptSlotResolveFailures = (ts.gptSlotResolveFailures || []).concat({
        divId: divId,
        prefixMatchCount: prefixMatches.length,
        activeMatchCount: activeMatches.length,
      });

divId: divId,
prefixMatchCount: prefixMatches.length,
activeMatchCount: activeMatches.length,
});
}
return null;
}

function runHandoffInternal(callback) {
var wasInternal = ts.gptSlotHandoffInternal;
ts.gptSlotHandoffInternal = true;
Expand Down Expand Up @@ -217,24 +274,10 @@
// slot that was never displayed, so these are display()ed instead.
var slotsToDisplay = [];
slots.forEach(function (slot) {
// Resolve actual div ID: exact match first, then safe prefix scan.
// div_id in config may be a stable prefix (e.g. "ad-header-0-") when
// the suffix is dynamically generated by the framework at render time.
var el = document.getElementById(slot.div_id);
if (!el) {
var idElements = document.querySelectorAll("[id]");
for (var i = 0; i < idElements.length; i++) {
var candidate = idElements[i];
if (
slot.div_id &&
candidate.id.startsWith(slot.div_id) &&
!candidate.id.endsWith("-container")
) {
el = candidate;
break;
}
}
}
// Resolve actual div ID: exact match first, then the one active prefix
// match. Responsive publishers may emit several mutually exclusive
// siblings for one stable prefix, so document order is not sufficient.
var el = findSlotElementByDivId(slot.div_id);
if (!el) return;
var actualDivId = el.id;
var b = bids[slot.id] || {};
Expand Down
40 changes: 36 additions & 4 deletions crates/trusted-server-js/lib/src/integrations/gpt/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,47 @@ interface SlotRenderEndedEvent {
slot: GoogleTagSlot;
}

function isElementVisible(element: HTMLElement): boolean {
const style = window.getComputedStyle(element);
return (
style.display !== 'none' && style.visibility !== 'hidden' && style.visibility !== 'collapse'
);
}

function slotElementHasLayout(element: HTMLElement): boolean {
if (!isElementVisible(element)) return false;
const elementRect = element.getBoundingClientRect();
if (elementRect.width > 0 && elementRect.height > 0) return true;

const container = document.getElementById(`${element.id}-container`);
if (!container || !isElementVisible(container)) return false;
const containerRect = container.getBoundingClientRect();
return containerRect.width > 0 && containerRect.height > 0;
}

function findSlotElementByDivId(divId: string): HTMLElement | null {
if (!divId) return null;

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — This fixes a real wildcard bug, not just an edge case. Previously an empty configured div_id fell through getElementById('') to el.id.startsWith(''), which matches every element with an id, so the slot bound to whatever [id] element happened to come first in the document. Good catch, and good that the table covers it for both implementations.

const exact = document.getElementById(divId);
if (exact) return exact;

return (
Array.from(document.querySelectorAll<HTMLElement>('[id]')).find(
(el) => el.id.startsWith(divId) && !el.id.endsWith('-container')
) ?? null
const prefixMatches = Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter(
(element) => element.id.startsWith(divId) && !element.id.endsWith('-container')
);
// A unique prefix match may be a lazy slot that has not been sized yet.
// Geometry is only needed to disambiguate multiple responsive siblings.
if (prefixMatches.length === 1) return prefixMatches[0] ?? null;

const activeMatches = prefixMatches.filter(slotElementHasLayout);

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — Same geometry-gate issue as gpt_bootstrap.js:121; the runtime needs the matching change so the two stay in parity.

  const visibleMatches = prefixMatches.filter(isElementVisible);
  if (visibleMatches.length === 1) return visibleMatches[0] ?? null;

  const activeMatches = visibleMatches.filter(slotElementHasLayout);
  if (activeMatches.length === 1) return activeMatches[0] ?? null;

Worth extending the it.each table with a hiddenElementIndexes: [0, 1, 3] + activeIndexes: [] case so the intended contract ("one visible sibling wins even with no reserved height") is pinned in both implementations.

if (activeMatches.length === 1) return activeMatches[0] ?? null;

if (prefixMatches.length > 1) {
log.warn('GPT slot prefix did not resolve to one active element', {
divId,
prefixMatchCount: prefixMatches.length,
activeMatchCount: activeMatches.length,
});
}
return null;
}

function candidateSlotRoots(divId: string): HTMLElement[] {
Expand Down
145 changes: 145 additions & 0 deletions crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,43 @@ type TestWindow = Window & {
tsjs?: any;
};

function appendResponsiveSlotElement(
id: string,
containerHasLayout: boolean,
elementHidden = false,
elementHasLayout = false
): HTMLDivElement {
const container = document.createElement('div');
container.id = `${id}-container`;
container.dataset.responsiveSlotTest = 'true';
container.style.display = containerHasLayout ? 'block' : 'none';
container.getBoundingClientRect = () =>
({
width: containerHasLayout ? 320 : 0,
height: containerHasLayout ? 100 : 0,
}) as DOMRect;

const element = document.createElement('div');
element.id = id;
element.style.display = elementHidden ? 'none' : 'block';
element.getBoundingClientRect = () =>
({
width: elementHasLayout ? 300 : 0,
height: elementHasLayout ? 250 : 0,
}) as DOMRect;
container.appendChild(element);
document.body.appendChild(container);
return element;
}

function runGptBootstrap(): void {
const bootstrap = readFileSync(
resolve(process.cwd(), '../../trusted-server-core/src/integrations/gpt_bootstrap.js'),
'utf8'
);
window.eval(bootstrap);
}

describe('installTsAdInit', () => {
beforeEach(() => {
vi.resetModules();
Expand All @@ -79,6 +116,7 @@ describe('installTsAdInit', () => {
document.getElementById('div-atf-sidebar')?.remove();
document.getElementById('ad-header-0-_r_1_')?.remove();
document.getElementById("ad'prefix-real")?.remove();
document.querySelectorAll('[data-responsive-slot-test]').forEach((element) => element.remove());
});

it('reads window.tsjs.bids synchronously and applies bid targeting before refresh', async () => {
Expand Down Expand Up @@ -1257,6 +1295,113 @@ describe('installTsAdInit', () => {
expect(mockPubads.refresh).toHaveBeenCalled();
});

it.each([

@prk-Jr prk-Jr Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — Driving both the bundle and the edge bootstrap from one case list (with the bootstrap read straight off gpt_bootstrap.js) is the right way to test two hand-maintained copies of the same resolver. It is what made the geometry-gate finding above verifiable in both paths from a single fixture.

{ implementation: 'runtime', activeIndexes: [2], publisherOwned: true, selectedIndex: 2 },
{ implementation: 'runtime', activeIndexes: [], selectedIndex: null },
{ implementation: 'runtime', activeIndexes: [], elementLayoutIndexes: [1], selectedIndex: 1 },
{ implementation: 'runtime', activeIndexes: [0, 2], selectedIndex: null },
{
implementation: 'runtime',
activeIndexes: [2, 3],
hiddenElementIndexes: [2],
selectedIndex: 3,
},
{ implementation: 'runtime', activeIndexes: [2], divId: '', selectedIndex: null },
{ implementation: 'bootstrap', activeIndexes: [2], publisherOwned: true, selectedIndex: 2 },
{ implementation: 'bootstrap', activeIndexes: [], selectedIndex: null },
{ implementation: 'bootstrap', activeIndexes: [], elementLayoutIndexes: [1], selectedIndex: 1 },
{ implementation: 'bootstrap', activeIndexes: [0, 2], selectedIndex: null },
{
implementation: 'bootstrap',
activeIndexes: [2, 3],
hiddenElementIndexes: [2],
selectedIndex: 3,
},
{ implementation: 'bootstrap', activeIndexes: [2], divId: '', selectedIndex: null },
] as const)(
'$implementation resolves responsive matches $activeIndexes to $selectedIndex',
async (testCase) => {
const { implementation, activeIndexes, selectedIndex } = testCase;
const hiddenElementIndexes =
'hiddenElementIndexes' in testCase ? testCase.hiddenElementIndexes : [];
const elementLayoutIndexes =
'elementLayoutIndexes' in testCase ? testCase.elementLayoutIndexes : [];
const divId = 'divId' in testCase ? testCase.divId : 'ad-responsive-';
const publisherOwned = 'publisherOwned' in testCase && testCase.publisherOwned;
const elements = ['a', 'b', 'c', 'd'].map((suffix, index) =>
appendResponsiveSlotElement(
`ad-responsive-${suffix}`,
(activeIndexes as readonly number[]).includes(index),
(hiddenElementIndexes as readonly number[]).includes(index),
(elementLayoutIndexes as readonly number[]).includes(index)
)
);
const selectedElement = selectedIndex === null ? undefined : elements[selectedIndex];
const mockSlot = {
addService: vi.fn().mockReturnThis(),
setTargeting: vi.fn().mockReturnThis(),
getSlotElementId: vi.fn().mockReturnValue(selectedElement?.id ?? elements[0]!.id),
getTargeting: vi.fn().mockReturnValue([]),
};
const nativeRefresh = vi.fn();
const mockPubads = {
enableSingleRequest: vi.fn(),
getSlots: vi.fn().mockReturnValue(publisherOwned ? [mockSlot] : []),
addEventListener: vi.fn(),
refresh: nativeRefresh,
};
const defineSlot = vi.fn().mockReturnValue(mockSlot);
const nativeDisplay = vi.fn();
(window as TestWindow).googletag = {
cmd: { push: vi.fn((fn: () => void) => fn()) },
defineSlot,
display: nativeDisplay,
pubads: vi.fn().mockReturnValue(mockPubads),
enableServices: vi.fn(),
};
(window as TestWindow).tsjs = {
adSlots: [
{
id: 'responsive_slot',
gam_unit_path: '/123/responsive',
div_id: divId,
formats: [[300, 250]],
targeting: {},
},
],
bids: {},
};

if (implementation === 'runtime') {
const { installTsAdInit } = await import('../../../src/integrations/gpt/index');
installTsAdInit();
} else {
runGptBootstrap();
}
(window as TestWindow).tsjs!.adInit!();

if (selectedElement) {
if (publisherOwned) {
expect(defineSlot).not.toHaveBeenCalled();
expect(nativeRefresh).toHaveBeenCalledWith([mockSlot]);
} else {
expect(defineSlot).toHaveBeenCalledWith(
'/123/responsive',
[[300, 250]],
selectedElement.id
);
expect(nativeDisplay).toHaveBeenCalledWith(selectedElement.id);
}
expect((window as TestWindow).tsjs!.divToSlotId).toEqual({
[selectedElement.id]: 'responsive_slot',
});
} else {
expect(defineSlot).not.toHaveBeenCalled();
expect((window as TestWindow).tsjs!.divToSlotId).toEqual({});
}
}
);

it('resolves dynamic div prefixes without interpolating div_id into a CSS selector', async () => {
const dynamicDiv = document.createElement('div');
dynamicDiv.id = "ad'prefix-real";
Expand Down
Loading