From 6814844acdfc60bbf31d6acad11630b630363f07 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 16 Jun 2023 13:04:42 +0200 Subject: [PATCH 1/3] feat(replay): Consider `window.open` for slow clicks When a click triggers `window.open()`, do not consider it a slow click. --- .../suites/replay/slowClick/template.html | 4 ++ .../replay/slowClick/windowOpen/test.ts | 61 +++++++++++++++++++ .../replay/src/coreHandlers/handleClick.ts | 7 +++ .../src/coreHandlers/util/onWindowOpen.ts | 44 +++++++++++++ 4 files changed, 116 insertions(+) create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts create mode 100644 packages/replay/src/coreHandlers/util/onWindowOpen.ts diff --git a/packages/browser-integration-tests/suites/replay/slowClick/template.html b/packages/browser-integration-tests/suites/replay/slowClick/template.html index f49c8b1d410d..83bda10053d1 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/template.html +++ b/packages/browser-integration-tests/suites/replay/slowClick/template.html @@ -19,6 +19,7 @@ + Link Link external @@ -73,6 +74,9 @@

Bottom

document.getElementById('mouseDownButton').addEventListener('mousedown', () => { document.getElementById('out').innerHTML += 'mutationButton clicked
'; }); + document.getElementById('windowOpenButton').addEventListener('click', () => { + window.open('https://example.com/', '_self'); + }); // Do nothing on these elements document diff --git a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts new file mode 100644 index 000000000000..04dc4bb745e7 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts @@ -0,0 +1,61 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; + +sentryTest('window.open() is considered for slow click', async ({ getLocalTestUrl, page }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + const reqPromise0 = waitForReplayRequest(page, 0); + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + const url = await getLocalTestUrl({ testDir: __dirname }); + + await page.goto(url); + await reqPromise0; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + await page.click('#windowOpenButton'); + const navPromise = page.waitForURL('https://example.com/'); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + + expect(breadcrumbs).toEqual([ + { + category: 'ui.click', + data: { + node: { + attributes: { + id: 'windowOpenButton', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '****** ****', + }, + nodeId: expect.any(Number), + }, + message: 'body > button#windowOpenButton', + timestamp: expect.any(Number), + type: 'default', + }, + ]); + + await navPromise; + + // Ensure window.open() still works as expected + expect(await page.url()).toBe('https://example.com/'); +}); diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index 5a75413b00d1..c5a65ddfdca0 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -4,6 +4,7 @@ import { WINDOW } from '../constants'; import type { MultiClickFrame, ReplayClickDetector, ReplayContainer, SlowClickConfig, SlowClickFrame } from '../types'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; import { getClickTargetNode } from './util/domUtils'; +import { onWindowOpen } from './util/onWindowOpen'; type ClickBreadcrumb = Breadcrumb & { timestamp: number; @@ -68,6 +69,11 @@ export class ClickDetector implements ReplayClickDetector { this._lastScroll = nowInSeconds(); }; + const cleanupWindowOpen = onWindowOpen(() => { + // Treat window.open as mutation + this._lastMutation = nowInSeconds(); + }); + const clickHandler = (event: MouseEvent): void => { if (!event.target) { return; @@ -94,6 +100,7 @@ export class ClickDetector implements ReplayClickDetector { this._teardown = () => { WINDOW.removeEventListener('scroll', scrollHandler); WINDOW.removeEventListener('click', clickHandler); + cleanupWindowOpen(); obs.disconnect(); this._clicks = []; diff --git a/packages/replay/src/coreHandlers/util/onWindowOpen.ts b/packages/replay/src/coreHandlers/util/onWindowOpen.ts new file mode 100644 index 000000000000..e3b6b7ac92ed --- /dev/null +++ b/packages/replay/src/coreHandlers/util/onWindowOpen.ts @@ -0,0 +1,44 @@ +import { fill } from '@sentry/utils'; + +import { WINDOW } from '../../constants'; + +type WindowOpenHandler = () => void; + +let handlers: undefined | WindowOpenHandler[]; + +/** + * Register a handler to be called when `window.open()` is called. + * Returns a cleanup function. + */ +export function onWindowOpen(cb: WindowOpenHandler): () => void { + // Ensure to only register this once + if (!handlers) { + handlers = []; + monkeyPatchWindowOpen(); + } + + handlers.push(cb); + + return () => { + const pos = handlers ? handlers.indexOf(cb) : -1; + if (pos > -1) { + (handlers as WindowOpenHandler[]).splice(pos, 1); + } + }; +} + +function monkeyPatchWindowOpen(): void { + fill(WINDOW, 'open', function (originalWindowOpen: () => void): () => void { + return function (...args: unknown[]): void { + if (handlers) { + try { + handlers.forEach(handler => handler()); + } catch (e) { + // ignore errors in here + } + } + + return originalWindowOpen.apply(WINDOW, args); + }; + }); +} From a00b111db76863dba48bb74d743acea2bb83a7a2 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 19 Jun 2023 13:55:53 +0200 Subject: [PATCH 2/3] ref: fix test --- .../suites/replay/slowClick/template.html | 2 +- .../suites/replay/slowClick/windowOpen/test.ts | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/slowClick/template.html b/packages/browser-integration-tests/suites/replay/slowClick/template.html index 83bda10053d1..19bd283db90e 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/template.html +++ b/packages/browser-integration-tests/suites/replay/slowClick/template.html @@ -75,7 +75,7 @@

Bottom

document.getElementById('out').innerHTML += 'mutationButton clicked
'; }); document.getElementById('windowOpenButton').addEventListener('click', () => { - window.open('https://example.com/', '_self'); + window.open('https://example.com/', '_blank'); }); // Do nothing on these elements diff --git a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts index 04dc4bb745e7..ee160820c769 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts @@ -3,7 +3,7 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../../utils/fixtures'; import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; -sentryTest('window.open() is considered for slow click', async ({ getLocalTestUrl, page }) => { +sentryTest('window.open() is considered for slow click', async ({ getLocalTestUrl, page, browser }) => { if (shouldSkipReplayTest()) { sentryTest.skip(); } @@ -29,8 +29,11 @@ sentryTest('window.open() is considered for slow click', async ({ getLocalTestUr return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); }); + // Ensure window.open() still works as expected + const context = browser.contexts()[0]; + const waitForNewPage = context.waitForEvent('page'); + await page.click('#windowOpenButton'); - const navPromise = page.waitForURL('https://example.com/'); const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); @@ -54,8 +57,10 @@ sentryTest('window.open() is considered for slow click', async ({ getLocalTestUr }, ]); - await navPromise; + await waitForNewPage; - // Ensure window.open() still works as expected - expect(await page.url()).toBe('https://example.com/'); + const pages = context.pages(); + + expect(pages.length).toBe(2); + expect(pages[1].url()).toBe('https://example.com/'); }); From c5aa4839b76b51671a04eefc8ec19b7a055d7885 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 19 Jun 2023 14:12:11 +0200 Subject: [PATCH 3/3] fix less flaky --- .../suites/replay/slowClick/windowOpen/test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts index ee160820c769..4c9401234ea2 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts @@ -37,7 +37,10 @@ sentryTest('window.open() is considered for slow click', async ({ getLocalTestUr const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); - expect(breadcrumbs).toEqual([ + // Filter out potential blur breadcrumb, as otherwise this can be flaky + const filteredBreadcrumb = breadcrumbs.filter(breadcrumb => breadcrumb.category !== 'ui.blur'); + + expect(filteredBreadcrumb).toEqual([ { category: 'ui.click', data: {