From 4fd5ebec3ce9a6d8e8c1796ed3962f98fe835590 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt?= Date: Tue, 7 Feb 2023 14:16:22 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20frustration=20animation=20?= =?UTF-8?q?in=20session=20replay=20(#1999)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 fix frustration animation in session replay Frustration records used to reference MouseInteraction/Click records. Since Click Actions are now generated based on mouseup events, we broke this reference. There is no MouseInteraction/PointerUp record that Frustration can reference. This commit is a quick fix. It changes how we are generating MouseInteraction/MouseUp records: instead of listening to mouseup DOM events, we now listen to pointerup events, allowing Frustration records to reference MouseInteraction/MouseUp records. This works on the player side without any modification. In the context of supporting Mobile Session Replay, we introduced `PointerInteraction` records used by the Mobile SDKs in place of `MouseInteraction`. In the future, it would be great to replace `MouseInteraction` by `PointerInteraction` in the Browser SDK so we have an uniform way to convey such interaction. This would cleanly solve the issue since we would have `PointerInteraction/Up` records that we could reference from `Frustration` records. * ✅ adjust unit tests --- .../rum/src/domain/record/observers.spec.ts | 22 ++++++++++++++++++- packages/rum/src/domain/record/observers.ts | 13 +++++++++-- .../scenario/recorder/recorder.scenario.ts | 18 +++++++-------- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/packages/rum/src/domain/record/observers.spec.ts b/packages/rum/src/domain/record/observers.spec.ts index db9ef38420..30d325bde9 100644 --- a/packages/rum/src/domain/record/observers.spec.ts +++ b/packages/rum/src/domain/record/observers.spec.ts @@ -13,6 +13,7 @@ import type { MousemoveCallBack, } from './observers' import { + getRecordIdForEvent, initStyleSheetObserver, initFrustrationObserver, initInputObserver, @@ -134,7 +135,7 @@ describe('initFrustrationObserver', () => { if (isIE()) { pending('IE not supported') } - mouseEvent = new MouseEvent('click') + mouseEvent = new MouseEvent('pointerup') frustrationsCallbackSpy = jasmine.createSpy() rumData = { @@ -169,6 +170,7 @@ describe('initFrustrationObserver', () => { expect(frustrationRecord.type).toEqual(RecordType.FrustrationRecord) expect(frustrationRecord.timestamp).toEqual(rumData.rawRumEvent.date) expect(frustrationRecord.data.frustrationTypes).toEqual(rumData.rawRumEvent.action.frustration!.type) + expect(frustrationRecord.data.recordIds).toEqual([getRecordIdForEvent(mouseEvent)]) }) it('ignores events other than click actions', () => { @@ -391,6 +393,24 @@ describe('initMouseInteractionObserver', () => { }) }) + it('should generate mouseup record on pointerup DOM event', () => { + const pointerupEvent = createNewEvent('pointerup', { clientX: 1, clientY: 2 }) + a.dispatchEvent(pointerupEvent) + + expect(mouseInteractionCallbackSpy).toHaveBeenCalledWith({ + id: getRecordIdForEvent(pointerupEvent), + type: RecordType.IncrementalSnapshot, + timestamp: jasmine.any(Number), + data: { + source: IncrementalSource.MouseInteraction, + type: MouseInteractionType.MouseUp, + id: jasmine.any(Number), + x: jasmine.any(Number), + y: jasmine.any(Number), + }, + }) + }) + it('should not generate click record if x/y are missing', () => { const clickEvent = createNewEvent('click') a.dispatchEvent(clickEvent) diff --git a/packages/rum/src/domain/record/observers.ts b/packages/rum/src/domain/record/observers.ts index 67dc02bbce..6048cb788e 100644 --- a/packages/rum/src/domain/record/observers.ts +++ b/packages/rum/src/domain/record/observers.ts @@ -51,7 +51,7 @@ const VISUAL_VIEWPORT_OBSERVER_THRESHOLD = 200 const recordIds = new WeakMap() let nextId = 1 -function getRecordIdForEvent(event: Event): number { +export function getRecordIdForEvent(event: Event): number { if (!recordIds.has(event)) { recordIds.set(event, nextId++) } @@ -184,7 +184,16 @@ export function initMoveObserver(cb: MousemoveCallBack): ListenerHandler { } const eventTypeToMouseInteraction = { - [DOM_EVENT.MOUSE_UP]: MouseInteractionType.MouseUp, + // Listen for pointerup DOM events instead of mouseup for MouseInteraction/MouseUp records. This + // allows to reference such records from Frustration records. + // + // In the context of supporting Mobile Session Replay, we introduced `PointerInteraction` records + // used by the Mobile SDKs in place of `MouseInteraction`. In the future, we should replace + // `MouseInteraction` by `PointerInteraction` in the Browser SDK so we have an uniform way to + // convey such interaction. This would cleanly solve the issue since we would have + // `PointerInteraction/Up` records that we could reference from `Frustration` records. + [DOM_EVENT.POINTER_UP]: MouseInteractionType.MouseUp, + [DOM_EVENT.MOUSE_DOWN]: MouseInteractionType.MouseDown, [DOM_EVENT.CLICK]: MouseInteractionType.Click, [DOM_EVENT.CONTEXT_MENU]: MouseInteractionType.ContextMenu, diff --git a/test/e2e/scenario/recorder/recorder.scenario.ts b/test/e2e/scenario/recorder/recorder.scenario.ts index 2efcf0c584..c411143cb8 100644 --- a/test/e2e/scenario/recorder/recorder.scenario.ts +++ b/test/e2e/scenario/recorder/recorder.scenario.ts @@ -719,7 +719,7 @@ describe('recorder', () => { describe('frustration records', () => { createTest('should detect a dead click and match it to mouse interaction record') - .withRum({ trackFrustrations: true }) + .withRum({ trackFrustrations: true, enableExperimentalFeatures: ['dead_click_fixes'] }) .withRumInit(initRumAndStartRecording) .withSetup(bundleSetup) .run(async ({ serverEvents }) => { @@ -730,20 +730,20 @@ describe('recorder', () => { expect(serverEvents.sessionReplay.length).toBe(1) const { segment } = serverEvents.sessionReplay[0] - const clickRecords = findMouseInteractionRecords(segment.data, MouseInteractionType.Click) + const mouseupRecords = findMouseInteractionRecords(segment.data, MouseInteractionType.MouseUp) const frustrationRecords = findAllFrustrationRecords(segment.data) - expect(clickRecords.length).toBe(1) - expect(clickRecords[0].id).toBeTruthy('mouse interaction record should have an id') + expect(mouseupRecords.length).toBe(1) + expect(mouseupRecords[0].id).toBeTruthy('mouse interaction record should have an id') expect(frustrationRecords.length).toBe(1) expect(frustrationRecords[0].data).toEqual({ frustrationTypes: [FrustrationType.DEAD_CLICK], - recordIds: [clickRecords[0].id!], + recordIds: [mouseupRecords[0].id!], }) }) createTest('should detect a rage click and match it to mouse interaction records') - .withRum({ trackFrustrations: true }) + .withRum({ trackFrustrations: true, enableExperimentalFeatures: ['dead_click_fixes'] }) .withRumInit(initRumAndStartRecording) .withSetup(bundleSetup) .withBody( @@ -763,14 +763,14 @@ describe('recorder', () => { expect(serverEvents.sessionReplay.length).toBe(1) const { segment } = serverEvents.sessionReplay[0] - const clickRecords = findMouseInteractionRecords(segment.data, MouseInteractionType.Click) + const mouseupRecords = findMouseInteractionRecords(segment.data, MouseInteractionType.MouseUp) const frustrationRecords = findAllFrustrationRecords(segment.data) - expect(clickRecords.length).toBe(4) + expect(mouseupRecords.length).toBe(4) expect(frustrationRecords.length).toBe(1) expect(frustrationRecords[0].data).toEqual({ frustrationTypes: [FrustrationType.RAGE_CLICK], - recordIds: clickRecords.map((r) => r.id!), + recordIds: mouseupRecords.map((r) => r.id!), }) }) })