♻️ [RUMF-1368] use the PointerDown event target for click actions (#1731

)

* ♻️ [RUMF-1368] add a time util function helper to add durations

* ✅ [RUMF-1368] tweak emulated clicks to better match reality

* dispatch `pointerdown` and `pointerup` events
* add a duration between those events

* ♻️ [RUMF-1368] extract processClick to a module function

* 🏷️♻️  [RUMF-1368] factorize a few types

* ♻️ [RUMF-1368] introduce a "onPointerDown" callback

* 🐛 [RUMF-1368] use the pointerdown target to get the selectors and name

* ✅ [RUMF-1368] add e2e test

* ✅ split e2e test and disable the unsupported part in Firefox

* ✅ fix e2e test on Safari

* 👌 remove remaining restrict-plus-operands

* 👌 fix e2e test name

* 👌 split onClick and onPointerDown processing

* 👌✅ add a test to make sure we don't reuse pointerdown context in two clicks

* 👌 light renaming

packages/core/src/tools/contextHistory.spec.ts

@@ -1,6 +1,7 @@
 import type { Clock } from '../../test/specHelper'
 import { mockClock } from '../../test/specHelper'
-import type { RelativeTime } from './timeUtils'
+import type { Duration, RelativeTime } from './timeUtils'
+import { addDuration } from './timeUtils'
 import { ONE_MINUTE } from './utils'
 import { CLEAR_OLD_CONTEXTS_INTERVAL, ContextHistory } from './contextHistory'
 
