Fix spacefinder placing ads too close together on tablet & desktop (#…

…1550)

* avoid previous passes for desktop right rail ads

* use mobile rules for tablets

* changesets

* update test

* spacefinder rules should use :scope

* update e2e tests to check tablet positions

* update mobile fn name

* update liveblog tests

.changeset/bright-beans-train.md

@@ -0,0 +1,5 @@
+---
+'@guardian/commercial': minor
+---
+
+Fix issue with double ads on tablets

.changeset/mean-items-attend.md

@@ -0,0 +1,5 @@
+---
+'@guardian/commercial': minor
+---
+
+Fix issue with first right rail ad very close to inline1

playwright/fixtures/pages/Page.ts

@@ -1,8 +1,14 @@
 export type GuPage = {
 	path: string;
 	name?: string;
-	expectedMinInlineSlotsOnMobile?: number;
-	expectedMinInlineSlotsOnDesktop?: number;
-	expectedSlotPositionsOnMobile?: number[];
-	expectedSlotPositionsOnDesktop?: number[];
+	expectedMinInlineSlots?: {
+		mobile: number;
+		tablet: number;
+		desktop: number;
+	};
+	expectedSlotPositions?: {
+		mobile: number[];
+		tablet: number[];
+		desktop: number[];
+	};
 };

playwright/fixtures/pages/articles.ts

@@ -28,8 +28,11 @@ const articles: GuPage[] = [
 			path: '/environment/2020/oct/13/maverick-rewilders-endangered-species-extinction-conservation-uk-wildlife',
 		}),
 		name: 'inlineSlots',
-		expectedMinInlineSlotsOnDesktop: 12,
-		expectedMinInlineSlotsOnMobile: 16,
+		expectedMinInlineSlots: {
+			desktop: 11,
+			tablet: 18,
+			mobile: 16,
+		},
 	},
 	{
 		path: getTestUrl({
@@ -44,17 +47,23 @@ const articles: GuPage[] = [
 			path: '/football/2024/feb/09/premier-league-10-things-to-look-out-for-this-weekend',
 		}),
 		name: 'inlineSlots',
-		expectedSlotPositionsOnMobile: [7, 14, 20, 26, 32, 39, 46, 58],
-		expectedSlotPositionsOnDesktop: [14, 30, 43, 55],
+		expectedSlotPositions: {
+			mobile: [7, 14, 20, 26, 32, 39, 46, 58],
+			tablet: [7, 14, 25, 37, 44, 56],
+			desktop: [14, 30, 43, 55],
+		},
 	},
 	{
 		path: getTestUrl({
 			stage,
 			path: '/culture/2024/feb/08/say-it-with-a-kiss-the-20-greatest-smooches-on-film-ranked',
 		}),
 		name: 'inlineSlots',
-		expectedSlotPositionsOnMobile: [4, 11, 18, 25, 32, 39, 46, 53, 57, 64],
-		expectedSlotPositionsOnDesktop: [6, 13, 20, 30, 37, 44, 51, 58, 65],
+		expectedSlotPositions: {
+			mobile: [4, 11, 18, 25, 32, 39, 46, 53, 57, 64],
+			tablet: [4, 11, 18, 25, 32, 39, 46, 53, 60, 67],
+			desktop: [6, 13, 20, 30, 37, 44, 51, 58, 65],
+		},
 	},
 ];
 

playwright/fixtures/pages/blogs.ts

@@ -19,17 +19,23 @@ const blogs: GuPage[] = [
 			type: 'liveblog',
 		}),
 		name: 'under-ad-limit',
-		expectedMinInlineSlotsOnDesktop: 4,
-		expectedMinInlineSlotsOnMobile: 6,
+		expectedMinInlineSlots: {
+			desktop: 4,
+			tablet: 6,
+			mobile: 6,
+		},
 	},
 	{
 		path: getTestUrl({
 			stage,
 			path: '/business/live/2023/aug/07/halifax-house-prices-gradual-drop-annual-fall-in-july-interest-rates-mortgages-business-live',
 			type: 'liveblog',
 		}),
-		expectedMinInlineSlotsOnDesktop: 5,
-		expectedMinInlineSlotsOnMobile: 7,
+		expectedMinInlineSlots: {
+			desktop: 5,
+			tablet: 7,
+			mobile: 7,
+		},
 	},
 ];
 

playwright/tests/article-inline-slots.spec.ts

