From 7dddaa9243c76d7a031812d01474d3eadd87cf53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 17 Jan 2023 10:16:24 +0100 Subject: [PATCH 1/8] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20[RUMF-1296]=20simplify?= =?UTF-8?q?=20a=20bit=20how=20we=20compute=20input=20user=20activity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit will help implementing click actions triggered by pointerup behind a FF. --- .../action/listenActionEvents.ts | 39 +++++++++---------- .../action/trackClickActions.ts | 6 +-- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index 1a259de74c..dddee75d6f 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -1,17 +1,22 @@ -import { addEventListener, DOM_EVENT, monitor } from '@datadog/browser-core' +import { addEventListener, DOM_EVENT } 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 + onClick: (context: ClickContext, event: MouseEventOnElement, getUserActivity: () => UserActivity) => void } export function listenActionEvents({ onPointerDown, onClick }: ActionEventsHooks) { - let hasSelectionChanged = false let selectionEmptyAtPointerDown: boolean - let hasInputChanged = false + let userActivity: UserActivity = { + selection: false, + input: false, + } let clickContext: ClickContext | undefined const listeners = [ @@ -19,8 +24,11 @@ export function listenActionEvents({ onPointerDown, onClick }: Act window, DOM_EVENT.POINTER_DOWN, (event) => { - hasSelectionChanged = false selectionEmptyAtPointerDown = isSelectionEmpty() + userActivity = { + selection: false, + input: false, + } if (isMouseEventOnElement(event)) { clickContext = onPointerDown(event) } @@ -33,7 +41,7 @@ export function listenActionEvents({ onPointerDown, onClick }: Act DOM_EVENT.SELECTION_CHANGE, () => { if (!selectionEmptyAtPointerDown || !isSelectionEmpty()) { - hasSelectionChanged = true + userActivity.selection = true } }, { capture: true } @@ -45,19 +53,8 @@ export function listenActionEvents({ onPointerDown, onClick }: Act (clickEvent: MouseEvent) => { if (isMouseEventOnElement(clickEvent) && 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 - }) - ) - } - - onClick(clickContext, clickEvent, () => userActivity) + const localUserActivity = userActivity + onClick(clickContext, clickEvent, () => localUserActivity) clickContext = undefined } }, @@ -68,7 +65,7 @@ export function listenActionEvents({ onPointerDown, onClick }: Act window, DOM_EVENT.INPUT, () => { - hasInputChanged = true + userActivity.input = true }, { capture: true } ), diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index 6d9b48835a..61ddd623d1 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' @@ -154,7 +154,7 @@ function processClick( appendClickToClickChain: (click: Click) => void, clickActionBase: ClickActionBase, clickEvent: MouseEventOnElement, - getUserActivity: GetUserActivity + getUserActivity: () => UserActivity ) { const click = newClick(lifeCycle, history, getUserActivity, clickActionBase, clickEvent) @@ -246,7 +246,7 @@ export type Click = ReturnType function newClick( lifeCycle: LifeCycle, history: ClickActionIdHistory, - getUserActivity: GetUserActivity, + getUserActivity: () => UserActivity, clickActionBase: ClickActionBase, clickEvent: MouseEventOnElement ) { From 38c32ae667d880048af90fa45c2406068fa51213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 18 Jan 2023 18:30:43 +0100 Subject: [PATCH 2/8] =?UTF-8?q?=E2=99=BB=EF=B8=8F=E2=9C=85=20simplify=20tr?= =?UTF-8?q?ackClickActions=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/trackClickActions.spec.ts | 150 +++++++++--------- packages/rum-core/test/specHelper.ts | 10 +- 2 files changed, 83 insertions(+), 77 deletions(-) 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..cfe2e0c88a 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') }) @@ -123,8 +122,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 +139,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 +150,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 +170,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 +187,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 +199,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 +209,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 +223,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 +236,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 +244,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 +253,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 +270,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 +280,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 +294,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 +309,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 +319,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 +327,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 +337,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 +367,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 +395,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 +408,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,7 +423,7 @@ 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) @@ -435,24 +432,15 @@ describe('trackClickActions', () => { }) }) - 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 } - // 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 @@ -465,9 +453,21 @@ describe('trackClickActions', () => { timeStamp: timeStampNow(), } target.dispatchEvent(createNewEvent('pointerdown', eventProperties)) - clock.tick(EMULATED_CLICK_DURATION) + setupBuilder.clock!.tick(EMULATED_CLICK_DURATION) target.dispatchEvent(createNewEvent('pointerup', eventProperties)) target.dispatchEvent(createNewEvent('click', eventProperties)) + + if (activity) { + 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/test/specHelper.ts b/packages/rum-core/test/specHelper.ts index 70c97df2eb..4ce7e3b271 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 @@ -124,6 +125,11 @@ export function setup(): TestSetupBuilder { } const setupBuilder: TestSetupBuilder = { + domMutationObservable, + get clock() { + return clock + }, + withFakeLocation(initialUrl: string) { fakeLocation = buildLocation(initialUrl) return setupBuilder @@ -154,9 +160,9 @@ export function setup(): TestSetupBuilder { }, withFakeClock() { clock = mockClock() - setupBuilder.clock = clock return setupBuilder }, + beforeBuild(callback: BeforeBuildCallback) { beforeBuildTasks.push(callback) return setupBuilder From a1c833323409bae44325d72c1dc352bae021ffde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 18 Jan 2023 17:42:20 +0100 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1296]=20ignore=20non-p?= =?UTF-8?q?rimary=20pointer=20events?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../action/listenActionEvents.spec.ts | 31 ++++++++++++++++--- .../action/listenActionEvents.ts | 28 +++++++++++------ .../action/trackClickActions.spec.ts | 1 + 3 files changed, 46 insertions(+), 14 deletions(-) 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 10728543c1..7db7caf342 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -27,6 +27,15 @@ 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( @@ -36,6 +45,16 @@ describe('listenActionEvents', () => { ) }) + it('listen to non-primary click events', () => { + // This emulates a Chrome behavior where all click events are non-primary + emulateClick({ clickEventIsPrimary: false }) + expect(actionEventsHooks.onClick).toHaveBeenCalledOnceWith( + {}, + jasmine.objectContaining({ type: 'click' }), + jasmine.any(Function) + ) + }) + it('aborts click lifecycle if the pointerdown event occurs on a non-element', () => { emulateClick({ target: document.createTextNode('foo') }) expect(actionEventsHooks.onPointerDown).not.toHaveBeenCalled() @@ -201,12 +220,16 @@ describe('listenActionEvents', () => { } }) - 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 dddee75d6f..d0d12854c2 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -23,13 +23,13 @@ export function listenActionEvents({ onPointerDown, onClick }: Act addEventListener( window, DOM_EVENT.POINTER_DOWN, - (event) => { - selectionEmptyAtPointerDown = isSelectionEmpty() - userActivity = { - selection: false, - input: false, - } - if (isMouseEventOnElement(event)) { + (event: PointerEvent) => { + if (isValidMouseEvent(event)) { + selectionEmptyAtPointerDown = isSelectionEmpty() + userActivity = { + selection: false, + input: false, + } clickContext = onPointerDown(event) } }, @@ -51,7 +51,7 @@ export function listenActionEvents({ onPointerDown, onClick }: Act window, DOM_EVENT.CLICK, (clickEvent: MouseEvent) => { - if (isMouseEventOnElement(clickEvent) && clickContext) { + if (isValidMouseEvent(clickEvent) && clickContext) { // Use a scoped variable to make sure the value is not changed by other clicks const localUserActivity = userActivity onClick(clickContext, clickEvent, () => localUserActivity) @@ -83,6 +83,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 cfe2e0c88a..57744d97a3 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -451,6 +451,7 @@ describe('trackClickActions', () => { offsetX, offsetY, timeStamp: timeStampNow(), + isPrimary: true, } target.dispatchEvent(createNewEvent('pointerdown', eventProperties)) setupBuilder.clock!.tick(EMULATED_CLICK_DURATION) From 238488e5df035db195c39ced656a8f1fe9af4a40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 18 Jan 2023 15:01:24 +0100 Subject: [PATCH 4/8] =?UTF-8?q?=E2=9A=97=F0=9F=90=9B=20[RUMF-1296]=20use?= =?UTF-8?q?=20pointerup=20to=20trigger=20click=20actions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../action/listenActionEvents.spec.ts | 45 ++++++++++++++----- .../action/listenActionEvents.ts | 14 +++--- .../action/trackClickActions.spec.ts | 45 ++++++++++++++----- .../action/trackClickActions.ts | 22 ++++----- 4 files changed, 87 insertions(+), 39 deletions(-) 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 7db7caf342..a8b1d14621 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -1,3 +1,4 @@ +import { resetExperimentalFeatures, updateExperimentalFeatures } from '@datadog/browser-core' import type { Clock } from '../../../../../core/test/specHelper' import { createNewEvent, mockClock } from '../../../../../core/test/specHelper' import type { ActionEventsHooks } from './listenActionEvents' @@ -5,14 +6,14 @@ import { listenActionEvents } from './listenActionEvents' describe('listenActionEvents', () => { let actionEventsHooks: { - onClick: jasmine.Spy['onClick']> + onActionStart: jasmine.Spy['onActionStart']> onPointerDown: jasmine.Spy['onPointerDown']> } let stopListenEvents: () => void beforeEach(() => { actionEventsHooks = { - onClick: jasmine.createSpy(), + onActionStart: jasmine.createSpy(), onPointerDown: jasmine.createSpy().and.returnValue({}), } ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) @@ -38,7 +39,7 @@ describe('listenActionEvents', () => { it('listen to click events', () => { emulateClick() - expect(actionEventsHooks.onClick).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), jasmine.any(Function) @@ -48,7 +49,7 @@ describe('listenActionEvents', () => { it('listen to non-primary click events', () => { // This emulates a Chrome behavior where all click events are non-primary emulateClick({ clickEventIsPrimary: false }) - expect(actionEventsHooks.onClick).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), jasmine.any(Function) @@ -63,23 +64,45 @@ 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.onActionStart).not.toHaveBeenCalled() }) - it('passes the context created in onPointerDown to onClick', () => { + it('passes the context created in onPointerDown to onActionStart', () => { const context = {} actionEventsHooks.onPointerDown.and.returnValue(context) emulateClick() - expect(actionEventsHooks.onClick.calls.mostRecent().args[0]).toBe(context) + expect(actionEventsHooks.onActionStart.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.onActionStart.calls.reset() window.dispatchEvent(createNewEvent('click', { target: document.body })) - expect(actionEventsHooks.onClick).not.toHaveBeenCalled() + expect(actionEventsHooks.onActionStart).not.toHaveBeenCalled() + }) + + describe('click_action_on_pointerup experimental feature', () => { + beforeEach(() => { + stopListenEvents() + + updateExperimentalFeatures(['click_action_on_pointerup']) + ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) + }) + + afterEach(() => { + resetExperimentalFeatures() + }) + + it('listen to pointerup events', () => { + emulateClick() + expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( + {}, + jasmine.objectContaining({ type: 'pointerup' }), + jasmine.any(Function) + ) + }) }) describe('selection change', () => { @@ -151,7 +174,7 @@ describe('listenActionEvents', () => { }) function hasSelectionChanged() { - return actionEventsHooks.onClick.calls.mostRecent().args[2]().selection + return actionEventsHooks.onActionStart.calls.mostRecent().args[2]().selection } function emulateNodeSelection( @@ -216,7 +239,7 @@ describe('listenActionEvents', () => { window.dispatchEvent(createNewEvent('input')) } function hasInputUserActivity() { - return actionEventsHooks.onClick.calls.mostRecent().args[2]().input + return actionEventsHooks.onActionStart.calls.mostRecent().args[2]().input } }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index d0d12854c2..eaaadbffab 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -1,4 +1,4 @@ -import { addEventListener, DOM_EVENT } from '@datadog/browser-core' +import { addEventListener, DOM_EVENT, isExperimentalFeatureEnabled } from '@datadog/browser-core' export type MouseEventOnElement = MouseEvent & { target: Element } @@ -8,10 +8,10 @@ export interface UserActivity { } export interface ActionEventsHooks { onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined - onClick: (context: ClickContext, event: MouseEventOnElement, getUserActivity: () => UserActivity) => void + onActionStart: (context: ClickContext, startEvent: MouseEventOnElement, getUserActivity: () => UserActivity) => void } -export function listenActionEvents({ onPointerDown, onClick }: ActionEventsHooks) { +export function listenActionEvents({ onPointerDown, onActionStart }: ActionEventsHooks) { let selectionEmptyAtPointerDown: boolean let userActivity: UserActivity = { selection: false, @@ -49,12 +49,12 @@ export function listenActionEvents({ onPointerDown, onClick }: Act addEventListener( window, - DOM_EVENT.CLICK, - (clickEvent: MouseEvent) => { - if (isValidMouseEvent(clickEvent) && clickContext) { + isExperimentalFeatureEnabled('click_action_on_pointerup') ? 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 localUserActivity = userActivity - onClick(clickContext, clickEvent, () => localUserActivity) + onActionStart(clickContext, startEvent, () => localUserActivity) clickContext = undefined } }, 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 57744d97a3..e6272a919c 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -429,6 +429,26 @@ describe('trackClickActions', () => { expect(events.length).toBe(1) expect(events[0].frustrationTypes).toEqual([FrustrationType.DEAD_CLICK]) }) + + describe('click_action_on_pointerup experimental feature', () => { + beforeEach(() => { + updateExperimentalFeatures(['click_action_on_pointerup']) + }) + + 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([]) + }) + }) }) }) @@ -439,6 +459,7 @@ describe('trackClickActions', () => { target?: HTMLElement activity?: { delay?: number + on?: 'pointerup' | 'click' } } = {}) { const targetPosition = target.getBoundingClientRect() @@ -456,18 +477,22 @@ describe('trackClickActions', () => { target.dispatchEvent(createNewEvent('pointerdown', eventProperties)) setupBuilder.clock!.tick(EMULATED_CLICK_DURATION) target.dispatchEvent(createNewEvent('pointerup', eventProperties)) + emulateActivityIfNeeded('pointerup') target.dispatchEvent(createNewEvent('click', eventProperties)) - - if (activity) { - 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) + 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() } - // Since we don't collect dom mutations for this test, manually dispatch one - setupBuilder.domMutationObservable.notify() } } diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index 61ddd623d1..f49ea265b7 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -80,8 +80,8 @@ export function trackClickActions( const { stop: stopActionEventsListener } = listenActionEvents({ onPointerDown: (pointerDownEvent) => processPointerDown(configuration, history, pointerDownEvent), - onClick: (clickActionBase, clickEvent, getUserActivity) => - processClick( + onActionStart: (clickActionBase, startEvent, getUserActivity) => + startClickAction( configuration, lifeCycle, domMutationObservable, @@ -89,7 +89,7 @@ export function trackClickActions( stopObservable, appendClickToClickChain, clickActionBase, - clickEvent, + startEvent, getUserActivity ), }) @@ -145,7 +145,7 @@ function processPointerDown( return clickActionBase } -function processClick( +function startClickAction( configuration: RumConfiguration, lifeCycle: LifeCycle, domMutationObservable: Observable, @@ -153,10 +153,10 @@ function processClick( stopObservable: Observable, appendClickToClickChain: (click: Click) => void, clickActionBase: ClickActionBase, - clickEvent: MouseEventOnElement, + startEvent: MouseEventOnElement, getUserActivity: () => UserActivity ) { - const click = newClick(lifeCycle, history, getUserActivity, clickActionBase, clickEvent) + const click = newClick(lifeCycle, history, getUserActivity, clickActionBase, startEvent) if (configuration.trackFrustrations) { appendClickToClickChain(click) @@ -248,7 +248,7 @@ function newClick( history: ClickActionIdHistory, getUserActivity: () => UserActivity, clickActionBase: ClickActionBase, - clickEvent: MouseEventOnElement + startEvent: MouseEventOnElement ) { const id = generateUUID() const startClocks = clocksNow() @@ -280,7 +280,7 @@ function newClick( } return { - event: clickEvent, + event: startEvent, stop, stopObservable, @@ -298,7 +298,7 @@ function newClick( isStopped: () => status === ClickStatus.STOPPED || status === ClickStatus.FINALIZED, - clone: () => newClick(lifeCycle, history, getUserActivity, clickActionBase, clickEvent), + clone: () => newClick(lifeCycle, history, getUserActivity, clickActionBase, startEvent), validate: (domEvents?: Event[]) => { stop() @@ -319,8 +319,8 @@ function newClick( errorCount, longTaskCount, }, - events: domEvents ?? [clickEvent], - event: clickEvent, + events: domEvents ?? [startEvent], + event: startEvent, }, clickActionBase ) From 7e96c151d5fa999153e88ced0c0ce05cd3193ca6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Thu, 19 Jan 2023 15:03:54 +0100 Subject: [PATCH 5/8] =?UTF-8?q?=F0=9F=94=8A=E2=9A=97=20[RUMF-1296]=20recor?= =?UTF-8?q?d=20the=20delay=20between=20pointerup=20and=20click?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/actionCollection.spec.ts | 1 + .../action/actionCollection.ts | 1 + .../action/listenActionEvents.spec.ts | 3 ++ .../action/listenActionEvents.ts | 28 +++++++++++++++++-- .../action/trackClickActions.spec.ts | 12 ++++++++ .../action/trackClickActions.ts | 16 +++++++---- packages/rum-core/src/rawRumEvent.types.ts | 1 + 7 files changed, 54 insertions(+), 8 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 e0cb0c86e6..ae5239ad1c 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts @@ -78,6 +78,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 a8b1d14621..1ae163da9d 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -42,6 +42,7 @@ describe('listenActionEvents', () => { expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), + jasmine.any(Function), jasmine.any(Function) ) }) @@ -52,6 +53,7 @@ describe('listenActionEvents', () => { expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), + jasmine.any(Function), jasmine.any(Function) ) }) @@ -100,6 +102,7 @@ describe('listenActionEvents', () => { expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'pointerup' }), + jasmine.any(Function), jasmine.any(Function) ) }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index eaaadbffab..3e633c7e8c 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -1,4 +1,5 @@ -import { addEventListener, DOM_EVENT, isExperimentalFeatureEnabled } 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 } @@ -8,7 +9,12 @@ export interface UserActivity { } export interface ActionEventsHooks { onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined - onActionStart: (context: ClickContext, startEvent: MouseEventOnElement, getUserActivity: () => UserActivity) => void + onActionStart: ( + context: ClickContext, + event: MouseEventOnElement, + getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined + ) => void } export function listenActionEvents({ onPointerDown, onActionStart }: ActionEventsHooks) { @@ -54,8 +60,24 @@ export function listenActionEvents({ onPointerDown, onActionStart if (isValidMouseEvent(startEvent) && clickContext) { // Use a scoped variable to make sure the value is not changed by other clicks const localUserActivity = userActivity - onActionStart(clickContext, startEvent, () => localUserActivity) + let clickEventTimeStamp: TimeStamp | undefined + onActionStart( + clickContext, + startEvent, + () => localUserActivity, + () => clickEventTimeStamp + ) clickContext = undefined + if (isExperimentalFeatureEnabled('click_action_on_pointerup')) { + addEventListener( + window, + DOM_EVENT.CLICK, + () => { + clickEventTimeStamp = timeStampNow() + }, + { capture: true, once: true } + ) + } } }, { capture: true } 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 e6272a919c..5d45560a9d 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -108,6 +108,7 @@ describe('trackClickActions', () => { target: undefined, position: undefined, events: [domEvent], + pointerUpDelay: undefined, }, ]) }) @@ -448,6 +449,17 @@ describe('trackClickActions', () => { 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) + }) }) }) }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index f49ea265b7..7629247005 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -51,6 +51,7 @@ export interface ClickAction { event: MouseEventOnElement frustrationTypes: FrustrationType[] events: Event[] + pointerUpDelay?: Duration } export interface ActionContexts { @@ -80,7 +81,7 @@ export function trackClickActions( const { stop: stopActionEventsListener } = listenActionEvents({ onPointerDown: (pointerDownEvent) => processPointerDown(configuration, history, pointerDownEvent), - onActionStart: (clickActionBase, startEvent, getUserActivity) => + onActionStart: (clickActionBase, startEvent, getUserActivity, getClickEventTimeStamp) => startClickAction( configuration, lifeCycle, @@ -90,7 +91,8 @@ export function trackClickActions( appendClickToClickChain, clickActionBase, startEvent, - getUserActivity + getUserActivity, + getClickEventTimeStamp ), }) @@ -154,9 +156,10 @@ function startClickAction( appendClickToClickChain: (click: Click) => void, clickActionBase: ClickActionBase, startEvent: MouseEventOnElement, - getUserActivity: () => UserActivity + getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined ) { - const click = newClick(lifeCycle, history, getUserActivity, clickActionBase, startEvent) + const click = newClick(lifeCycle, history, getUserActivity, getClickEventTimeStamp, clickActionBase, startEvent) if (configuration.trackFrustrations) { appendClickToClickChain(click) @@ -247,6 +250,7 @@ function newClick( lifeCycle: LifeCycle, history: ClickActionIdHistory, getUserActivity: () => UserActivity, + getClickEventTimeStamp: () => TimeStamp | undefined, clickActionBase: ClickActionBase, startEvent: MouseEventOnElement ) { @@ -298,7 +302,7 @@ function newClick( isStopped: () => status === ClickStatus.STOPPED || status === ClickStatus.FINALIZED, - clone: () => newClick(lifeCycle, history, getUserActivity, clickActionBase, startEvent), + 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, @@ -321,6 +326,7 @@ function newClick( }, 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 4f46c123c3..63eff357d0 100644 --- a/packages/rum-core/src/rawRumEvent.types.ts +++ b/packages/rum-core/src/rawRumEvent.types.ts @@ -179,6 +179,7 @@ export interface RawRumActionEvent { x: number y: number } + pointer_up_delay?: Duration } } } From 87ecde74132926d1c3abaa412e4eead582391e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 27 Jan 2023 14:44:15 +0100 Subject: [PATCH 6/8] =?UTF-8?q?=F0=9F=9A=A9=20rename=20feature=20flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../rumEventsCollection/action/listenActionEvents.spec.ts | 4 ++-- .../domain/rumEventsCollection/action/listenActionEvents.ts | 4 ++-- .../rumEventsCollection/action/trackClickActions.spec.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) 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 b2a163674c..d9a10d0e2c 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -85,11 +85,11 @@ describe('listenActionEvents', () => { expect(actionEventsHooks.onActionStart).not.toHaveBeenCalled() }) - describe('click_action_on_pointerup experimental feature', () => { + describe('dead_click_fixes experimental feature', () => { beforeEach(() => { stopListenEvents() - updateExperimentalFeatures(['click_action_on_pointerup']) + updateExperimentalFeatures(['dead_click_fixes']) ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index 3e633c7e8c..7873261d2e 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -55,7 +55,7 @@ export function listenActionEvents({ onPointerDown, onActionStart addEventListener( window, - isExperimentalFeatureEnabled('click_action_on_pointerup') ? DOM_EVENT.POINTER_UP : DOM_EVENT.CLICK, + 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 @@ -68,7 +68,7 @@ export function listenActionEvents({ onPointerDown, onActionStart () => clickEventTimeStamp ) clickContext = undefined - if (isExperimentalFeatureEnabled('click_action_on_pointerup')) { + if (isExperimentalFeatureEnabled('dead_click_fixes')) { addEventListener( window, DOM_EVENT.CLICK, 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 5d45560a9d..9a9cbd5238 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -431,9 +431,9 @@ describe('trackClickActions', () => { expect(events[0].frustrationTypes).toEqual([FrustrationType.DEAD_CLICK]) }) - describe('click_action_on_pointerup experimental feature', () => { + describe('dead_click_fixes experimental feature', () => { beforeEach(() => { - updateExperimentalFeatures(['click_action_on_pointerup']) + updateExperimentalFeatures(['dead_click_fixes']) }) afterEach(() => { From 3225f3d7ac32dca42a3d8d6b9ecf634c3cb07842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 27 Jan 2023 14:44:31 +0100 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=90=9B=20restore=20existing=20behavio?= =?UTF-8?q?r=20for=20dead=20clicks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/listenActionEvents.spec.ts | 11 +++++++++-- .../rumEventsCollection/action/listenActionEvents.ts | 6 +++++- 2 files changed, 14 insertions(+), 3 deletions(-) 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 d9a10d0e2c..0ac0c31af7 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts @@ -229,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)) }) @@ -249,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() { diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index 7873261d2e..cd28dc7c71 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -34,7 +34,11 @@ export function listenActionEvents({ onPointerDown, onActionStart selectionEmptyAtPointerDown = isSelectionEmpty() userActivity = { selection: false, - input: 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) } From a9e0a39de18427f5cb0ba352d4d4c6c062ad0dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Mon, 30 Jan 2023 10:11:01 +0100 Subject: [PATCH 8/8] =?UTF-8?q?=F0=9F=91=8C=20rename=20onActionStart=20to?= =?UTF-8?q?=20onStartEvent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/listenActionEvents.spec.ts | 24 +++++++++---------- .../action/listenActionEvents.ts | 6 ++--- .../action/trackClickActions.ts | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) 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 0ac0c31af7..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: { - onActionStart: jasmine.Spy['onActionStart']> + onStartEvent: jasmine.Spy['onStartEvent']> onPointerDown: jasmine.Spy['onPointerDown']> } let stopListenEvents: () => void beforeEach(() => { actionEventsHooks = { - onActionStart: jasmine.createSpy(), + onStartEvent: jasmine.createSpy(), onPointerDown: jasmine.createSpy().and.returnValue({}), } ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks)) @@ -39,7 +39,7 @@ describe('listenActionEvents', () => { it('listen to click events', () => { emulateClick() - expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), jasmine.any(Function), @@ -50,7 +50,7 @@ describe('listenActionEvents', () => { it('listen to non-primary click events', () => { // This emulates a Chrome behavior where all click events are non-primary emulateClick({ clickEventIsPrimary: false }) - expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'click' }), jasmine.any(Function), @@ -66,23 +66,23 @@ describe('listenActionEvents', () => { it('can abort click lifecycle by returning undefined from the onPointerDown callback', () => { actionEventsHooks.onPointerDown.and.returnValue(undefined) emulateClick() - expect(actionEventsHooks.onActionStart).not.toHaveBeenCalled() + expect(actionEventsHooks.onStartEvent).not.toHaveBeenCalled() }) - it('passes the context created in onPointerDown to onActionStart', () => { + it('passes the context created in onPointerDown to onStartEvent', () => { const context = {} actionEventsHooks.onPointerDown.and.returnValue(context) emulateClick() - expect(actionEventsHooks.onActionStart.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.onActionStart.calls.reset() + actionEventsHooks.onStartEvent.calls.reset() window.dispatchEvent(createNewEvent('click', { target: document.body })) - expect(actionEventsHooks.onActionStart).not.toHaveBeenCalled() + expect(actionEventsHooks.onStartEvent).not.toHaveBeenCalled() }) describe('dead_click_fixes experimental feature', () => { @@ -99,7 +99,7 @@ describe('listenActionEvents', () => { it('listen to pointerup events', () => { emulateClick() - expect(actionEventsHooks.onActionStart).toHaveBeenCalledOnceWith( + expect(actionEventsHooks.onStartEvent).toHaveBeenCalledOnceWith( {}, jasmine.objectContaining({ type: 'pointerup' }), jasmine.any(Function), @@ -177,7 +177,7 @@ describe('listenActionEvents', () => { }) function hasSelectionChanged() { - return actionEventsHooks.onActionStart.calls.mostRecent().args[2]().selection + return actionEventsHooks.onStartEvent.calls.mostRecent().args[2]().selection } function emulateNodeSelection( @@ -269,7 +269,7 @@ describe('listenActionEvents', () => { window.dispatchEvent(createNewEvent('input')) } function hasInputUserActivity() { - return actionEventsHooks.onActionStart.calls.mostRecent().args[2]().input + return actionEventsHooks.onStartEvent.calls.mostRecent().args[2]().input } }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts index cd28dc7c71..b247a7a041 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts @@ -9,7 +9,7 @@ export interface UserActivity { } export interface ActionEventsHooks { onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined - onActionStart: ( + onStartEvent: ( context: ClickContext, event: MouseEventOnElement, getUserActivity: () => UserActivity, @@ -17,7 +17,7 @@ export interface ActionEventsHooks { ) => void } -export function listenActionEvents({ onPointerDown, onActionStart }: ActionEventsHooks) { +export function listenActionEvents({ onPointerDown, onStartEvent }: ActionEventsHooks) { let selectionEmptyAtPointerDown: boolean let userActivity: UserActivity = { selection: false, @@ -65,7 +65,7 @@ export function listenActionEvents({ onPointerDown, onActionStart // Use a scoped variable to make sure the value is not changed by other clicks const localUserActivity = userActivity let clickEventTimeStamp: TimeStamp | undefined - onActionStart( + onStartEvent( clickContext, startEvent, () => localUserActivity, diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index 7629247005..abc9e37305 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -81,7 +81,7 @@ export function trackClickActions( const { stop: stopActionEventsListener } = listenActionEvents({ onPointerDown: (pointerDownEvent) => processPointerDown(configuration, history, pointerDownEvent), - onActionStart: (clickActionBase, startEvent, getUserActivity, getClickEventTimeStamp) => + onStartEvent: (clickActionBase, startEvent, getUserActivity, getClickEventTimeStamp) => startClickAction( configuration, lifeCycle,