Skip to content

Commit

Permalink
Merge branch 'najib.boutaib/RUM-96-ignore-frustrations-on-clicks-lead…
Browse files Browse the repository at this point in the history
…ing-to-scrolls' (71226c9) into staging-13

 pm_trace_id: 30729242
 feature_branch_pipeline_id: 30729242
 source: to-staging

* commit '71226c955f868db09302f3f996f31fb8cce123cd':
  ✅ [RUM-96] Add more unit tests
  ✅ [RUM-96] Add e2e tests
  ✅ [RUM-96] Add unit tests
  🐛 [RUM-96] Ignore frustrations on clicks resulting in scrolls
  • Loading branch information
N-Boutaib committed Mar 25, 2024
2 parents eb11386 + 71226c9 commit 36b238b
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 4 deletions.
17 changes: 17 additions & 0 deletions packages/rum-core/src/domain/action/computeFrustration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ describe('computeFrustration', () => {
expect(getFrustrations(clicks[1])).toEqual([])
})

it('does not add a dead frustration when clicking to scroll', () => {
clicks[0] = createFakeClick({ userActivity: { scroll: true } })
clicks[1] = createFakeClick()
computeFrustration(clicks, rageClick)
expect(getFrustrations(clicks[1])).toEqual([])
})

it('adds an error frustration to clicks that have an error', () => {
clicks[1] = createFakeClick({ hasError: true })
computeFrustration(clicks, rageClick)
Expand Down Expand Up @@ -101,6 +108,12 @@ describe('isRage', () => {
)
})

it('does not consider rage when at least one click is related to a "scroll" event', () => {
expect(isRage([createFakeClick(), createFakeClick({ userActivity: { scroll: true } }), createFakeClick()])).toBe(
false
)
})

it('does not consider as rage two clicks happening at the same time', () => {
expect(isRage([createFakeClick(), createFakeClick()])).toBe(false)
})
Expand Down Expand Up @@ -143,6 +156,10 @@ describe('isDead', () => {
expect(isDead(createFakeClick({ hasPageActivity: false, userActivity: { input: true } }))).toBe(false)
})

it('does not consider as dead when the click is related to a "scroll" event', () => {
expect(isDead(createFakeClick({ hasPageActivity: false, userActivity: { scroll: true } }))).toBe(false)
})

for (const { element, expected } of [
{ element: '<input />', expected: false },
{ element: '<textarea />', expected: false },
Expand Down
9 changes: 6 additions & 3 deletions packages/rum-core/src/domain/action/computeFrustration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@ export function computeFrustration(clicks: Click[], rageClick: Click) {
}

const hasSelectionChanged = clicks.some((click) => click.getUserActivity().selection)
const didScrollOccur = clicks.some((click) => click.getUserActivity().scroll)
clicks.forEach((click) => {
if (click.hasError) {
click.addFrustration(FrustrationType.ERROR_CLICK)
}
if (
isDead(click) &&
// Avoid considering clicks part of a double-click or triple-click selections as dead clicks
!hasSelectionChanged
!hasSelectionChanged &&
// Avoid considering clicks that resulted in a scroll as dead clicks
!didScrollOccur
) {
click.addFrustration(FrustrationType.DEAD_CLICK)
}
Expand All @@ -34,7 +37,7 @@ export function computeFrustration(clicks: Click[], rageClick: Click) {
}

export function isRage(clicks: Click[]) {
if (clicks.some((click) => click.getUserActivity().selection)) {
if (clicks.some((click) => click.getUserActivity().selection || click.getUserActivity().scroll)) {
return false
}
for (let i = 0; i < clicks.length - (MIN_CLICKS_PER_SECOND_TO_CONSIDER_RAGE - 1); i += 1) {
Expand Down Expand Up @@ -65,7 +68,7 @@ const DEAD_CLICK_EXCLUDE_SELECTOR =
'a[href] *'

export function isDead(click: Click) {
if (click.hasPageActivity || click.getUserActivity().input) {
if (click.hasPageActivity || click.getUserActivity().input || click.getUserActivity().scroll) {
return false
}
return !elementMatches(click.event.target, DEAD_CLICK_EXCLUDE_SELECTOR)
Expand Down
35 changes: 35 additions & 0 deletions packages/rum-core/src/domain/action/listenActionEvents.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,41 @@ describe('listenActionEvents', () => {
}
})

describe('scroll', () => {
it('click that do not trigger a scroll event should not report scroll user activity', () => {
emulateClick()
expect(hasScrollUserActivity()).toBe(false)
})

it('click that triggers a scroll event during the click should report a scroll user activity', () => {
emulateClick({
beforeMouseUp() {
emulateScrollEvent()
},
})
expect(hasScrollUserActivity()).toBe(true)
})

it('click that triggers a scroll event slightly after the click should report a scroll user activity', () => {
emulateClick()
emulateScrollEvent()
expect(hasScrollUserActivity()).toBe(true)
})

it('scroll events that precede clicks should not be taken into account', () => {
emulateScrollEvent()
emulateClick()
expect(hasScrollUserActivity()).toBe(false)
})

function emulateScrollEvent() {
window.dispatchEvent(createNewEvent('scroll'))
}
function hasScrollUserActivity() {
return actionEventsHooks.onPointerUp.calls.mostRecent().args[2]().scroll
}
})

function emulateClick({
beforeMouseUp,
target = document.body,
Expand Down
13 changes: 13 additions & 0 deletions packages/rum-core/src/domain/action/listenActionEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export type MouseEventOnElement = PointerEvent & { target: Element }
export interface UserActivity {
selection: boolean
input: boolean
scroll: boolean
}
export interface ActionEventsHooks<ClickContext> {
onPointerDown: (event: MouseEventOnElement) => ClickContext | undefined
Expand All @@ -20,6 +21,7 @@ export function listenActionEvents<ClickContext>(
let userActivity: UserActivity = {
selection: false,
input: false,
scroll: false,
}
let clickContext: ClickContext | undefined

Expand All @@ -34,6 +36,7 @@ export function listenActionEvents<ClickContext>(
userActivity = {
selection: false,
input: false,
scroll: false,
}
clickContext = onPointerDown(event)
}
Expand All @@ -53,6 +56,16 @@ export function listenActionEvents<ClickContext>(
{ capture: true }
),

addEventListener(
configuration,
window,
DOM_EVENT.SCROLL,
() => {
userActivity.scroll = true
},
{ capture: true, passive: true }
),

addEventListener(
configuration,
window,
Expand Down
3 changes: 2 additions & 1 deletion packages/rum-core/test/createFakeClick.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export function createFakeClick({
}: {
hasError?: boolean
hasPageActivity?: boolean
userActivity?: { selection?: boolean; input?: boolean }
userActivity?: { selection?: boolean; input?: boolean; scroll?: boolean }
event?: Partial<PointerEvent & { target: Element }>
} = {}) {
const stopObservable = new Observable<void>()
Expand All @@ -37,6 +37,7 @@ export function createFakeClick({
getUserActivity: () => ({
selection: false,
input: false,
scroll: false,
...userActivity,
}),
addFrustration: jasmine.createSpy<Click['addFrustration']>(),
Expand Down
48 changes: 48 additions & 0 deletions test/e2e/scenario/rum/actions.scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,54 @@ describe('action collection', () => {
expect(actionEvents[0].action.frustration!.type).toEqual([])
})

createTest('do not consider clicks leading to scrolls as "dead_click"')
.withRum({ trackUserInteractions: true })
.withBody(html`
<div style="height: 2500px;">
<button>click me</button>
<script>
const button = document.querySelector('button')
button.addEventListener('click', () => {
window.scrollTo(0, 2000)
})
</script>
</div>
`)
.run(async ({ intakeRegistry }) => {
const button = await $('button')
await button.click()

await flushEvents()
const actionEvents = intakeRegistry.rumActionEvents

expect(actionEvents.length).toBe(1)
expect(actionEvents[0].action.frustration!.type).toEqual([])
})

createTest('do not consider clicks leading to scrolls as "rage_click"')
.withRum({ trackUserInteractions: true })
.withBody(html`
<div style="height: 2500px;">
<button>click me</button>
<script>
const button = document.querySelector('button')
button.addEventListener('click', () => {
window.scrollTo(0, 2000)
})
</script>
</div>
`)
.run(async ({ intakeRegistry }) => {
const button = await $('button')
await Promise.all([button.click(), button.click(), button.click()])

await flushEvents()
const actionEvents = intakeRegistry.rumActionEvents

expect(actionEvents.length).toBe(3)
expect(actionEvents[0].action.frustration!.type).toEqual([])
})

createTest('collect a "rage click"')
.withRum({ trackUserInteractions: true })
.withBody(html`
Expand Down

0 comments on commit 36b238b

Please sign in to comment.