From ed158914c18403f557042f4f8dadc3bce639e48c Mon Sep 17 00:00:00 2001 From: Gitlab staging reset job Date: Wed, 25 May 2022 11:05:20 +0200 Subject: [PATCH 01/12] Compute css selector path --- packages/core/src/tools/utils.ts | 8 ++ .../action/getSelectorFromElement.spec.ts | 78 +++++++++++++++++++ .../action/getSelectorFromElement.ts | 47 +++++++++++ 3 files changed, 133 insertions(+) create mode 100644 packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts create mode 100644 packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts diff --git a/packages/core/src/tools/utils.ts b/packages/core/src/tools/utils.ts index 31273a9b6d..c59b17cd02 100644 --- a/packages/core/src/tools/utils.ts +++ b/packages/core/src/tools/utils.ts @@ -208,6 +208,14 @@ export function includes(candidate: string | unknown[], search: any) { return candidate.indexOf(search) !== -1 } +export function arrayFrom(arrayLike: ArrayLike): T[] { + const array = [] + for (let i = 0; i < arrayLike.length; i++) { + array.push(arrayLike[i]) + } + return array +} + export function find( array: T[], predicate: (item: T, index: number, array: T[]) => item is S diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts new file mode 100644 index 0000000000..338f2104b7 --- /dev/null +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts @@ -0,0 +1,78 @@ +import { getSelectorFromElement } from './getSelectorFromElement' + +describe('getSelectorFromElement', () => { + const iframes: HTMLIFrameElement[] = [] + + function element(s: TemplateStringsArray) { + // Simply using a DOMParser does not fit here, because script tags created this way are + // considered as normal markup, so they are not ignored when getting the textual content of the + // target via innerText + + const iframe = document.createElement('iframe') + iframes.push(iframe) + document.body.appendChild(iframe) + const doc = iframe.contentDocument! + doc.open() + doc.write(`${s[0]}`) + doc.close() + return doc.querySelector('[target]') || doc.body.children[0] + } + + afterEach(() => { + iframes.forEach((iframe) => iframe.parentNode!.removeChild(iframe)) + iframes.length = 0 + }) + + describe('ID selector', () => { + it('should use the ID selector when the element as an ID', () => { + expect(getSelectorFromElement(element`
`)).toBe('#foo') + }) + + it('should not use the ID selector when the ID is not unique', () => { + expect(getSelectorFromElement(element`
`)).not.toContain('#foo') + }) + }) + + describe('class selector', () => { + it('should use the class selector when the element as classes', () => { + expect(getSelectorFromElement(element`
`)).toBe('BODY>DIV.bar.foo') + }) + + it('should use the class selector when siblings have the same classes but different tags', () => { + expect(getSelectorFromElement(element`
`)).toBe( + 'BODY>DIV.foo' + ) + }) + + it('should not use the class selector when siblings have the tag + classes', () => { + expect(getSelectorFromElement(element`
`)).not.toContain( + 'DIV.foo' + ) + expect( + getSelectorFromElement(element`
`) + ).not.toContain('DIV.foo') + }) + }) + + describe('position selector', () => { + it('should use nth-of-type when the element as siblings', () => { + const html = element`
` + expect(getSelectorFromElement(html)).toBe('BODY>DIV:nth-of-type(2)') + }) + + it('should not use nth-of-type when the element has no siblings', () => { + const html = element`
` + expect(getSelectorFromElement(html)).toBe('BODY>DIV') + }) + }) + + describe('strategies priority', () => { + it('ID selector should take precedence over class selector', () => { + expect(getSelectorFromElement(element`
`)).toBe('#foo') + }) + + it('class selector should take precedence over position selector', () => { + expect(getSelectorFromElement(element`
`)).toBe('BODY>DIV.bar') + }) + }) +}) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts new file mode 100644 index 0000000000..eeda21cef8 --- /dev/null +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts @@ -0,0 +1,47 @@ +import { arrayFrom } from '@datadog/browser-core' + +export function getSelectorFromElement(targetElement: Element): string { + const targetElementSelector = [] + let element: Element | null = targetElement + + while (element && element.nodeName !== 'HTML') { + const idSelector = getIDSelector(element) + if (idSelector) { + targetElementSelector.unshift(idSelector) + break + } + + targetElementSelector.unshift(getClassSelector(element) || getPositionSelector(element)) + + element = element.parentElement + } + + return targetElementSelector.join('>') +} + +function getIDSelector(element: Element): string | undefined { + const isUnique = element.id && element.ownerDocument.body.querySelectorAll(`#${element.id}`).length === 1 + + if (isUnique) return `#${element.id}` +} + +function getClassSelector(element: Element): string | undefined { + const orderedClassList = arrayFrom(element.classList).sort() + const siblings = arrayFrom(element.parentElement!.children).filter((child) => child !== element) + const classUniqueAmongSiblings = siblings.every( + (sibling) => + sibling.tagName !== element.tagName || + orderedClassList.some((className) => !sibling.classList.contains(className)) + ) + + if (classUniqueAmongSiblings) + return `${element.tagName}${orderedClassList.map((className) => `.${className}`).join('')}` +} + +function getPositionSelector(element: Element): string | undefined { + const isUniqueChild = !element.previousElementSibling && !element.nextElementSibling + if (isUniqueChild) return `${element.tagName}` + + const index = arrayFrom(element.parentElement!.children).indexOf(element) + return `${element.tagName}:nth-of-type(${index + 1})` +} From d5db08b8805536cf6f89a08dad1b20815b162628 Mon Sep 17 00:00:00 2001 From: Gitlab staging reset job Date: Wed, 25 May 2022 11:06:08 +0200 Subject: [PATCH 02/12] Collect click position --- packages/core/test/specHelper.ts | 5 ++- .../action/actionCollection.spec.ts | 15 ++++++- .../action/actionCollection.ts | 2 + .../action/trackClickActions.spec.ts | 43 +++++++++++++++++-- .../action/trackClickActions.ts | 32 ++++++++++++-- packages/rum-core/src/rawRumEvent.types.ts | 7 +++ packages/rum-core/src/rumEvent.types.ts | 26 +++++++++++ rum-events-format | 2 +- test/e2e/scenario/rum/actions.scenario.ts | 9 +++- 9 files changed, 130 insertions(+), 11 deletions(-) diff --git a/packages/core/test/specHelper.ts b/packages/core/test/specHelper.ts index 35c933dc84..d8de03339a 100644 --- a/packages/core/test/specHelper.ts +++ b/packages/core/test/specHelper.ts @@ -311,7 +311,10 @@ class StubXhr extends StubEventEmitter { } } -export function createNewEvent(eventName: 'click', properties?: { [name: string]: unknown }): MouseEvent +export function createNewEvent( + eventName: 'click', + properties?: { [name: string]: unknown } +): MouseEvent & { target: HTMLElement } export function createNewEvent(eventName: string, properties?: { [name: string]: unknown }): Event export function createNewEvent(eventName: string, properties: { [name: string]: unknown } = {}) { let event: Event diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts index 73d43e4068..716764f86e 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.spec.ts @@ -25,7 +25,7 @@ describe('actionCollection', () => { }) it('should create action from auto action', () => { const { lifeCycle, rawRumEvents } = setupBuilder.build() - const event = createNewEvent('click') + const event = createNewEvent('click', { target: { offsetWidth: 1, offsetHeight: 2 } }) lifeCycle.notify(LifeCycleEventType.AUTO_ACTION_COMPLETED, { counts: { errorCount: 10, @@ -39,6 +39,12 @@ describe('actionCollection', () => { startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp }, type: ActionType.CLICK, event, + target: { + selector: '#foo', + width: 1, + height: 2, + }, + position: { x: 1, y: 2 }, }) expect(rawRumEvents[0].startTime).toBe(1234 as RelativeTime) @@ -60,6 +66,13 @@ describe('actionCollection', () => { }, target: { name: 'foo', + selector: '#foo', + width: 1, + height: 2, + }, + position: { + x: 1, + y: 2, }, type: ActionType.CLICK, }, diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts index 189f81bb03..84881ae768 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts @@ -60,6 +60,8 @@ function processAction( ? { action: { id: action.id, + target: action.target, + position: action.position, loading_time: toServerDuration(action.duration), frustration: { type: action.frustrationTypes, diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts index 893b7f0430..40195afc52 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -1,5 +1,11 @@ import type { Context, Observable, Duration } from '@datadog/browser-core' -import { clocksNow, timeStampNow, relativeNow } from '@datadog/browser-core' +import { + updateExperimentalFeatures, + resetExperimentalFeatures, + clocksNow, + timeStampNow, + relativeNow, +} from '@datadog/browser-core' import type { Clock } from '../../../../../core/test/specHelper' import { createNewEvent } from '../../../../../core/test/specHelper' import type { TestSetupBuilder } from '../../../../test/specHelper' @@ -44,6 +50,9 @@ describe('trackClickActions', () => { beforeEach(() => { button = document.createElement('button') button.type = 'button' + button.id = 'button' + button.style.width = '100px' + button.style.height = '100px' button.appendChild(document.createTextNode('Click me')) document.body.appendChild(button) @@ -90,10 +99,34 @@ describe('trackClickActions', () => { type: ActionType.CLICK, event: createNewEvent('click'), frustrationTypes: [], + target: undefined, + position: undefined, }, ]) }) + describe('when clickmap ff is enabled', () => { + beforeEach(() => { + updateExperimentalFeatures(['clickmap']) + }) + + afterEach(() => { + resetExperimentalFeatures() + }) + + it('should set click position and target', () => { + const { domMutationObservable, clock } = setupBuilder.build() + emulateClickWithActivity(domMutationObservable, clock) + clock.tick(EXPIRE_DELAY) + expect(events[0]).toEqual( + jasmine.objectContaining({ + target: { selector: '#button', width: 100, height: 100 }, + position: { x: 50, y: 50 }, + }) + ) + }) + }) + it('should keep track of previously validated click actions', () => { const { domMutationObservable, clock } = setupBuilder.build() const clickActionStartTime = relativeNow() @@ -374,11 +407,15 @@ describe('trackClickActions', () => { function emulateClickWithoutActivity(target: HTMLElement = button) { const targetPosition = target.getBoundingClientRect() + const offsetX = targetPosition.width / 2 + const offsetY = targetPosition.height / 2 target.dispatchEvent( createNewEvent('click', { target, - clientX: targetPosition.left + targetPosition.width / 2, - clientY: targetPosition.top + targetPosition.height / 2, + clientX: targetPosition.left + offsetX, + clientY: targetPosition.top + offsetY, + offsetX, + offsetY, timeStamp: timeStampNow(), }) ) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index d5ddeb2d2f..dca60a947c 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -1,5 +1,6 @@ import type { Duration, ClocksState, RelativeTime, TimeStamp } from '@datadog/browser-core' import { + isExperimentalFeatureEnabled, setToArray, Observable, assign, @@ -22,6 +23,7 @@ import { waitPageActivityEnd } from '../../waitPageActivityEnd' import type { RageClickChain } from './rageClickChain' import { createRageClickChain } from './rageClickChain' import { getActionNameFromElement } from './getActionNameFromElement' +import { getSelectorFromElement } from './getSelectorFromElement' interface ActionCounts { errorCount: number @@ -33,10 +35,16 @@ export interface ClickAction { type: ActionType.CLICK id: string name: string + target?: { + selector: string + width: number + height: number + } + position?: { x: number; y: number } startClocks: ClocksState duration?: Duration counts: ActionCounts - event: MouseEvent + event: MouseEvent & { target: HTMLElement } frustrationTypes: FrustrationType[] } @@ -89,7 +97,7 @@ export function trackClickActions( } } - function processClick(event: MouseEvent & { target: Element }) { + function processClick(event: MouseEvent & { target: HTMLElement }) { if (!trackFrustrations && history.find()) { // TODO: remove this in a future major version. To keep retrocompatibility, ignore any new // action if another one is already occurring. @@ -159,13 +167,13 @@ export function trackClickActions( } } -function listenClickEvents(callback: (clickEvent: MouseEvent & { target: Element }) => void) { +function listenClickEvents(callback: (clickEvent: MouseEvent & { target: HTMLElement }) => void) { return addEventListener( window, DOM_EVENT.CLICK, (clickEvent: MouseEvent) => { if (clickEvent.target instanceof Element) { - callback(clickEvent as MouseEvent & { target: Element }) + callback(clickEvent as MouseEvent & { target: HTMLElement }) } }, { capture: true } @@ -195,6 +203,20 @@ function newClick( base: Pick ) { const id = generateUUID() + let target: ClickAction['target'] + let position: ClickAction['position'] + + if (isExperimentalFeatureEnabled('clickmap')) { + target = { + selector: getSelectorFromElement(base.event.target), + width: base.event.target.offsetWidth, + height: base.event.target.offsetHeight, + } + position = { + x: base.event.offsetX, + y: base.event.offsetY, + } + } const historyEntry = history.add(id, base.startClocks.relative) const eventCountsSubscription = trackEventCounts(lifeCycle) let state: ClickState = { status: ClickStatus.ONGOING } @@ -250,6 +272,8 @@ function newClick( duration: state.endTime && elapsed(base.startClocks.timeStamp, state.endTime), id, frustrationTypes: setToArray(frustrations), + target, + position, counts: { resourceCount, errorCount, diff --git a/packages/rum-core/src/rawRumEvent.types.ts b/packages/rum-core/src/rawRumEvent.types.ts index 679eef1117..f1f3c3d96d 100644 --- a/packages/rum-core/src/rawRumEvent.types.ts +++ b/packages/rum-core/src/rawRumEvent.types.ts @@ -149,6 +149,13 @@ export interface RawRumActionEvent { resource?: Count target: { name: string + selector?: string + width?: number + height?: number + } + position?: { + x: number + y: number } } view?: { diff --git a/packages/rum-core/src/rumEvent.types.ts b/packages/rum-core/src/rumEvent.types.ts index b12c9080d3..4d6347ca1d 100644 --- a/packages/rum-core/src/rumEvent.types.ts +++ b/packages/rum-core/src/rumEvent.types.ts @@ -39,6 +39,18 @@ export type RumActionEvent = CommonProperties & { * Target name */ name: string + /** + * CSS selector path of the target element + */ + readonly selector?: string + /** + * Width of the target element (in pixels) + */ + readonly width?: number + /** + * Height of the target element (in pixels) + */ + readonly height?: number [k: string]: unknown } /** @@ -51,6 +63,20 @@ export type RumActionEvent = CommonProperties & { readonly type: ('rage_click' | 'dead_click' | 'error_click')[] [k: string]: unknown } + /** + * Action position properties + */ + readonly position?: { + /** + * X coordinate of the action (in pixels) + */ + readonly x: number + /** + * Y coordinate of the action (in pixels) + */ + readonly y: number + [k: string]: unknown + } /** * Properties of the errors of the action */ diff --git a/rum-events-format b/rum-events-format index d09b33de6b..58f68dcaac 160000 --- a/rum-events-format +++ b/rum-events-format @@ -1 +1 @@ -Subproject commit d09b33de6b9e445968c4bfe1440304e6cd839faa +Subproject commit 58f68dcaacbde82a7d72ac35fd00e56e6b6d278e diff --git a/test/e2e/scenario/rum/actions.scenario.ts b/test/e2e/scenario/rum/actions.scenario.ts index cd8341f872..ddb4e7ca41 100644 --- a/test/e2e/scenario/rum/actions.scenario.ts +++ b/test/e2e/scenario/rum/actions.scenario.ts @@ -3,7 +3,7 @@ import { createTest, flushEvents, html, waitForServersIdle } from '../../lib/fra describe('action collection', () => { createTest('track a click action') - .withRum({ trackInteractions: true }) + .withRum({ trackInteractions: true, enableExperimentalFeatures: ['clickmap'] }) .withBody( html` @@ -36,6 +36,13 @@ describe('action collection', () => { }, target: { name: 'click me', + selector: jasmine.any(String), + width: jasmine.any(Number), + height: jasmine.any(Number), + }, + position: { + x: jasmine.any(Number), + y: jasmine.any(Number), }, type: 'click', frustration: { From 618b5bc49d32186832995fa990480de1389f5613 Mon Sep 17 00:00:00 2001 From: Gitlab staging reset job Date: Mon, 30 May 2022 10:40:21 +0200 Subject: [PATCH 03/12] =?UTF-8?q?=F0=9F=91=8C=20Set=20element=20in=20spec?= =?UTF-8?q?=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/getActionNameFromElement.spec.ts | 100 ++++++++---------- .../action/getSelectorFromElement.spec.ts | 49 ++++----- packages/rum-core/test/createIsolatedDOM.ts | 21 ++++ rum-events-format | 2 +- 4 files changed, 88 insertions(+), 84 deletions(-) create mode 100644 packages/rum-core/test/createIsolatedDOM.ts diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getActionNameFromElement.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getActionNameFromElement.spec.ts index 8cb7c4609a..fe048c4d00 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getActionNameFromElement.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getActionNameFromElement.spec.ts @@ -1,65 +1,54 @@ +import { createIsolatedDOM } from '../../../../test/createIsolatedDom' import { getActionNameFromElement } from './getActionNameFromElement' describe('getActionNameFromElement', () => { - const iframes: HTMLIFrameElement[] = [] - - function element(s: TemplateStringsArray) { - // Simply using a DOMParser does not fit here, because script tags created this way are - // considered as normal markup, so they are not ignored when getting the textual content of the - // target via innerText - - const iframe = document.createElement('iframe') - iframes.push(iframe) - document.body.appendChild(iframe) - const doc = iframe.contentDocument! - doc.open() - doc.write(`${s[0]}`) - doc.close() - return doc.querySelector('[target]') || doc.body.children[0] - } + let isolatedDOM: ReturnType + + beforeEach(() => { + isolatedDOM = createIsolatedDOM() + }) afterEach(() => { - iframes.forEach((iframe) => iframe.parentNode!.removeChild(iframe)) - iframes.length = 0 + isolatedDOM.clear() }) it('extracts the textual content of an element', () => { - expect(getActionNameFromElement(element`
Foo
bar
`)).toBe('Foo bar') + expect(getActionNameFromElement(isolatedDOM.element`
Foo
bar
`)).toBe('Foo bar') }) it('extracts the text of an input button', () => { - expect(getActionNameFromElement(element``)).toBe('Click') + expect(getActionNameFromElement(isolatedDOM.element``)).toBe('Click') }) it('extracts the alt text of an image', () => { - expect(getActionNameFromElement(element`bar`)).toBe('bar') + expect(getActionNameFromElement(isolatedDOM.element`bar`)).toBe('bar') }) it('extracts the title text of an image', () => { - expect(getActionNameFromElement(element``)).toBe('foo') + expect(getActionNameFromElement(isolatedDOM.element``)).toBe('foo') }) it('extracts the text of an aria-label attribute', () => { - expect(getActionNameFromElement(element``)).toBe('Foo') + expect(getActionNameFromElement(isolatedDOM.element``)).toBe('Foo') }) it('gets the parent element textual content if everything else fails', () => { - expect(getActionNameFromElement(element`
Foo
`)).toBe('Foo') + expect(getActionNameFromElement(isolatedDOM.element`
Foo
`)).toBe('Foo') }) it("doesn't get the value of a text input", () => { - expect(getActionNameFromElement(element``)).toBe('') + expect(getActionNameFromElement(isolatedDOM.element``)).toBe('') }) it("doesn't get the value of a password input", () => { - expect(getActionNameFromElement(element``)).toBe('') + expect(getActionNameFromElement(isolatedDOM.element``)).toBe('') }) it('limits the name length to a reasonable size', () => { expect( getActionNameFromElement( // eslint-disable-next-line max-len - element`
Foooooooooooooooooo baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaz
` + isolatedDOM.element`
Foooooooooooooooooo baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaz
` ) ).toBe( // eslint-disable-next-line max-len @@ -68,20 +57,20 @@ describe('getActionNameFromElement', () => { }) it('normalize white spaces', () => { - expect(getActionNameFromElement(element`
foo\tbar\n\n baz
`)).toBe('foo bar baz') + expect(getActionNameFromElement(isolatedDOM.element`
foo\tbar\n\n baz
`)).toBe('foo bar baz') }) it('ignores the inline script textual content', () => { - expect(getActionNameFromElement(element`
b
`)).toBe('b') + expect(getActionNameFromElement(isolatedDOM.element`
b
`)).toBe('b') }) it('extracts text from SVG elements', () => { - expect(getActionNameFromElement(element`foo bar`)).toBe('foo bar') + expect(getActionNameFromElement(isolatedDOM.element`foo bar`)).toBe('foo bar') }) it('extracts text from an associated label', () => { expect( - getActionNameFromElement(element` + getActionNameFromElement(isolatedDOM.element`
ignored
@@ -93,7 +82,7 @@ describe('getActionNameFromElement', () => { it('extracts text from a parent label', () => { expect( - getActionNameFromElement(element` + getActionNameFromElement(isolatedDOM.element`