@@ -8,28 +8,19 @@ const pages = articles.filter(({ name }) => name === 'inlineSlots');
 
 test.describe('Slots and iframes load on article pages', () => {
 	pages.forEach(
-		(
-			{
-				path,
-				expectedMinInlineSlotsOnDesktop,
-				expectedMinInlineSlotsOnMobile,
-				expectedSlotPositionsOnMobile,
-				expectedSlotPositionsOnDesktop,
-			},
-			index,
-		) => {
+		({ path, expectedMinInlineSlots, expectedSlotPositions }, index) => {
 			breakpoints.forEach(({ breakpoint, width, height }) => {
 				const expectedMinSlotsOnPage =
-					(breakpoint === 'mobile'
-						? expectedMinInlineSlotsOnMobile
-						: expectedMinInlineSlotsOnDesktop) ?? 999;
+					expectedMinInlineSlots?.[
+						breakpoint as keyof typeof expectedMinInlineSlots
+					];
 
-				const expectedSlotPositions =
-					breakpoint === 'mobile'
-						? expectedSlotPositionsOnMobile
-						: expectedSlotPositionsOnDesktop;
+				const expectedSlotPositionsForBreakpoint =
+					expectedSlotPositions?.[
+						breakpoint as keyof typeof expectedSlotPositions
+					];
 
-				if (!expectedSlotPositions) {
+				if (expectedMinSlotsOnPage) {
 					test(`Test article ${index} has at least ${expectedMinSlotsOnPage} inline total slots at breakpoint ${breakpoint}`, async ({
 						page,
 					}) => {
@@ -59,8 +50,8 @@ test.describe('Slots and iframes load on article pages', () => {
 							expectedMinSlotsOnPage,
 						);
 					});
-				} else {
-					test(`Test article ${index} has slots at positions ${expectedSlotPositions.join(
+				} else if (expectedSlotPositionsForBreakpoint) {
+					test(`Test article ${index} has slots at positions ${expectedSlotPositionsForBreakpoint.join(
 						',',
 					)} at breakpoint ${breakpoint}`, async ({ page }) => {
 						await page.setViewportSize({
@@ -89,7 +80,9 @@ test.describe('Slots and iframes load on article pages', () => {
 									.filter((index) => index !== undefined),
 							);
 
-						expect(slotPositions).toEqual(expectedSlotPositions);
+						expect(slotPositions).toEqual(
+							expectedSlotPositionsForBreakpoint,
+						);
 					});
 				}
 			});

playwright/tests/liveblog-ad-limit.spec.ts

@@ -52,7 +52,7 @@ const addAndAwaitNewBlocks = async (page: Page, blockContent: string) => {
 };
 
 test.describe('A minimum amount of ad slots load', () => {
-	pages.forEach(({ path, expectedMinInlineSlotsOnDesktop }) => {
+	pages.forEach(({ path, expectedMinInlineSlots }) => {
 		/**
 		 * First ensure that the we receive the expected initial amount of ad slots.
 		 *
@@ -77,7 +77,12 @@ test.describe('A minimum amount of ad slots load', () => {
 				page,
 				false,
 			);
-			expect(initialSlotCount).toEqual(expectedMinInlineSlotsOnDesktop);
+
+			if (expectedMinInlineSlots) {
+				expect(initialSlotCount).toEqual(
+					expectedMinInlineSlots.desktop,
+				);
+			}
 
 			await addAndAwaitNewBlocks(page, 'First batch of inserted blocks');
 			const slotCountAfterFirstInsert = await countLiveblogInlineSlots(

playwright/tests/liveblog-inline-slots.spec.ts

@@ -12,19 +12,15 @@ const blogPages = blogs.filter(
 );
 
 test.describe.serial('A minimum number of ad slots load', () => {
-	blogPages.forEach(
-		({
-			path,
-			expectedMinInlineSlotsOnDesktop,
-			expectedMinInlineSlotsOnMobile,
-		}) => {
-			breakpoints.forEach(({ breakpoint, width, height }) => {
-				const isMobile = breakpoint === 'mobile';
-				const expectedMinSlotsOnPage =
-					(isMobile
-						? expectedMinInlineSlotsOnMobile
-						: expectedMinInlineSlotsOnDesktop) ?? 999;
-
+	blogPages.forEach(({ path, expectedMinInlineSlots }) => {
+		breakpoints.forEach(({ breakpoint, width, height }) => {
+			const isMobile = breakpoint === 'mobile';
+			const expectedMinSlotsOnPage =
+				expectedMinInlineSlots?.[
+					breakpoint as keyof typeof expectedMinInlineSlots
+				];
+
+			if (expectedMinSlotsOnPage) {
 				test(`There are at least ${expectedMinSlotsOnPage} inline total slots at breakpoint ${breakpoint}`, async ({
 					page,
 				}) => {
@@ -45,9 +41,9 @@ test.describe.serial('A minimum number of ad slots load', () => {
 						expectedMinSlotsOnPage,
 					);
 				});
-			});
-		},
-	);
+			}
+		});
+	});
 });
 
 test.describe.serial('Correct set of slots are displayed', () => {

src/insert/spacefinder/article.ts

@@ -191,11 +191,20 @@ const addDesktopRightRailAds = (
 	);
 };
 
-const addMobileInlineAds = (fillSlot: FillAdSlot): Promise<boolean> => {
+const addMobileAndTabletInlineAds = (
+	fillSlot: FillAdSlot,
+	currentBreakpoint: ReturnType<typeof getCurrentBreakpoint>,
+): Promise<boolean> => {
 	const insertAds: SpacefinderWriter = async (paras) => {
 		const slots = paras.map(async (para, i) => {
-			const name = i === 0 ? 'top-above-nav' : `inline${i}`;
-			const type = i === 0 ? 'top-above-nav' : 'inline';
+			const name =
+				currentBreakpoint === 'mobile' && i === 0
+					? 'top-above-nav'
+					: `inline${i}`;
+			const type =
+				currentBreakpoint === 'mobile' && i === 0
+					? 'top-above-nav'
+					: 'inline';
 			const slot = await insertSlotAtPara(para, name, type, 'inline');
 			return fillSlot(
 				name,
@@ -211,7 +220,7 @@ const addMobileInlineAds = (fillSlot: FillAdSlot): Promise<boolean> => {
 		await Promise.all(slots);
 	};
 
-	return spaceFiller.fillSpace(rules.mobileInlines, insertAds, {
+	return spaceFiller.fillSpace(rules.mobileAndTabletInlines, insertAds, {
 		waitForImages: true,
 		waitForInteractives: true,
 		pass: 'mobile-inlines',
@@ -227,9 +236,9 @@ const addInlineAds = (
 	fillSlot: FillAdSlot,
 	isConsentless: boolean,
 ): Promise<boolean> => {
-	const isMobile = getCurrentBreakpoint() === 'mobile';
-	if (isMobile) {
-		return addMobileInlineAds(fillSlot);
+	const currentBreakpoint = getCurrentBreakpoint();
+	if (['mobile', 'tablet'].includes(currentBreakpoint)) {
+		return addMobileAndTabletInlineAds(fillSlot, currentBreakpoint);
 	}
 
 	if (isPaidContent) {

src/insert/spacefinder/rules.ts

@@ -33,11 +33,12 @@ const minDistanceBetweenInlineAds = isInHighValueSection ? 500 : 750;
 const candidateSelector = ':scope > p, [data-spacefinder-role="nested"] > p';
 
 const leftColumnOpponentSelector = ['richLink', 'thumbnail']
-	.map((role) => `[data-spacefinder-role="${role}"]`)
+	.map((role) => `:scope > [data-spacefinder-role="${role}"]`)
 	.join(',');
-const rightColumnOpponentSelector = '[data-spacefinder-role="immersive"]';
+const rightColumnOpponentSelector =
+	':scope > [data-spacefinder-role="immersive"]';
 const inlineOpponentSelector = ['inline', 'supporting', 'showcase']
-	.map((role) => `[data-spacefinder-role="${role}"]`)
+	.map((role) => `:scope > [data-spacefinder-role="${role}"]`)
 	.join(',');
 
 const headingSelector = `:scope > h2, [data-spacefinder-role="nested"] > h2, :scope > h3, [data-spacefinder-role="nested"] > h3`;
@@ -104,6 +105,10 @@ const desktopRightRail = (isConsentless: boolean): SpacefinderRules => {
 		minDistanceFromTop: desktopRightRailMinAbove(isConsentless),
 		minDistanceFromBottom: 300,
 		opponentSelectorRules: {
+			[adSlotContainerSelector]: {
+				marginBottom: 500,
+				marginTop: 500,
+			},
 			[rightColumnOpponentSelector]: {
 				marginBottom: 0,
 				marginTop: 600,
@@ -157,7 +162,7 @@ const mobileOpponentSelectorRules: OpponentSelectorRules = {
 	},
 };
 
-const mobileInlines: SpacefinderRules = {
+const mobileAndTabletInlines: SpacefinderRules = {
 	bodySelector,
 	candidateSelector: mobileCandidateSelector,
 	minDistanceFromTop: mobileMinDistanceFromArticleTop,
@@ -180,5 +185,5 @@ const mobileInlines: SpacefinderRules = {
 export const rules = {
 	desktopInline1,
 	desktopRightRail,
-	mobileInlines,
+	mobileAndTabletInlines,
 };

src/insert/spacefinder/spacefinder-debug-tools.ts

@@ -108,6 +108,8 @@ const addHoverListener = (
 					opponent.element,
 					`${opponent.actual}px/${opponent.required}px`,
 				);
+			} else {
+				addOverlay(opponent.element, 'Blocking element');
 			}
 		});