From 92ea1e5cb644c0b93a6b8f7fe4ea089e382ecc5c Mon Sep 17 00:00:00 2001 From: Rick Viscomi Date: Wed, 24 Jan 2024 12:08:44 -0500 Subject: [PATCH] Pick the first non-null target for INP attribution (#421) * Update onINP.ts * Update src/attribution/onINP.ts Co-authored-by: Barry Pollard * Apply suggestions from code review Co-authored-by: Barry Pollard * prettier * Apply suggestions from code review Co-authored-by: Philip Walton * prettier * Add test for empty target logic --------- Co-authored-by: Barry Pollard Co-authored-by: Philip Walton Co-authored-by: Philip Walton --- src/attribution/onINP.ts | 11 ++++++++- test/e2e/onINP-test.js | 48 ++++++++++++++++++++++++++++++++-------- test/views/inp.njk | 45 ++++++++++++++++++++++++++++++++----- 3 files changed, 88 insertions(+), 16 deletions(-) diff --git a/src/attribution/onINP.ts b/src/attribution/onINP.ts index d787fc6c..94c877ca 100644 --- a/src/attribution/onINP.ts +++ b/src/attribution/onINP.ts @@ -37,8 +37,17 @@ const attributeINP = (metric: INPMetric): void => { ); })[0]; + // Currently Chrome can return a null target for certain event types + // (especially pointer events). As the event target should be the same + // for all events in the same interaction, we pick the first non-null one. + // TODO: remove when 1367329 is resolved + // https://bugs.chromium.org/p/chromium/issues/detail?id=1367329 + const firstEntryWithTarget = metric.entries.find((entry) => entry.target); + (metric as INPMetricWithAttribution).attribution = { - eventTarget: getSelector(longestEntry.target), + eventTarget: getSelector( + firstEntryWithTarget && firstEntryWithTarget.target, + ), eventType: longestEntry.name, eventTime: longestEntry.startTime, eventEntry: longestEntry, diff --git a/test/e2e/onINP-test.js b/test/e2e/onINP-test.js index cdb4dbcb..920923ce 100644 --- a/test/e2e/onINP-test.js +++ b/test/e2e/onINP-test.js @@ -282,6 +282,9 @@ describe('onINP()', async function () { const h1 = await $('h1'); await h1.click(); + // Ensure the interaction completes. + await nextFrame(); + await stubForwardBack(); await beaconCountIs(1); @@ -452,9 +455,10 @@ describe('onINP()', async function () { assert.equal(eventEntry1.processingStart, clickEntry.processingStart); await clearBeacons(); - await setBlockingTime('pointerup', 200); await stubVisibilityChange('visible'); + await setBlockingTime('pointerup', 200); + const reset = await $('#reset'); await reset.click(); @@ -525,6 +529,37 @@ describe('onINP()', async function () { const [inp2] = await getBeacons(); assert.equal(inp2.attribution.loadState, 'loading'); }); + + // TODO: remove this test once the following bug is fixed: + // https://bugs.chromium.org/p/chromium/issues/detail?id=1367329 + it('reports the event target from any entry where target is defined', async function () { + if (!browserSupportsINP) this.skip(); + + await browser.url('/test/inp?attribution=1&mousedown=100&click=50'); + + const h1 = await $('h1'); + + // Simulate a long click on the h1 to ensure the `click` entry + // has a startTime after the `pointerdown` entry. + await browser + .action('pointer') + .move({x: 0, y: 0, origin: h1}) + .down({button: 0}) // left button + .pause(100) + .up({button: 0}) + .perform(); + + await stubVisibilityChange('hidden'); + await beaconCountIs(1); + + const [inp1] = await getBeacons(); + + assert.equal(inp1.attribution.eventType, 'pointerdown'); + // The event target should match the h1, even if the `pointerdown` + // entry doesn't contain a target. + // See: https://bugs.chromium.org/p/chromium/issues/detail?id=1367329 + assert.equal(inp1.attribution.eventTarget, 'html>body>main>h1'); + }); }); }); @@ -536,12 +571,7 @@ const interactionIDsMatch = (entries) => { return entries.every((e) => e.interactionId === entries[0].interactionId); }; -const setBlockingTime = (event, value) => { - return browser.execute( - (event, value) => { - document.getElementById(`${event}-blocking-time`).value = value; - }, - event, - value, - ); +const setBlockingTime = async (event, value) => { + const input = await $(`#${event}-blocking-time`); + await input.setValue(value); }; diff --git a/test/views/inp.njk b/test/views/inp.njk index eabbd652..120d9f4a 100644 --- a/test/views/inp.njk +++ b/test/views/inp.njk @@ -17,6 +17,14 @@ {% block content %}

INP Test

+

+ +

+

+ +

@@ -63,15 +71,40 @@ } } - addEventListener('pointerdown', block, true); - addEventListener('pointerup', block, true); - addEventListener('keydown', block, true); - addEventListener('keyup', block, true); - addEventListener('click', block, true); + function onInput(event) { + const input = event.target; + const eventName = input.id.slice(0, input.id.indexOf('-')); + if (input.value > 0) { + addEventListener(eventName, block, true); + } else { + removeEventListener(eventName, block, true); + } + } + + function addBlockingListeners() { + const eventNames = [ + 'mousedown', + 'mouseup', + 'pointerdown', + 'pointerup', + 'keydown', + 'keyup', + 'click', + ]; + for (const eventName of eventNames) { + const input = document.getElementById(`${eventName}-blocking-time`); + input.addEventListener('input', onInput, true); + if (input.value > 0) { + addEventListener(eventName, block, true); + } + } + } document.getElementById('reset').addEventListener('click', () => { [...document.querySelectorAll('label>input')].forEach((n) => n.value = 0); }); + + addBlockingListeners();

Navigate away

@@ -80,7 +113,7 @@