From 757ffbfe363c932f1e1fc975841bc1ecaa479ecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt?= Date: Mon, 30 Jan 2023 14:56:44 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9A=97=F0=9F=90=9B=20[RUMF-1296]=20use=20poi?= =?UTF-8?q?nterup=20to=20trigger=20click=20actions=20=20(#1958)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ♻️ [RUMF-1296] simplify a bit how we compute input user activity This commit will help implementing click actions triggered by pointerup behind a FF. * ♻️✅ simplify trackClickActions tests * 🐛 [RUMF-1296] ignore non-primary pointer events In 'listenActionEvents', we expect that a 'click' event is preceded by a single 'pointerdown' event. It turns out this is not always the case on multitouch devices. This issue would be amplified when listening to 'pointerup' events. Let's just focus on the 'primary' pointer events. This will ignore secondary touches until we implement proper support for them (if ever). * ⚗🐛 [RUMF-1296] use pointerup to trigger click actions This slightly change the action beginning, but takes any activity produced by 'pointerup' 'mouseup' 'touchend' events into account. If those actions don't produce any activity, the action start is likely to be the same as before, since 'pointerup/mouseup/touchend' should be triggered very close to the 'click' event. * 🔊⚗ [RUMF-1296] record the delay between pointerup and click * 🚩 rename feature flag * 🐛 restore existing behavior for dead clicks * 👌 rename onActionStart to onStartEvent --- .../action/actionCollection.spec.ts | 1 + .../action/actionCollection.ts | 1 + .../action/listenActionEvents.spec.ts | 87 ++++++-- .../action/listenActionEvents.ts | 92 ++++++--- .../action/trackClickActions.spec.ts | 188 +++++++++++------- .../action/trackClickActions.ts | 36 ++-- packages/rum-core/src/rawRumEvent.types.ts | 1 + packages/rum-core/test/specHelper.ts | 10 +- 8 files changed, 276 insertions(+), 140 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts index ae5db2d9b6..c7203b3459 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts @@ -87,6 +87,7 @@ describe('actionCollection', () => { x: 1, y: 2, }, + pointer_up_delay: undefined, }, }, }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts index ac0208f383..7a21b3059f 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts @@ -79,6 +79,7 @@ function processAction( action: { target: action.target, position: action.position, + pointer_up_delay: action.pointerUpDelay, }, }, } diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts index 96d9de98f9..bb1497fc53 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -6,14 +6,14 @@ import { listenActionEvents } from './listenActionEvents' describe('listenActionEvents', () => { let actionEventsHooks: { - onClick: jasmine.Spy['onClick']> + onStartEvent: jasmine.Spy['onStartEvent']> onPointerDown: jasmine.Spy['onPointerDown']> } let stopListenEvents: () => void beforeEach(() => { actionEventsHooks = { - onClick: jasmine.createSpy(), + onStartEvent: jasmine.createSpy(), onPointerDown: jasmine.createSpy().and.returnValue({}), } ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) @@ -28,11 +28,32 @@ describe('listenActionEvents', () => { expect(actionEventsHooks.onPointerDown).toHaveBeenCalledOnceWith(jasmine.objectContaining({ type: 'pointerdown' })) }) + it('ignore non-primary pointerdown events', () => { + emulateClick({ + beforeMouseUp() { + window.dispatchEvent(createNewEvent('pointerdown', { target: document.body, isPrimary: false })) + }, + }) + expect(actionEventsHooks.onPointerDown).toHaveBeenCalledTimes(1) + }) + it('listen to click events', () => { emulateClick() - expect(actionEventsHooks.onClick).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), + jasmine.any(Function), + jasmine.any(Function) + ) + }) + + it('listen to non-primary click events', () => { + // This emulates a Chrome behavior where all click events are non-primary + emulateClick({ clickEventIsPrimary: false }) + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( + {}, + jasmine.objectContaining({ type: 'click' }), + jasmine.any(Function), jasmine.any(Function) ) }) @@ -45,23 +66,46 @@ describe('listenActionEvents', () => { it('can abort click lifecycle by returning undefined from the onPointerDown callback', () => { actionEventsHooks.onPointerDown.and.returnValue(undefined) emulateClick() - expect(actionEventsHooks.onClick).not.toHaveBeenCalled() + expect(actionEventsHooks.onStartEvent).not.toHaveBeenCalled() }) - it('passes the context created in onPointerDown to onClick', () => { + it('passes the context created in onPointerDown to onStartEvent', () => { const context = {} actionEventsHooks.onPointerDown.and.returnValue(context) emulateClick() - expect(actionEventsHooks.onClick.calls.mostRecent().args[0]).toBe(context) + expect(actionEventsHooks.onStartEvent.calls.mostRecent().args[0]).toBe(context) }) it('ignore "click" events if no "pointerdown" event happened since the previous "click" event', () => { emulateClick() - actionEventsHooks.onClick.calls.reset() + actionEventsHooks.onStartEvent.calls.reset() window.dispatchEvent(createNewEvent('click', { target: document.body })) - expect(actionEventsHooks.onClick).not.toHaveBeenCalled() + expect(actionEventsHooks.onStartEvent).not.toHaveBeenCalled() + }) + + describe('dead_click_fixes experimental feature', () => { + beforeEach(() => { + stopListenEvents() + + updateExperimentalFeatures(['dead_click_fixes']) + ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) + }) + + afterEach(() => { + resetExperimentalFeatures() + }) + + it('listen to pointerup events', () => { + emulateClick() + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( + {}, + jasmine.objectContaining({ type: 'pointerup' }), + jasmine.any(Function), + jasmine.any(Function) + ) + }) }) describe('selection change', () => { @@ -133,7 +177,7 @@ describe('listenActionEvents', () => { }) function hasSelectionChanged() { - return actionEventsHooks.onClick.calls.mostRecent().args[2]().selection + return actionEventsHooks.onStartEvent.calls.mostRecent().args[2]().selection } function emulateNodeSelection( @@ -185,11 +229,11 @@ describe('listenActionEvents', () => { expect(hasInputUserActivity()).toBe(true) }) - describe('with fix_dead_clicks_after_input flag', () => { + describe('with dead_click_fixes flag', () => { beforeEach(() => { stopListenEvents() - updateExperimentalFeatures(['fix_dead_clicks_after_input']) + updateExperimentalFeatures(['dead_click_fixes']) ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) }) @@ -205,6 +249,13 @@ describe('listenActionEvents', () => { }) }) + it('without dead_click_fixes, input events that precede clicks should still be taken into account', () => { + emulateInputEvent() + emulateClick() + clock.tick(1) // run immediate timeouts + expect(hasInputUserActivity()).toBe(true) + }) + it('click and type should report an input user activity', () => { emulateClick({ beforeMouseUp() { @@ -218,16 +269,20 @@ describe('listenActionEvents', () => { window.dispatchEvent(createNewEvent('input')) } function hasInputUserActivity() { - return actionEventsHooks.onClick.calls.mostRecent().args[2]().input + return actionEventsHooks.onStartEvent.calls.mostRecent().args[2]().input } }) - function emulateClick({ beforeMouseUp, target = document.body }: { beforeMouseUp?(): void; target?: Node } = {}) { - window.dispatchEvent(createNewEvent('pointerdown', { target })) + function emulateClick({ + beforeMouseUp, + target = document.body, + clickEventIsPrimary = undefined, + }: { beforeMouseUp?(): void; target?: Node; clickEventIsPrimary?: boolean } = {}) { + window.dispatchEvent(createNewEvent('pointerdown', { target, isPrimary: true })) window.dispatchEvent(createNewEvent('mousedown', { target })) beforeMouseUp?.() - window.dispatchEvent(createNewEvent('pointerup', { target })) + window.dispatchEvent(createNewEvent('pointerup', { target, isPrimary: true })) window.dispatchEvent(createNewEvent('mouseup', { target })) - window.dispatchEvent(createNewEvent('click', { target })) + window.dispatchEvent(createNewEvent('click', { target, isPrimary: clickEventIsPrimary })) } }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index a6306fc5b8..b247a7a041 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -1,30 +1,45 @@ -import { addEventListener, DOM_EVENT, isExperimentalFeatureEnabled, monitor } from '@datadog/browser-core' +import type { TimeStamp } from '@datadog/browser-core' +import { addEventListener, DOM_EVENT, isExperimentalFeatureEnabled, timeStampNow } from '@datadog/browser-core' export type MouseEventOnElement = MouseEvent & { target: Element } -export type GetUserActivity = () => { selection: boolean; input: boolean } +export interface UserActivity { + selection: boolean + input: boolean +} export interface ActionEventsHooks { onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined - onClick: (context: ClickContext, event: MouseEventOnElement, getUserActivity: GetUserActivity) => void + onStartEvent: ( + context: ClickContext, + event: MouseEventOnElement, + getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined + ) => void } -export function listenActionEvents({ onPointerDown, onClick }: ActionEventsHooks) { - let hasSelectionChanged = false +export function listenActionEvents({ onPointerDown, onStartEvent }: ActionEventsHooks) { let selectionEmptyAtPointerDown: boolean - let hasInputChanged = false + let userActivity: UserActivity = { + selection: false, + input: false, + } let clickContext: ClickContext | undefined const listeners = [ addEventListener( window, DOM_EVENT.POINTER_DOWN, - (event) => { - hasSelectionChanged = false - selectionEmptyAtPointerDown = isSelectionEmpty() - if (isExperimentalFeatureEnabled('fix_dead_clicks_after_input')) { - hasInputChanged = false - } - if (isMouseEventOnElement(event)) { + (event: PointerEvent) => { + if (isValidMouseEvent(event)) { + selectionEmptyAtPointerDown = isSelectionEmpty() + userActivity = { + selection: false, + input: isExperimentalFeatureEnabled('dead_click_fixes') + ? false + : // Mimics the issue that was fixed in https://github.com/DataDog/browser-sdk/pull/1968 + // The goal is to release all dead click fixes at the same time + userActivity.input, + } clickContext = onPointerDown(event) } }, @@ -36,7 +51,7 @@ export function listenActionEvents({ onPointerDown, onClick }: Act DOM_EVENT.SELECTION_CHANGE, () => { if (!selectionEmptyAtPointerDown || !isSelectionEmpty()) { - hasSelectionChanged = true + userActivity.selection = true } }, { capture: true } @@ -44,24 +59,29 @@ export function listenActionEvents({ onPointerDown, onClick }: Act addEventListener( window, - DOM_EVENT.CLICK, - (clickEvent: MouseEvent) => { - if (isMouseEventOnElement(clickEvent) && clickContext) { + isExperimentalFeatureEnabled('dead_click_fixes') ? DOM_EVENT.POINTER_UP : DOM_EVENT.CLICK, + (startEvent: MouseEvent) => { + if (isValidMouseEvent(startEvent) && clickContext) { // Use a scoped variable to make sure the value is not changed by other clicks - const userActivity = { - selection: hasSelectionChanged, - input: hasInputChanged, - } - if (!hasInputChanged) { - setTimeout( - monitor(() => { - userActivity.input = hasInputChanged - }) + const localUserActivity = userActivity + let clickEventTimeStamp: TimeStamp | undefined + onStartEvent( + clickContext, + startEvent, + () => localUserActivity, + () => clickEventTimeStamp + ) + clickContext = undefined + if (isExperimentalFeatureEnabled('dead_click_fixes')) { + addEventListener( + window, + DOM_EVENT.CLICK, + () => { + clickEventTimeStamp = timeStampNow() + }, + { capture: true, once: true } ) } - - onClick(clickContext, clickEvent, () => userActivity) - clickContext = undefined } }, { capture: true } @@ -71,7 +91,7 @@ export function listenActionEvents({ onPointerDown, onClick }: Act window, DOM_EVENT.INPUT, () => { - hasInputChanged = true + userActivity.input = true }, { capture: true } ), @@ -89,6 +109,14 @@ function isSelectionEmpty(): boolean { return !selection || selection.isCollapsed } -function isMouseEventOnElement(event: Event): event is MouseEventOnElement { - return event.target instanceof Element +function isValidMouseEvent(event: MouseEvent): event is MouseEventOnElement { + return ( + event.target instanceof Element && + // Only consider 'primary' pointer events for now. Multi-touch support could be implemented in + // the future. + // On Chrome, click events are PointerEvent with `isPrimary = false`, but we should still + // consider them valid. This could be removed when we enable the `click-action-on-pointerup` + // flag, since we won't rely on click events anymore. + (event.type === 'click' || (event as PointerEvent).isPrimary !== false) + ) } diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts index 1665fd7db2..9a9cbd5238 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -1,4 +1,4 @@ -import type { Context, Observable, Duration } from '@datadog/browser-core' +import type { Context, Duration } from '@datadog/browser-core' import { addDuration, updateExperimentalFeatures, @@ -7,7 +7,6 @@ import { timeStampNow, relativeNow, } from '@datadog/browser-core' -import type { Clock } from '../../../../../core/test/specHelper' import { createNewEvent } from '../../../../../core/test/specHelper' import type { TestSetupBuilder } from '../../../../test/specHelper' import { setup } from '../../../../test/specHelper' @@ -83,9 +82,9 @@ describe('trackClickActions', () => { }) it('starts a click action when clicking on an element', () => { - const { domMutationObservable, clock } = setupBuilder.build() + const { clock } = setupBuilder.build() const pointerDownClocks = clocksNow() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) expect(findActionId()).not.toBeUndefined() clock.tick(EXPIRE_DELAY) const domEvent = createNewEvent('click', { target: document.createElement('button') }) @@ -109,6 +108,7 @@ describe('trackClickActions', () => { target: undefined, position: undefined, events: [domEvent], + pointerUpDelay: undefined, }, ]) }) @@ -123,8 +123,8 @@ describe('trackClickActions', () => { }) it('should set click position and target', () => { - const { domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + const { clock } = setupBuilder.build() + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(events[0]).toEqual( jasmine.objectContaining({ @@ -140,9 +140,9 @@ describe('trackClickActions', () => { }) it('should keep track of previously validated click actions', () => { - const { domMutationObservable, clock } = setupBuilder.build() + const { clock } = setupBuilder.build() const pointerDownStart = relativeNow() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(findActionId(addDuration(pointerDownStart, EMULATED_CLICK_DURATION))).not.toBeUndefined() @@ -151,7 +151,7 @@ describe('trackClickActions', () => { it('counts errors occurring during the click action', () => { const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, createFakeErrorEvent()) clock.tick(BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY) @@ -171,9 +171,9 @@ describe('trackClickActions', () => { }) it('does not count child events unrelated to the click action', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() + const { lifeCycle, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, { type: RumEventType.RESOURCE, @@ -188,12 +188,10 @@ describe('trackClickActions', () => { }) it('should take the name from user-configured attribute', () => { - const { domMutationObservable, clock } = setupBuilder - .withConfiguration({ actionNameAttribute: 'data-my-custom-attribute' }) - .build() + const { clock } = setupBuilder.withConfiguration({ actionNameAttribute: 'data-my-custom-attribute' }).build() button.setAttribute('data-my-custom-attribute', 'test-1') - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -202,8 +200,8 @@ describe('trackClickActions', () => { describe('without tracking frustrations', () => { it('discards any click action with a negative duration', () => { - const { domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock, button, -1) + const { clock } = setupBuilder.build() + emulateClick({ activity: { delay: -1 } }) expect(findActionId()).not.toBeUndefined() clock.tick(EXPIRE_DELAY) @@ -212,8 +210,8 @@ describe('trackClickActions', () => { }) it('discards ongoing click action on view ended', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + const { lifeCycle, clock } = setupBuilder.build() + emulateClick({ activity: {} }) expect(findActionId()).not.toBeUndefined() lifeCycle.notify(LifeCycleEventType.VIEW_ENDED, { @@ -226,11 +224,11 @@ describe('trackClickActions', () => { }) it('ignores any starting click action while another one is ongoing', () => { - const { domMutationObservable, clock } = setupBuilder.build() + const { clock } = setupBuilder.build() const firstPointerDownTimeStamp = timeStampNow() - emulateClickWithActivity(domMutationObservable, clock) - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -239,7 +237,7 @@ describe('trackClickActions', () => { it('discards a click action when nothing happens after a click', () => { const { clock } = setupBuilder.build() - emulateClickWithoutActivity(clock) + emulateClick() clock.tick(EXPIRE_DELAY) expect(events).toEqual([]) @@ -247,8 +245,8 @@ describe('trackClickActions', () => { }) it('ignores a click action if it fails to find a name', () => { - const { domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock, emptyElement) + const { clock } = setupBuilder.build() + emulateClick({ activity: {}, target: emptyElement }) expect(findActionId()).toBeUndefined() clock.tick(EXPIRE_DELAY) @@ -256,9 +254,9 @@ describe('trackClickActions', () => { }) it('does not populate the frustrationTypes array', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() + const { lifeCycle, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, createFakeErrorEvent()) clock.tick(EXPIRE_DELAY) @@ -273,8 +271,8 @@ describe('trackClickActions', () => { }) it('discards any click action with a negative duration', () => { - const { domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock, button, -1) + const { clock } = setupBuilder.build() + emulateClick({ activity: { delay: -1 } }) expect(findActionId()!.length).toEqual(2) clock.tick(EXPIRE_DELAY) @@ -283,8 +281,8 @@ describe('trackClickActions', () => { }) it('ongoing click action is stopped on view end', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock, button, BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY) + const { lifeCycle, clock } = setupBuilder.build() + emulateClick({ activity: { delay: BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY } }) clock.tick(BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY) @@ -297,12 +295,12 @@ describe('trackClickActions', () => { }) it('collect click actions even if another one is ongoing', () => { - const { domMutationObservable, clock } = setupBuilder.build() + const { clock } = setupBuilder.build() const firstPointerDownTimeStamp = timeStampNow() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) const secondPointerDownTimeStamp = timeStampNow() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(2) @@ -312,7 +310,7 @@ describe('trackClickActions', () => { it('collect click actions even if nothing happens after a click (dead click)', () => { const { clock } = setupBuilder.build() - emulateClickWithoutActivity(clock) + emulateClick() clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -322,7 +320,7 @@ describe('trackClickActions', () => { it('does not set a duration for dead clicks', () => { const { clock } = setupBuilder.build() - emulateClickWithoutActivity(clock) + emulateClick() clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -330,8 +328,8 @@ describe('trackClickActions', () => { }) it('collect click actions even if it fails to find a name', () => { - const { domMutationObservable, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock, emptyElement) + const { clock } = setupBuilder.build() + emulateClick({ activity: {}, target: emptyElement }) expect(findActionId()!.length).toBeGreaterThan(0) clock.tick(EXPIRE_DELAY) @@ -340,28 +338,28 @@ describe('trackClickActions', () => { describe('rage clicks', () => { it('considers a chain of three clicks or more as a single action with "rage" frustration type', () => { - const { domMutationObservable, clock } = setupBuilder.build() + const { clock } = setupBuilder.build() const firstPointerDownTimeStamp = timeStampNow() - const actionDuration = 5 - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) + const activityDelay = 5 + emulateClick({ activity: { delay: activityDelay } }) + emulateClick({ activity: { delay: activityDelay } }) + emulateClick({ activity: { delay: activityDelay } }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) expect(events[0].startClocks.timeStamp).toBe(addDuration(firstPointerDownTimeStamp, EMULATED_CLICK_DURATION)) expect(events[0].frustrationTypes).toEqual([FrustrationType.RAGE_CLICK]) expect(events[0].duration).toBe( - (MAX_DURATION_BETWEEN_CLICKS + 2 * actionDuration + 2 * EMULATED_CLICK_DURATION) as Duration + (MAX_DURATION_BETWEEN_CLICKS + 2 * activityDelay + 2 * EMULATED_CLICK_DURATION) as Duration ) }) it('should contain original events from of rage sequence', () => { - const { domMutationObservable, clock } = setupBuilder.build() - const actionDuration = 5 - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) - emulateClickWithActivity(domMutationObservable, clock, undefined, actionDuration) + const { clock } = setupBuilder.build() + const activityDelay = 5 + emulateClick({ activity: { delay: activityDelay } }) + emulateClick({ activity: { delay: activityDelay } }) + emulateClick({ activity: { delay: activityDelay } }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -370,19 +368,19 @@ describe('trackClickActions', () => { }) it('aggregates frustrationTypes from all clicks', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() + const { lifeCycle, clock } = setupBuilder.build() // Dead - emulateClickWithoutActivity(clock) + emulateClick() clock.tick(PAGE_ACTIVITY_VALIDATION_DELAY) // Error - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, createFakeErrorEvent()) clock.tick(PAGE_ACTIVITY_VALIDATION_DELAY) // Third click to make a rage click - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) @@ -398,9 +396,9 @@ describe('trackClickActions', () => { describe('error clicks', () => { it('considers a "click with activity" followed by an error as a click action with "error" frustration type', () => { - const { lifeCycle, domMutationObservable, clock } = setupBuilder.build() + const { lifeCycle, clock } = setupBuilder.build() - emulateClickWithActivity(domMutationObservable, clock) + emulateClick({ activity: {} }) lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, createFakeErrorEvent()) clock.tick(EXPIRE_DELAY) @@ -411,7 +409,7 @@ describe('trackClickActions', () => { it('considers a "click without activity" followed by an error as a click action with "error" (and "dead") frustration type', () => { const { lifeCycle, clock } = setupBuilder.build() - emulateClickWithoutActivity(clock) + emulateClick() lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, createFakeErrorEvent()) clock.tick(EXPIRE_DELAY) @@ -426,33 +424,56 @@ describe('trackClickActions', () => { it('considers a "click without activity" as a dead click', () => { const { clock } = setupBuilder.build() - emulateClickWithoutActivity(clock) + emulateClick() clock.tick(EXPIRE_DELAY) expect(events.length).toBe(1) expect(events[0].frustrationTypes).toEqual([FrustrationType.DEAD_CLICK]) }) + + describe('dead_click_fixes experimental feature', () => { + beforeEach(() => { + updateExperimentalFeatures(['dead_click_fixes']) + }) + + afterEach(() => { + resetExperimentalFeatures() + }) + + it('does not consider a click with activity happening on pointerup as a dead click', () => { + const { clock } = setupBuilder.build() + + emulateClick({ activity: { on: 'pointerup' } }) + + clock.tick(EXPIRE_DELAY) + expect(events.length).toBe(1) + expect(events[0].frustrationTypes).toEqual([]) + }) + + it('reports the delay between pointerup and click event', () => { + const { clock } = setupBuilder.build() + + const pointerUpActivityDelay = 5 as Duration + emulateClick({ activity: { on: 'pointerup', delay: pointerUpActivityDelay } }) + + clock.tick(EXPIRE_DELAY) + expect(events.length).toBe(1) + expect(events[0].pointerUpDelay).toBe(pointerUpActivityDelay) + }) + }) }) }) - function emulateClickWithActivity( - domMutationObservable: Observable, - clock: Clock, - target: HTMLElement = button, - clickActionDuration: number = BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY - ) { - emulateClickWithoutActivity(clock, target) - if (clickActionDuration < 0) { - // Do not use `.tick()` here because negative clock tick does not work since jasmine 4: https://github.com/jasmine/jasmine/pull/1948 - clock.setDate(new Date(Date.now() + clickActionDuration)) - } else { - clock.tick(clickActionDuration) + function emulateClick({ + target = button, + activity, + }: { + target?: HTMLElement + activity?: { + delay?: number + on?: 'pointerup' | 'click' } - // Since we don't collect dom mutations for this test, manually dispatch one - domMutationObservable.notify() - } - - function emulateClickWithoutActivity(clock: Clock, target: HTMLElement = button) { + } = {}) { const targetPosition = target.getBoundingClientRect() const offsetX = targetPosition.width / 2 const offsetY = targetPosition.height / 2 @@ -463,11 +484,28 @@ describe('trackClickActions', () => { offsetX, offsetY, timeStamp: timeStampNow(), + isPrimary: true, } target.dispatchEvent(createNewEvent('pointerdown', eventProperties)) - clock.tick(EMULATED_CLICK_DURATION) + setupBuilder.clock!.tick(EMULATED_CLICK_DURATION) target.dispatchEvent(createNewEvent('pointerup', eventProperties)) + emulateActivityIfNeeded('pointerup') target.dispatchEvent(createNewEvent('click', eventProperties)) + emulateActivityIfNeeded('click') + + function emulateActivityIfNeeded(event: 'pointerup' | 'click') { + if (activity && (activity.on ?? 'click') === event) { + const delay = activity.delay ?? BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY + if (delay < 0) { + // Do not use `.tick()` here because negative clock tick does not work since jasmine 4: https://github.com/jasmine/jasmine/pull/1948 + setupBuilder.clock!.setDate(new Date(Date.now() + delay)) + } else { + setupBuilder.clock!.tick(delay) + } + // Since we don't collect dom mutations for this test, manually dispatch one + setupBuilder.domMutationObservable.notify() + } + } } function createFakeErrorEvent() { diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index 6d9b48835a..abc9e37305 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -24,7 +24,7 @@ import type { ClickChain } from './clickChain' import { createClickChain } from './clickChain' import { getActionNameFromElement } from './getActionNameFromElement' import { getSelectorFromElement } from './getSelectorFromElement' -import type { MouseEventOnElement, GetUserActivity } from './listenActionEvents' +import type { MouseEventOnElement, UserActivity } from './listenActionEvents' import { listenActionEvents } from './listenActionEvents' import { computeFrustration } from './computeFrustration' @@ -51,6 +51,7 @@ export interface ClickAction { event: MouseEventOnElement frustrationTypes: FrustrationType[] events: Event[] + pointerUpDelay?: Duration } export interface ActionContexts { @@ -80,8 +81,8 @@ export function trackClickActions( const { stop: stopActionEventsListener } = listenActionEvents({ onPointerDown: (pointerDownEvent) => processPointerDown(configuration, history, pointerDownEvent), - onClick: (clickActionBase, clickEvent, getUserActivity) => - processClick( + onStartEvent: (clickActionBase, startEvent, getUserActivity, getClickEventTimeStamp) => + startClickAction( configuration, lifeCycle, domMutationObservable, @@ -89,8 +90,9 @@ export function trackClickActions( stopObservable, appendClickToClickChain, clickActionBase, - clickEvent, - getUserActivity + startEvent, + getUserActivity, + getClickEventTimeStamp ), }) @@ -145,7 +147,7 @@ function processPointerDown( return clickActionBase } -function processClick( +function startClickAction( configuration: RumConfiguration, lifeCycle: LifeCycle, domMutationObservable: Observable, @@ -153,10 +155,11 @@ function processClick( stopObservable: Observable, appendClickToClickChain: (click: Click) => void, clickActionBase: ClickActionBase, - clickEvent: MouseEventOnElement, - getUserActivity: GetUserActivity + startEvent: MouseEventOnElement, + getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined ) { - const click = newClick(lifeCycle, history, getUserActivity, clickActionBase, clickEvent) + const click = newClick(lifeCycle, history, getUserActivity, getClickEventTimeStamp, clickActionBase, startEvent) if (configuration.trackFrustrations) { appendClickToClickChain(click) @@ -246,9 +249,10 @@ export type Click = ReturnType function newClick( lifeCycle: LifeCycle, history: ClickActionIdHistory, - getUserActivity: GetUserActivity, + getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined, clickActionBase: ClickActionBase, - clickEvent: MouseEventOnElement + startEvent: MouseEventOnElement ) { const id = generateUUID() const startClocks = clocksNow() @@ -280,7 +284,7 @@ function newClick( } return { - event: clickEvent, + event: startEvent, stop, stopObservable, @@ -298,7 +302,7 @@ function newClick( isStopped: () => status === ClickStatus.STOPPED || status === ClickStatus.FINALIZED, - clone: () => newClick(lifeCycle, history, getUserActivity, clickActionBase, clickEvent), + clone: () => newClick(lifeCycle, history, getUserActivity, getClickEventTimeStamp, clickActionBase, startEvent), validate: (domEvents?: Event[]) => { stop() @@ -307,6 +311,7 @@ function newClick( } const { resourceCount, errorCount, longTaskCount } = eventCountsSubscription.eventCounts + const clickEventTimeStamp = getClickEventTimeStamp() const clickAction: ClickAction = assign( { type: ActionType.CLICK as const, @@ -319,8 +324,9 @@ function newClick( errorCount, longTaskCount, }, - events: domEvents ?? [clickEvent], - event: clickEvent, + events: domEvents ?? [startEvent], + event: startEvent, + pointerUpDelay: clickEventTimeStamp && elapsed(startClocks.timeStamp, clickEventTimeStamp), }, clickActionBase ) diff --git a/packages/rum-core/src/rawRumEvent.types.ts b/packages/rum-core/src/rawRumEvent.types.ts index 289a6783da..1ea9e1ce51 100644 --- a/packages/rum-core/src/rawRumEvent.types.ts +++ b/packages/rum-core/src/rawRumEvent.types.ts @@ -178,6 +178,7 @@ export interface RawRumActionEvent { x: number y: number } + pointer_up_delay?: Duration } } } diff --git a/packages/rum-core/test/specHelper.ts b/packages/rum-core/test/specHelper.ts index 0ea6140e54..57a1acc5be 100644 --- a/packages/rum-core/test/specHelper.ts +++ b/packages/rum-core/test/specHelper.ts @@ -34,7 +34,8 @@ export interface TestSetupBuilder { withFakeClock: () => TestSetupBuilder beforeBuild: (callback: BeforeBuildCallback) => TestSetupBuilder - clock?: Clock + clock: Clock | undefined + domMutationObservable: Observable cleanup: () => void build: () => TestIO @@ -129,6 +130,11 @@ export function setup(): TestSetupBuilder { } const setupBuilder: TestSetupBuilder = { + domMutationObservable, + get clock() { + return clock + }, + withFakeLocation(initialUrl: string) { fakeLocation = buildLocation(initialUrl) return setupBuilder @@ -159,9 +165,9 @@ export function setup(): TestSetupBuilder { }, withFakeClock() { clock = mockClock() - setupBuilder.clock = clock return setupBuilder }, + beforeBuild(callback: BeforeBuildCallback) { beforeBuildTasks.push(callback) return setupBuilder