Automattic / Automattic/newspack-rolling-coverage
Dynamically inserted ads are excluded from Newspack Ads' slot lifecycle
- 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