From 6758606bedaa2fc893ce5207d520ef4e154835e5 Mon Sep 17 00:00:00 2001 From: Jake Lee Kennedy Date: Tue, 14 May 2024 10:06:44 +0100 Subject: [PATCH] Spacefinder handle opponents/candidates overlapping (#1377) --- .changeset/itchy-cows-relate.md | 5 ++ playwright/tests/front-inline-slots.spec.ts | 3 +- src/insert/spacefinder/article.ts | 2 +- .../spacefinder/spacefinder-debug-tools.ts | 21 ++++-- src/insert/spacefinder/spacefinder.ts | 75 ++++++++++++------- 5 files changed, 72 insertions(+), 34 deletions(-) create mode 100644 .changeset/itchy-cows-relate.md diff --git a/.changeset/itchy-cows-relate.md b/.changeset/itchy-cows-relate.md new file mode 100644 index 000000000..fe970d99a --- /dev/null +++ b/.changeset/itchy-cows-relate.md @@ -0,0 +1,5 @@ +--- +'@guardian/commercial': minor +--- + +Spacefinder - Handle opponents/candidates overlapping diff --git a/playwright/tests/front-inline-slots.spec.ts b/playwright/tests/front-inline-slots.spec.ts index 593836247..c61f4d91d 100644 --- a/playwright/tests/front-inline-slots.spec.ts +++ b/playwright/tests/front-inline-slots.spec.ts @@ -15,7 +15,8 @@ test.describe('Slots and iframes load on fronts pages', () => { }); }); -test.describe('Slots and iframes load on tag pages', () => { +// currently no banners on tag fronts +test.skip('Slots and iframes load on tag pages', () => { tagPages.forEach(({ path }) => { test(`fronts-banner ads are loaded on ${path}`, async ({ page }) => { await loadPage(page, path); diff --git a/src/insert/spacefinder/article.ts b/src/insert/spacefinder/article.ts index c9c357e07..57f59c1bc 100644 --- a/src/insert/spacefinder/article.ts +++ b/src/insert/spacefinder/article.ts @@ -171,7 +171,7 @@ const addDesktopInline1 = (fillSlot: FillAdSlot): Promise => { minBelowSlot: 600, }, 'figure.element--supporting': { - minAboveSlot: 500, + minAboveSlot: 100, minBelowSlot: 0, }, }, diff --git a/src/insert/spacefinder/spacefinder-debug-tools.ts b/src/insert/spacefinder/spacefinder-debug-tools.ts index a9d92332d..3b1f29b62 100644 --- a/src/insert/spacefinder/spacefinder-debug-tools.ts +++ b/src/insert/spacefinder/spacefinder-debug-tools.ts @@ -58,6 +58,10 @@ const exclusionTypes = { colour: colours.blue, reason: 'Too close to other element', }, + overlaps: { + colour: colours.blue, + reason: 'Overlaps other element', + }, } as const; const isExclusionType = (type: string): type is keyof typeof exclusionTypes => @@ -72,7 +76,9 @@ const addOverlay = (element: HTMLElement, text: string) => { const addHoverListener = ( candidate: HTMLElement, - tooClose: Exclude['tooClose'], + tooClose: Exclude[ + | 'tooClose' + | 'overlaps'], pass: SpacefinderPass, ) => { tooClose.forEach((opponent) => { @@ -100,10 +106,12 @@ const addHoverListener = ( } opponent.element.classList.add('blocking-element'); - addOverlay( - opponent.element, - `${opponent.actual}px/${opponent.required}px`, - ); + if (opponent.actual && opponent.required) { + addOverlay( + opponent.element, + `${opponent.actual}px/${opponent.required}px`, + ); + } }); candidate.addEventListener('mouseleave', () => { @@ -138,6 +146,9 @@ const annotateExclusions = ( element.setAttribute(`data-sfdebug-${pass}`, 'isStartAt'); } else if (type) { element.setAttribute(`data-sfdebug-${pass}`, key); + } else if (meta && meta.overlaps.length > 0) { + element.setAttribute(`data-sfdebug-${pass}`, 'overlaps'); + addHoverListener(element, meta.overlaps, pass); } else if (meta && meta.tooClose.length > 0) { element.setAttribute(`data-sfdebug-${pass}`, 'tooClose'); addHoverListener(element, meta.tooClose, pass); diff --git a/src/insert/spacefinder/spacefinder.ts b/src/insert/spacefinder/spacefinder.ts index 29ce42ae6..87f1d9993 100644 --- a/src/insert/spacefinder/spacefinder.ts +++ b/src/insert/spacefinder/spacefinder.ts @@ -17,16 +17,19 @@ type RuleSpacing = { bypassMinBelow?: string; }; +type SpacefinderMetaItem = { + required?: number; + actual?: number; + element: HTMLElement; +}; + type SpacefinderItem = { top: number; bottom: number; element: HTMLElement; - meta?: { - tooClose: Array<{ - required: number; - actual: number; - element: HTMLElement; - }>; + meta: { + tooClose: SpacefinderMetaItem[]; + overlaps: SpacefinderMetaItem[]; }; }; @@ -313,29 +316,45 @@ const testCandidate = ( return true; } - const isOpponentBelow = opponent.bottom > candidate.bottom; - - const pass = isTopOfCandidateFarEnoughFromOpponent( - candidate, - opponent, - rule, - isOpponentBelow, - ); + const isOpponentBelow = + opponent.bottom > candidate.bottom && opponent.top > candidate.bottom; + const isOpponentAbove = + opponent.top < candidate.top && opponent.bottom < candidate.top; + + // this can happen when the an opponent like an image or interactive is floated right + const opponentOverlaps = + (isOpponentAbove && isOpponentBelow) || + (!isOpponentAbove && !isOpponentBelow); + + const pass = + !opponentOverlaps && + isTopOfCandidateFarEnoughFromOpponent( + candidate, + opponent, + rule, + isOpponentBelow, + ); if (!pass) { - // if the test fails, add debug information to the candidate metadata - const required = isOpponentBelow - ? rule.minBelowSlot - : rule.minAboveSlot; - const actual = isOpponentBelow - ? opponent.top - candidate.top - : candidate.top - opponent.bottom; - - candidate.meta?.tooClose.push({ - required, - actual, - element: opponent.element, - }); + if (opponentOverlaps) { + candidate.meta.overlaps.push({ + element: opponent.element, + }); + } else { + // if the test fails, add debug information to the candidate metadata + const required = isOpponentBelow + ? rule.minBelowSlot + : rule.minAboveSlot; + const actual = isOpponentBelow + ? opponent.top - candidate.top + : candidate.top - opponent.bottom; + + candidate.meta.tooClose.push({ + required, + actual, + element: opponent.element, + }); + } } return pass; @@ -502,6 +521,7 @@ const getDimensions = (element: HTMLElement): Readonly => element, meta: { tooClose: [], + overlaps: [], }, }); @@ -609,4 +629,5 @@ export type { SpacefinderItem, SpacefinderExclusions, SpacefinderPass, + SpacefinderMetaItem, };