diff --git a/.changeset/bright-beans-train.md b/.changeset/bright-beans-train.md new file mode 100644 index 000000000..c3574b96c --- /dev/null +++ b/.changeset/bright-beans-train.md @@ -0,0 +1,5 @@ +--- +'@guardian/commercial': minor +--- + +Fix issue with double ads on tablets diff --git a/.changeset/mean-items-attend.md b/.changeset/mean-items-attend.md new file mode 100644 index 000000000..003a85a53 --- /dev/null +++ b/.changeset/mean-items-attend.md @@ -0,0 +1,5 @@ +--- +'@guardian/commercial': minor +--- + +Fix issue with first right rail ad very close to inline1 diff --git a/playwright/fixtures/pages/Page.ts b/playwright/fixtures/pages/Page.ts index 5bdd1ddcb..4fa8b957f 100644 --- a/playwright/fixtures/pages/Page.ts +++ b/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[]; + }; }; diff --git a/playwright/fixtures/pages/articles.ts b/playwright/fixtures/pages/articles.ts index f72618384..16de45cf7 100644 --- a/playwright/fixtures/pages/articles.ts +++ b/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,8 +47,11 @@ 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({ @@ -53,8 +59,11 @@ const articles: GuPage[] = [ 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], + }, }, ]; diff --git a/playwright/fixtures/pages/blogs.ts b/playwright/fixtures/pages/blogs.ts index 2e9c99b6c..ce2134f87 100644 --- a/playwright/fixtures/pages/blogs.ts +++ b/playwright/fixtures/pages/blogs.ts @@ -19,8 +19,11 @@ const blogs: GuPage[] = [ type: 'liveblog', }), name: 'under-ad-limit', - expectedMinInlineSlotsOnDesktop: 4, - expectedMinInlineSlotsOnMobile: 6, + expectedMinInlineSlots: { + desktop: 4, + tablet: 6, + mobile: 6, + }, }, { path: getTestUrl({ @@ -28,8 +31,11 @@ const blogs: GuPage[] = [ 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, + }, }, ]; diff --git a/playwright/tests/article-inline-slots.spec.ts b/playwright/tests/article-inline-slots.spec.ts index 5a53f9234..a9464f6b1 100644 --- a/playwright/tests/article-inline-slots.spec.ts +++ b/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, + ); }); } }); diff --git a/playwright/tests/liveblog-ad-limit.spec.ts b/playwright/tests/liveblog-ad-limit.spec.ts index aa6fb82e0..0a6662203 100644 --- a/playwright/tests/liveblog-ad-limit.spec.ts +++ b/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( diff --git a/playwright/tests/liveblog-inline-slots.spec.ts b/playwright/tests/liveblog-inline-slots.spec.ts index 0d31b5ee6..f28d45f65 100644 --- a/playwright/tests/liveblog-inline-slots.spec.ts +++ b/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', () => { diff --git a/src/insert/spacefinder/article.ts b/src/insert/spacefinder/article.ts index a975487c0..e85c57547 100644 --- a/src/insert/spacefinder/article.ts +++ b/src/insert/spacefinder/article.ts @@ -191,11 +191,20 @@ const addDesktopRightRailAds = ( ); }; -const addMobileInlineAds = (fillSlot: FillAdSlot): Promise => { +const addMobileAndTabletInlineAds = ( + fillSlot: FillAdSlot, + currentBreakpoint: ReturnType, +): Promise => { 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 => { 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 => { - const isMobile = getCurrentBreakpoint() === 'mobile'; - if (isMobile) { - return addMobileInlineAds(fillSlot); + const currentBreakpoint = getCurrentBreakpoint(); + if (['mobile', 'tablet'].includes(currentBreakpoint)) { + return addMobileAndTabletInlineAds(fillSlot, currentBreakpoint); } if (isPaidContent) { diff --git a/src/insert/spacefinder/rules.ts b/src/insert/spacefinder/rules.ts index b2452803f..f49160a64 100644 --- a/src/insert/spacefinder/rules.ts +++ b/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, }; diff --git a/src/insert/spacefinder/spacefinder-debug-tools.ts b/src/insert/spacefinder/spacefinder-debug-tools.ts index 0c32ad7e0..70cc1399c 100644 --- a/src/insert/spacefinder/spacefinder-debug-tools.ts +++ b/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'); } });