🐛 [RUMF-1296][RUMF-1293] fix dead click computation

packages/core/test/specHelper.ts

@@ -329,6 +329,10 @@ class StubXhr extends StubEventEmitter {
 }
 
 export function createNewEvent<P extends Record<string, unknown>>(eventName: 'click', properties?: P): MouseEvent & P
+export function createNewEvent<P extends Record<string, unknown>>(
+  eventName: 'pointerup',
+  properties?: P
+): PointerEvent & P
 export function createNewEvent(eventName: string, properties?: { [name: string]: unknown }): Event
 export function createNewEvent(eventName: string, properties: { [name: string]: unknown } = {}) {
   let event: Event

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

@@ -26,7 +26,7 @@ describe('actionCollection', () => {
   it('should create action from auto action', () => {
     const { lifeCycle, rawRumEvents } = setupBuilder.build()
 
-    const event = createNewEvent('click', { target: document.createElement('button') })
+    const event = createNewEvent('pointerup', { target: document.createElement('button') })
     lifeCycle.notify(LifeCycleEventType.AUTO_ACTION_COMPLETED, {
       counts: {
         errorCount: 10,
@@ -87,7 +87,6 @@ describe('actionCollection', () => {
             x: 1,
             y: 2,
           },
-          pointer_up_delay: undefined,
         },
       },
     })

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

@@ -79,7 +79,6 @@ function processAction(
           action: {
             target: action.target,
             position: action.position,
-            pointer_up_delay: action.pointerUpDelay,
           },
         },
       }

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

@@ -1,4 +1,4 @@
-import { ONE_SECOND, resetExperimentalFeatures, updateExperimentalFeatures } from '@datadog/browser-core'
+import { ONE_SECOND } from '@datadog/browser-core'
 import { FrustrationType } from '../../../rawRumEvent.types'
 import type { Clock } from '../../../../../core/test/specHelper'
 import { mockClock } from '../../../../../core/test/specHelper'
@@ -137,12 +137,10 @@ describe('isDead', () => {
 
   beforeEach(() => {
     isolatedDom = createIsolatedDom()
-    updateExperimentalFeatures(['dead_click_fixes'])
   })
 
   afterEach(() => {
     isolatedDom.clear()
-    resetExperimentalFeatures()
   })
 
   it('considers as dead when the click has no page activity', () => {

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

@@ -1,4 +1,4 @@
-import { elementMatches, isExperimentalFeatureEnabled, ONE_SECOND } from '@datadog/browser-core'
+import { elementMatches, ONE_SECOND } from '@datadog/browser-core'
 import { FrustrationType } from '../../../rawRumEvent.types'
 import type { Click } from './trackClickActions'
 
@@ -53,6 +53,9 @@ const DEAD_CLICK_EXCLUDE_SELECTOR =
   'input:not([type="checkbox"]):not([type="radio"]):not([type="button"]):not([type="submit"]):not([type="reset"]):not([type="range"]),' +
   'textarea,' +
   'select,' +
+  // contenteditable and their descendants don't always trigger meaningful changes when manipulated
+  '[contenteditable],' +
+  '[contenteditable] *,' +
   // canvas, as there is no good way to detect activity occurring on them
   'canvas,' +
   // links that are interactive (have an href attribute) or any of their descendants, as they can
@@ -64,11 +67,5 @@ export function isDead(click: Click) {
   if (click.hasPageActivity || click.getUserActivity().input) {
     return false
   }
-  return !elementMatches(
-    click.event.target,
-    isExperimentalFeatureEnabled('dead_click_fixes')
-      ? // contenteditable and their descendants don't always trigger meaningful changes when manipulated
-        `${DEAD_CLICK_EXCLUDE_SELECTOR},[contenteditable],[contenteditable] *`
-      : DEAD_CLICK_EXCLUDE_SELECTOR
-  )
+  return !elementMatches(click.event.target, DEAD_CLICK_EXCLUDE_SELECTOR)
 }

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

@@ -1,6 +1,4 @@
-import { resetExperimentalFeatures, updateExperimentalFeatures } from '@datadog/browser-core'
-import type { Clock } from '../../../../../core/test/specHelper'
-import { createNewEvent, mockClock } from '../../../../../core/test/specHelper'
+import { createNewEvent } from '../../../../../core/test/specHelper'
 import type { ActionEventsHooks } from './listenActionEvents'
 import { listenActionEvents } from './listenActionEvents'
 
@@ -37,23 +35,11 @@ describe('listenActionEvents', () => {
     expect(actionEventsHooks.onPointerDown).toHaveBeenCalledTimes(1)
   })
 
-  it('listen to click events', () => {
+  it('listen to pointerup events', () => {
     emulateClick()
     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.objectContaining({ type: 'pointerup' }),
       jasmine.any(Function)
     )
   })
@@ -85,29 +71,6 @@ describe('listenActionEvents', () => {
     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', () => {
     let text: Text
 
@@ -198,16 +161,6 @@ describe('listenActionEvents', () => {
   })
 
   describe('input user activity', () => {
-    let clock: Clock
-
-    beforeEach(() => {
-      clock = mockClock()
-    })
-
-    afterEach(() => {
-      clock.cleanup()
-    })
-
     it('click that do not trigger an input input event should not report input user activity', () => {
       emulateClick()
       expect(hasInputUserActivity()).toBe(false)
@@ -225,35 +178,13 @@ describe('listenActionEvents', () => {
     it('click that triggers an input event slightly after the click should report an input user activity', () => {
       emulateClick()
       emulateInputEvent()
-      clock.tick(1) // run immediate timeouts
       expect(hasInputUserActivity()).toBe(true)
     })
 
-    describe('with dead_click_fixes flag', () => {
-      beforeEach(() => {
-        stopListenEvents()
-
-        updateExperimentalFeatures(['dead_click_fixes'])
-        ;({ stop: stopListenEvents } = listenActionEvents(actionEventsHooks))
-      })
-
-      afterEach(() => {
-        resetExperimentalFeatures()
-      })
-
-      it('input events that precede clicks should not be taken into account', () => {
-        emulateInputEvent()
-        emulateClick()
-        clock.tick(1) // run immediate timeouts
-        expect(hasInputUserActivity()).toBe(false)
-      })
-    })
-
-    it('without dead_click_fixes, input events that precede clicks should still be taken into account', () => {
+    it('input events that precede clicks should not be taken into account', () => {
       emulateInputEvent()
       emulateClick()
-      clock.tick(1) // run immediate timeouts
-      expect(hasInputUserActivity()).toBe(true)
+      expect(hasInputUserActivity()).toBe(false)
     })
 
     it('click and type should report an input user activity', () => {

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

@@ -1,20 +1,14 @@
-import type { TimeStamp } from '@datadog/browser-core'
-import { addEventListener, DOM_EVENT, isExperimentalFeatureEnabled, timeStampNow } from '@datadog/browser-core'
+import { addEventListener, DOM_EVENT } from '@datadog/browser-core'
 
-export type MouseEventOnElement = MouseEvent & { target: Element }
+export type MouseEventOnElement = PointerEvent & { target: Element }
 
 export interface UserActivity {
   selection: boolean
   input: boolean
 }
 export interface ActionEventsHooks<ClickContext> {
   onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined
-  onStartEvent: (
-    context: ClickContext,
-    event: MouseEventOnElement,
-    getUserActivity: () => UserActivity,
-    getClickEventTimeStamp: () => TimeStamp | undefined
-  ) => void
+  onStartEvent: (context: ClickContext, event: MouseEventOnElement, getUserActivity: () => UserActivity) => void
 }
 
 export function listenActionEvents<ClickContext>({ onPointerDown, onStartEvent }: ActionEventsHooks<ClickContext>) {
@@ -30,15 +24,11 @@ export function listenActionEvents<ClickContext>({ onPointerDown, onStartEvent }
       window,
       DOM_EVENT.POINTER_DOWN,
       (event: PointerEvent) => {
-        if (isValidMouseEvent(event)) {
+        if (isValidPointerEvent(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,
+            input: false,
           }
           clickContext = onPointerDown(event)
         }
@@ -59,29 +49,13 @@ export function listenActionEvents<ClickContext>({ onPointerDown, onStartEvent }
 
     addEventListener(
       window,
-      isExperimentalFeatureEnabled('dead_click_fixes') ? DOM_EVENT.POINTER_UP : DOM_EVENT.CLICK,
-      (startEvent: MouseEvent) => {
-        if (isValidMouseEvent(startEvent) && clickContext) {
+      DOM_EVENT.POINTER_UP,
+      (startEvent: PointerEvent) => {
+        if (isValidPointerEvent(startEvent) && clickContext) {
           // Use a scoped variable to make sure the value is not changed by other clicks
           const localUserActivity = userActivity
-          let clickEventTimeStamp: TimeStamp | undefined
-          onStartEvent(
-            clickContext,
-            startEvent,
-            () => localUserActivity,
-            () => clickEventTimeStamp
-          )
+          onStartEvent(clickContext, startEvent, () => localUserActivity)
           clickContext = undefined
-          if (isExperimentalFeatureEnabled('dead_click_fixes')) {
-            addEventListener(
-              window,
-              DOM_EVENT.CLICK,
-              () => {
-                clickEventTimeStamp = timeStampNow()
-              },
-              { capture: true, once: true }
-            )
-          }
         }
       },
       { capture: true }
@@ -109,14 +83,11 @@ function isSelectionEmpty(): boolean {
   return !selection || selection.isCollapsed
 }
 
-function isValidMouseEvent(event: MouseEvent): event is MouseEventOnElement {
+function isValidPointerEvent(event: PointerEvent): 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)
+    event.isPrimary !== false
   )
 }

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

@@ -87,7 +87,7 @@ describe('trackClickActions', () => {
     emulateClick({ activity: {} })
     expect(findActionId()).not.toBeUndefined()
     clock.tick(EXPIRE_DELAY)
-    const domEvent = createNewEvent('click', { target: document.createElement('button') })
+    const domEvent = createNewEvent('pointerup', { target: document.createElement('button') })
     expect(events).toEqual([
       {
         counts: {
@@ -108,7 +108,6 @@ describe('trackClickActions', () => {
         target: undefined,
         position: undefined,
         events: [domEvent],
-        pointerUpDelay: undefined,
       },
     ])
   })
@@ -431,55 +430,34 @@ describe('trackClickActions', () => {
         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 pointerdown as a dead click', () => {
-          const { clock } = setupBuilder.build()
-
-          emulateClick({ activity: { on: 'pointerdown' } })
-
-          clock.tick(EXPIRE_DELAY)
-          expect(events.length).toBe(1)
-          expect(events[0].frustrationTypes).toEqual([])
-        })
-
-        it('activity happening on pointerdown is not taken into account for the action duration', () => {
-          const { clock } = setupBuilder.build()
+      it('does not consider a click with activity happening on pointerdown as a dead click', () => {
+        const { clock } = setupBuilder.build()
 
-          emulateClick({ activity: { on: 'pointerdown' } })
+        emulateClick({ activity: { on: 'pointerdown' } })
 
-          clock.tick(EXPIRE_DELAY)
-          expect(events.length).toBe(1)
-          expect(events[0].duration).toBe(0 as Duration)
-        })
+        clock.tick(EXPIRE_DELAY)
+        expect(events.length).toBe(1)
+        expect(events[0].frustrationTypes).toEqual([])
+      })
 
-        it('does not consider a click with activity happening on pointerup as a dead click', () => {
-          const { clock } = setupBuilder.build()
+      it('activity happening on pointerdown is not taken into account for the action duration', () => {
+        const { clock } = setupBuilder.build()
 
-          emulateClick({ activity: { on: 'pointerup' } })
+        emulateClick({ activity: { on: 'pointerdown' } })
 
-          clock.tick(EXPIRE_DELAY)
-          expect(events.length).toBe(1)
-          expect(events[0].frustrationTypes).toEqual([])
-        })
+        clock.tick(EXPIRE_DELAY)
+        expect(events.length).toBe(1)
+        expect(events[0].duration).toBe(0 as Duration)
+      })
 
-        it('reports the delay between pointerup and click event', () => {
-          const { clock } = setupBuilder.build()
+      it('does not consider a click with activity happening on pointerup as a dead click', () => {
+        const { clock } = setupBuilder.build()
 
-          const pointerUpActivityDelay = 5 as Duration
-          emulateClick({ activity: { on: 'pointerup', delay: pointerUpActivityDelay } })
+        emulateClick({ activity: { on: 'pointerup' } })
 
-          clock.tick(EXPIRE_DELAY)
-          expect(events.length).toBe(1)
-          expect(events[0].pointerUpDelay).toBe(pointerUpActivityDelay)
-        })
+        clock.tick(EXPIRE_DELAY)
+        expect(events.length).toBe(1)
+        expect(events[0].frustrationTypes).toEqual([])
       })
     })
   })