⚗🐛 [RUMF-1296] use pointerup to trigger click actions

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

@@ -87,6 +87,7 @@ describe('actionCollection', () => {
             x: 1,
             y: 2,
           },
+          pointer_up_delay: undefined,
         },
       },
     })

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,
           },
         },
       }

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<ActionEventsHooks<object>['onClick']>
+    onStartEvent: jasmine.Spy<ActionEventsHooks<object>['onStartEvent']>
     onPointerDown: jasmine.Spy<ActionEventsHooks<object>['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 }))
   }
 })

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<ClickContext> {
   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<ClickContext>({ onPointerDown, onClick }: ActionEventsHooks<ClickContext>) {
-  let hasSelectionChanged = false
+export function listenActionEvents<ClickContext>({ onPointerDown, onStartEvent }: ActionEventsHooks<ClickContext>) {
   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,32 +51,37 @@ export function listenActionEvents<ClickContext>({ onPointerDown, onClick }: Act
       DOM_EVENT.SELECTION_CHANGE,
       () => {
         if (!selectionEmptyAtPointerDown || !isSelectionEmpty()) {
-          hasSelectionChanged = true
+          userActivity.selection = true
         }
       },
       { capture: true }
     ),
 
     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<ClickContext>({ 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)
+  )
 }