@@ -115,8 +116,7 @@ describe('contextHistory', () => {
 
   it('should clear old contexts', () => {
     const originalTime = performance.now() as RelativeTime
-    // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
-    contextHistory.add('foo', originalTime).close((originalTime + 10) as RelativeTime)
+    contextHistory.add('foo', originalTime).close(addDuration(originalTime, 10 as Duration))
     clock.tick(10)
 
     expect(contextHistory.find(originalTime)).toBeDefined()

packages/core/src/tools/timeUtils.ts

@@ -11,18 +11,16 @@ export function relativeToClocks(relative: RelativeTime) {
 }
 
 function getCorrectedTimeStamp(relativeTime: RelativeTime) {
-  const correctedOrigin = dateNow() - performance.now()
+  const correctedOrigin = (dateNow() - performance.now()) as TimeStamp
   // apply correction only for positive drift
   if (correctedOrigin > getNavigationStart()) {
-    // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
-    return Math.round(correctedOrigin + relativeTime) as TimeStamp
+    return Math.round(addDuration(correctedOrigin, relativeTime)) as TimeStamp
   }
   return getTimeStamp(relativeTime)
 }
 
 export function currentDrift() {
-  // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
-  return Math.round(dateNow() - (getNavigationStart() + performance.now()))
+  return Math.round(dateNow() - addDuration(getNavigationStart(), performance.now() as Duration))
 }
 
 export function toServerDuration(duration: Duration): ServerDuration
@@ -65,6 +63,13 @@ export function elapsed(start: number, end: number) {
   return (end - start) as Duration
 }
 
+export function addDuration(a: TimeStamp, b: Duration): TimeStamp
+export function addDuration(a: RelativeTime, b: Duration): RelativeTime
+export function addDuration(a: Duration, b: Duration): Duration
+export function addDuration(a: number, b: number) {
+  return a + b
+}
+
 /**
  * Get the time since the navigation was started.
  *
@@ -77,8 +82,7 @@ export function getRelativeTime(timestamp: TimeStamp) {
 }
 
 export function getTimeStamp(relativeTime: RelativeTime) {
-  // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
-  return Math.round(getNavigationStart() + relativeTime) as TimeStamp
+  return Math.round(addDuration(getNavigationStart(), relativeTime)) as TimeStamp
 }
 
 export function looksLikeRelativeTime(time: RelativeTime | TimeStamp): time is RelativeTime {

packages/rum-core/src/domain/contexts/foregroundContexts.ts

@@ -1,5 +1,5 @@
 import type { RelativeTime, Duration } from '@datadog/browser-core'
-import { addEventListener, DOM_EVENT, elapsed, relativeNow, toServerDuration } from '@datadog/browser-core'
+import { addDuration, addEventListener, DOM_EVENT, elapsed, relativeNow, toServerDuration } from '@datadog/browser-core'
 import type { InForegroundPeriod } from '../../rawRumEvent.types'
 
 // Arbitrary value to cap number of element mostly for backend & to save bandwidth
@@ -99,8 +99,7 @@ function isInForegroundAt(startTime: RelativeTime): boolean {
 }
 
 function selectInForegroundPeriodsFor(eventStartTime: RelativeTime, duration: Duration): InForegroundPeriod[] {
-  // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
-  const eventEndTime = (eventStartTime + duration) as RelativeTime
+  const eventEndTime = addDuration(eventStartTime, duration)
   const filteredForegroundPeriods: InForegroundPeriod[] = []
 
   const earliestIndex = Math.max(0, foregroundPeriods.length - MAX_NUMBER_OF_SELECTABLE_FOREGROUND_PERIODS)

packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts

@@ -1,27 +1,66 @@
 import type { Clock } from '../../../../../core/test/specHelper'
 import { createNewEvent, mockClock } from '../../../../../core/test/specHelper'
-import type { OnClickContext } from './listenActionEvents'
+import type { ActionEventsHooks } from './listenActionEvents'
 import { listenActionEvents } from './listenActionEvents'
 
 describe('listenActionEvents', () => {
-  let onClickSpy: jasmine.Spy<(context: OnClickContext) => void>
+  let actionEventsHooks: {
+    onClick: jasmine.Spy<ActionEventsHooks<object>['onClick']>
+    onPointerDown: jasmine.Spy<ActionEventsHooks<object>['onPointerDown']>
+  }
   let stopListenEvents: () => void
 
   beforeEach(() => {
-    onClickSpy = jasmine.createSpy()
-    ;({ stop: stopListenEvents } = listenActionEvents({ onClick: onClickSpy }))
+    actionEventsHooks = {
+      onClick: jasmine.createSpy(),
+      onPointerDown: jasmine.createSpy().and.returnValue({}),
+    }
+    ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks))
   })
 
   afterEach(() => {
     stopListenEvents()
   })
 
+  it('listen to pointerdown events', () => {
+    emulateClick()
+    expect(actionEventsHooks.onPointerDown).toHaveBeenCalledOnceWith(jasmine.objectContaining({ type: 'pointerdown' }))
+  })
+
   it('listen to click events', () => {
     emulateClick()
-    expect(onClickSpy).toHaveBeenCalledOnceWith({
-      event: jasmine.objectContaining({ type: 'click' }),
-      getUserActivity: jasmine.any(Function),
-    })
+    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()
+  })
+
+  it('can abort click lifecycle by returning undefined from the onPointerDown callback', () => {
+    actionEventsHooks.onPointerDown.and.returnValue(undefined)
+    emulateClick()
+    expect(actionEventsHooks.onClick).not.toHaveBeenCalled()
+  })
+
+  it('passes the context created in onPointerDown to onClick', () => {
+    const context = {}
+    actionEventsHooks.onPointerDown.and.returnValue(context)
+    emulateClick()
+    expect(actionEventsHooks.onClick.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()
+
+    window.dispatchEvent(createNewEvent('click', { target: document.body }))
+
+    expect(actionEventsHooks.onClick).not.toHaveBeenCalled()
   })
 
   describe('selection change', () => {
@@ -93,7 +132,7 @@ describe('listenActionEvents', () => {
     })
 
     function hasSelectionChanged() {
-      return onClickSpy.calls.mostRecent().args[0].getUserActivity().selection
+      return actionEventsHooks.onClick.calls.mostRecent().args[2]().selection
     }
 
     function emulateNodeSelection(
@@ -158,14 +197,16 @@ describe('listenActionEvents', () => {
       window.dispatchEvent(createNewEvent('input'))
     }
     function hasInputUserActivity() {
-      return onClickSpy.calls.mostRecent().args[0].getUserActivity().input
+      return actionEventsHooks.onClick.calls.mostRecent().args[2]().input
     }
   })
 
-  function emulateClick({ beforeMouseUp }: { beforeMouseUp?(): void } = {}) {
-    document.body.dispatchEvent(createNewEvent('mousedown'))
+  function emulateClick({ beforeMouseUp, target = document.body }: { beforeMouseUp?(): void; target?: Node } = {}) {
+    window.dispatchEvent(createNewEvent('pointerdown', { target }))
+    window.dispatchEvent(createNewEvent('mousedown', { target }))
     beforeMouseUp?.()
-    document.body.dispatchEvent(createNewEvent('mouseup'))
-    document.body.dispatchEvent(createNewEvent('click'))
+    window.dispatchEvent(createNewEvent('pointerup', { target }))
+    window.dispatchEvent(createNewEvent('mouseup', { target }))
+    window.dispatchEvent(createNewEvent('click', { target }))
   }
 })

packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts

@@ -1,22 +1,29 @@
 import { addEventListener, DOM_EVENT, monitor } from '@datadog/browser-core'
 
-export interface OnClickContext {
-  event: MouseEvent & { target: Element }
-  getUserActivity(): { selection: boolean; input: boolean }
+export type MouseEventOnElement = MouseEvent & { target: Element }
+
+export type GetUserActivity = () => { selection: boolean; input: boolean }
+export interface ActionEventsHooks<ClickContext> {
+  onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined
+  onClick: (context: ClickContext, event: MouseEventOnElement, getUserActivity: GetUserActivity) => void
 }
 
-export function listenActionEvents({ onClick }: { onClick(context: OnClickContext): void }) {
+export function listenActionEvents<ClickContext>({ onPointerDown, onClick }: ActionEventsHooks<ClickContext>) {
   let hasSelectionChanged = false
-  let selectionEmptyAtMouseDown: boolean
+  let selectionEmptyAtPointerDown: boolean
   let hasInputChanged = false
+  let clickContext: ClickContext | undefined
 
   const listeners = [
     addEventListener(
       window,
-      DOM_EVENT.MOUSE_DOWN,
-      () => {
+      DOM_EVENT.POINTER_DOWN,
+      (event) => {
         hasSelectionChanged = false
-        selectionEmptyAtMouseDown = isSelectionEmpty()
+        selectionEmptyAtPointerDown = isSelectionEmpty()
+        if (isMouseEventOnElement(event)) {
+          clickContext = onPointerDown(event)
+        }
       },
       { capture: true }
     ),
@@ -25,7 +32,7 @@ export function listenActionEvents({ onClick }: { onClick(context: OnClickContex
       window,
       DOM_EVENT.SELECTION_CHANGE,
       () => {
-        if (!selectionEmptyAtMouseDown || !isSelectionEmpty()) {
+        if (!selectionEmptyAtPointerDown || !isSelectionEmpty()) {
           hasSelectionChanged = true
         }
       },
@@ -36,7 +43,7 @@ export function listenActionEvents({ onClick }: { onClick(context: OnClickContex
       window,
       DOM_EVENT.CLICK,
       (clickEvent: MouseEvent) => {
-        if (clickEvent.target instanceof Element) {
+        if (isMouseEventOnElement(clickEvent) && clickContext) {
           // Use a scoped variable to make sure the value is not changed by other clicks
           const userActivity = {
             selection: hasSelectionChanged,
@@ -50,10 +57,8 @@ export function listenActionEvents({ onClick }: { onClick(context: OnClickContex
             )
           }
 
-          onClick({
-            event: clickEvent as MouseEvent & { target: Element },
-            getUserActivity: () => userActivity,
-          })
+          onClick(clickContext, clickEvent, () => userActivity)
+          clickContext = undefined
         }
       },
       { capture: true }
@@ -80,3 +85,7 @@ function isSelectionEmpty(): boolean {
   const selection = window.getSelection()
   return !selection || selection.isCollapsed
 }
+
+function isMouseEventOnElement(event: Event): event is MouseEventOnElement {
+  return event.target instanceof Element
+}