Automattic / Automattic/newspack-rolling-coverage

Dynamically inserted ads are excluded from Newspack Ads' slot lifecycle

Open
#21 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
1
Forks
1
Avg merge
2d 18h
Merged PRs (30d)
3

Description

## Summary

Ads inserted into the Rolling Coverage feed after page load — by live polling and by Load More — define their GPT slots in `displayAdSlots()` (`src/blocks/rolling-coverage/view.ts`) rather than through Newspack Ads' footer bootstrap. Those slots are never registered with Newspack Ads' own `slotRenderEnded` handling, so several pieces of per-slot behaviour that every server-rendered ad gets are silently skipped for them.

Deferred from [#8](https://github.com/Automattic/newspack-rolling-coverage/pull/8) — the rest of that PR's ad integration is unaffected.

## Symptoms

With the default Newspack Ads configuration (`fixed_height` is `active: true`, `use_max_height: true`, `max_height: 100` — see `newspack-ads/includes/class-fixed-height.php`):

- **An unfilled dynamic ad leaves a permanent blank gap in the feed.** `measureAdSlot()` writes a pixel height onto the slot's wrapper to reserve space, but the handler that would collapse an unsold slot never runs — so the reserved space stays, roughly 100px, between entries at every ad position.
- **Fluid creatives can render at zero width.** GPT sets fluid iframes to `width: 0` with `min-width: 100%`; Newspack Ads corrects this in the same handler.
- **Fixed-height slots are not locked to their rendered size**, so a larger creative can render on a later refresh.
- **`initial_display` is never applied**, so any placement relying on it stays hidden.

Server-rendered ads on the same page behave correctly, which makes the difference easy to miss in testing.

## Root cause

Newspack Ads' GPT bootstrap tags each container with the ad-unit payload:

```js
// newspack-ads/includes/providers/gam/class-gam-scripts.php:208
ad_unit.in_viewport = inOrPastViewport( container );
container.ad_unit = ad_unit;
```

Every `slotRenderEnded` listener it registers reads that property and returns early when it's absent:

```js
// newspack-ads/includes/providers/gam/class-gam-scripts.php:375 (and again at :434)
var ad_unit = container.ad_unit;
if ( ! ad_unit ) {
return;
}
```

Containers created by `displayAdSlots()` never get `container.ad_unit`, so all of the following are skipped for them:

| Behaviour | Location in `class-gam-scripts.php` |
| --- | --- |
| Fixed-height late lock-in | `:388-391` |
| `initial_display` | `:397-399` |
| Empty-slot collapse (`display: none`) | `:400-404` |
| Fluid iframe `width: 100%` fix | `:412-423` |

`newspack-ads/src/frontend/side-rail-placements.js:131` registers a third listener that reads the same property.

## Why this can't just be fixed locally today

Newspack Ads exposes no public JS entry point for defining a slot after its footer script has run — `src/frontend/index.js` is only:

```js
import './side-rail-placements';
import './style.scss';
```

`container.ad_unit` is therefore an internal contract, not a published one.

## Options

**A — Assign `container.ad_unit` before `display()`** (local, small)

Set an object shaped like the bootstrap's `prepared_unit_data` entry — at minimum `fixed_height`, `in_viewport`, `sticky`, `initial_display`, `size_map` — so the existing handlers pick these slots up. Fixes it entirely in this repo, but depends on an undocumented property shape that can change without notice.

**B — Add a public API to `newspack-ads`** (durable, cross-repo)

Export a small `defineSlots( slots )` from `newspack-ads/src/frontend/` that defines slots and wires them into the same `slotRenderEnded` handlers, and call it from here. Removes the duplicated bootstrap in `view.ts` rather than patching around it, and would be reusable by anything else that inserts ads after page load.

A reasonable path is A now with a comment recording the coupling and the `newspack-ads` version verified against, and B tracked separately — but B is the fix that stops this drifting on the next Newspack Ads release.

## Acceptance criteria

On a GAM-configured site with a Rolling Coverage block and ads enabled:

- [ ] An unfilled ad slot inserted by polling or Load More collapses instead of leaving reserved blank space.
- [ ] A fluid creative in a dynamically inserted slot renders at full width.
- [ ] A fixed-height dynamic slot does not grow on refresh.
- [ ] Server-rendered ads are unchanged.

Needs a real GAM setup (network code plus an ad unit assigned to the **Rolling Coverage: Entry** placement) — the empty-slot path is easiest to exercise with an ad unit that has no matching line item, so slots reliably come back empty.

## Notes

Related but tracked separately in the [#8](https://github.com/Automattic/newspack-rolling-coverage/pull/8) review: the same dynamically defined slots also need `googletag.pubads().refresh()` when initial load is disabled, and the `fixed-height` class is applied to server-rendered ads without its CSS being emitted.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/blocks/rolling-coverage/view.ts at displayAdSlots(), then compare its dynamically created containers with the Newspack Ads bootstrap in newspack-ads/includes/providers/gam/class-gam-scripts.php and src/frontend/side-rail-placements.js. Verify the relevant slot lifecycle handlers and determine whether the local or cross-repository approach is appropriate. On a GAM-configured site, confirm that polling and Load More slots collapse when empty, render fluid creatives at full width, and preserve fixed-height behavior without changing server-rendered ads.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript, wordpress
Domain
frontend, web-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.