Skip to content

Commit

Permalink
πŸš©πŸ› enable fix #1958
Browse files Browse the repository at this point in the history
  • Loading branch information
BenoitZugmeyer committed Feb 6, 2023
1 parent 7accf89 commit 2dfd9be
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 61 deletions.
4 changes: 4 additions & 0 deletions packages/core/test/specHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
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'
Expand Down Expand Up @@ -37,22 +36,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.objectContaining({ type: 'pointerup' }),
jasmine.any(Function),
jasmine.any(Function)
)
Expand Down Expand Up @@ -85,29 +73,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

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
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 MouseEventOnElement = PointerEvent & { target: Element }

export interface UserActivity {
selection: boolean
Expand Down Expand Up @@ -30,7 +30,7 @@ export function listenActionEvents<ClickContext>({ onPointerDown, onStartEvent }
window,
DOM_EVENT.POINTER_DOWN,
(event: PointerEvent) => {
if (isValidMouseEvent(event)) {
if (isValidPointerEvent(event)) {
selectionEmptyAtPointerDown = isSelectionEmpty()
userActivity = {
selection: false,
Expand All @@ -55,9 +55,9 @@ 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
Expand Down Expand Up @@ -105,14 +105,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
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -460,16 +460,6 @@ describe('trackClickActions', () => {
expect(events[0].duration).toBe(0 as Duration)
})

it('does not consider a click with activity happening on pointerup as a dead click', () => {
const { clock } = setupBuilder.build()

emulateClick({ activity: { on: 'pointerup' } })

clock.tick(EXPIRE_DELAY)
expect(events.length).toBe(1)
expect(events[0].frustrationTypes).toEqual([])
})

it('reports the delay between pointerup and click event', () => {
const { clock } = setupBuilder.build()

Expand All @@ -481,6 +471,16 @@ describe('trackClickActions', () => {
expect(events[0].pointerUpDelay).toBe(pointerUpActivityDelay)
})
})

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([])
})
})
})

Expand Down
4 changes: 2 additions & 2 deletions packages/rum-core/test/createFakeClick.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export function createFakeClick({
hasError?: boolean
hasPageActivity?: boolean
userActivity?: { selection?: boolean; input?: boolean }
event?: Partial<MouseEvent & { target: Element }>
event?: Partial<PointerEvent & { target: Element }>
} = {}) {
const stopObservable = new Observable<void>()
let isStopped = false
Expand Down Expand Up @@ -42,7 +42,7 @@ export function createFakeClick({
addFrustration: jasmine.createSpy<Click['addFrustration']>(),
clone: jasmine.createSpy<typeof clone>().and.callFake(clone),

event: createNewEvent('click', {
event: createNewEvent('pointerup', {
clientX: 100,
clientY: 100,
timeStamp: timeStampNow(),
Expand Down

0 comments on commit 2dfd9be

Please sign in to comment.