From a4e0ad6257486e89b5399efa544c925d972aa087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Mon, 7 Nov 2022 17:30:25 +0100 Subject: [PATCH 1/5] =?UTF-8?q?=F0=9F=9A=9A=20[RUMF-1379]=20rename=20modul?= =?UTF-8?q?e=20getSelectorsFromElement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we won't need multiple selectors anymore, rename the module to singular 'getSelectorFromElement' --- ...ectorsFromElement.spec.ts => getSelectorFromElement.spec.ts} | 2 +- .../{getSelectorsFromElement.ts => getSelectorFromElement.ts} | 0 .../src/domain/rumEventsCollection/action/trackClickActions.ts | 2 +- packages/rum-core/src/index.ts | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename packages/rum-core/src/domain/rumEventsCollection/action/{getSelectorsFromElement.spec.ts => getSelectorFromElement.spec.ts} (99%) rename packages/rum-core/src/domain/rumEventsCollection/action/{getSelectorsFromElement.ts => getSelectorFromElement.ts} (100%) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts similarity index 99% rename from packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.spec.ts rename to packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts index 60cdf65a30..6debf58dc7 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts @@ -1,6 +1,6 @@ import type { IsolatedDom } from '../../../../test/createIsolatedDom' import { createIsolatedDom } from '../../../../test/createIsolatedDom' -import { getSelectorsFromElement, supportScopeSelector } from './getSelectorsFromElement' +import { getSelectorsFromElement, supportScopeSelector } from './getSelectorFromElement' describe('getSelectorFromElement', () => { let isolatedDom: IsolatedDom diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts similarity index 100% rename from packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.ts rename to packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index 6a38d81eae..abf75f049d 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -22,7 +22,7 @@ import { waitPageActivityEnd } from '../../waitPageActivityEnd' import type { ClickChain } from './clickChain' import { createClickChain } from './clickChain' import { getActionNameFromElement } from './getActionNameFromElement' -import { getSelectorsFromElement } from './getSelectorsFromElement' +import { getSelectorsFromElement } from './getSelectorFromElement' import type { MouseEventOnElement, GetUserActivity } from './listenActionEvents' import { listenActionEvents } from './listenActionEvents' import { computeFrustration } from './computeFrustration' diff --git a/packages/rum-core/src/index.ts b/packages/rum-core/src/index.ts index 2271384b30..84166adf07 100644 --- a/packages/rum-core/src/index.ts +++ b/packages/rum-core/src/index.ts @@ -35,4 +35,4 @@ export { getMutationObserverConstructor } from './browser/domMutationObservable' export { initViewportObservable, getViewportDimension } from './browser/viewportObservable' export { RumInitConfiguration, RumConfiguration } from './domain/configuration' export { DEFAULT_PROGRAMMATIC_ACTION_NAME_ATTRIBUTE } from './domain/rumEventsCollection/action/getActionNameFromElement' -export { STABLE_ATTRIBUTES } from './domain/rumEventsCollection/action/getSelectorsFromElement' +export { STABLE_ATTRIBUTES } from './domain/rumEventsCollection/action/getSelectorFromElement' From 21614adb84b33c0aafb5f220b9fd6b9094dadcee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Mon, 7 Nov 2022 18:13:22 +0100 Subject: [PATCH 2/5] =?UTF-8?q?=E2=9C=A8=20[RUMF-1379]=20remove=20experime?= =?UTF-8?q?ntal=20selectors=20and=20enable=20composed=20selectors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/getSelectorFromElement.spec.ts | 253 +++++++----------- .../action/getSelectorFromElement.ts | 63 ++--- .../action/trackClickActions.spec.ts | 3 - .../action/trackClickActions.ts | 14 +- 4 files changed, 117 insertions(+), 216 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts index 6debf58dc7..e7f353acd6 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts @@ -1,6 +1,6 @@ import type { IsolatedDom } from '../../../../test/createIsolatedDom' import { createIsolatedDom } from '../../../../test/createIsolatedDom' -import { getSelectorsFromElement, supportScopeSelector } from './getSelectorFromElement' +import { getSelectorFromElement, supportScopeSelector } from './getSelectorFromElement' describe('getSelectorFromElement', () => { let isolatedDom: IsolatedDom @@ -13,159 +13,73 @@ describe('getSelectorFromElement', () => { isolatedDom.clear() }) - describe('default selector', () => { - const getDefaultSelector = getSelector.bind(null, 'selector') - - describe('ID selector', () => { - it('should use the ID selector when the element as an ID', () => { - expect(getDefaultSelector('
')).toBe('#foo') - }) - - it('should not use the ID selector when the ID is not unique', () => { - expect(getDefaultSelector('
')).not.toContain('#foo') - }) - - it('should not use generated IDs', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV') - }) + describe('ID selector', () => { + it('should use the ID selector when the element as an ID', () => { + expect(getSelector('
')).toBe('#foo') }) - describe('class selector', () => { - it('should use the class selector when the element as classes', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV.foo') - }) - - it('should use the class selector when siblings have the same classes but different tags', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV.foo') - }) - - it('should not use the class selector when siblings have the tag + classes', () => { - expect(getDefaultSelector('
')).not.toContain('DIV.foo') - expect(getDefaultSelector('
')).not.toContain( - 'DIV.foo' - ) - }) - - it('should not use the class selector for body elements', () => { - const element = isolatedDom.append('
') - element.ownerDocument.body.classList.add('foo') - expect(getDefaultSelector(element)).toBe('BODY>DIV') - }) - - it('should not use generated classes', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV') - }) - - it('uses only the first class', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV.foo') - }) + it('should not use the ID selector when the ID is not unique', () => { + expect(getSelector('
')).not.toContain('#foo') }) - describe('position selector', () => { - it('should use nth-of-type when the element as siblings', () => { - expect( - getDefaultSelector(` - -
- -
- `) - ).toBe('BODY>DIV:nth-of-type(2)>BUTTON') - }) - - it('should not use nth-of-type when the element has no siblings', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV') - }) + it('should not use generated IDs', () => { + expect(getSelector('
')).toBe('BODY>DIV') }) + }) - describe('strategies priority', () => { - it('ID selector should take precedence over class selector', () => { - expect(getDefaultSelector('
')).toBe('#foo') - }) - - it('class selector should take precedence over position selector', () => { - expect(getDefaultSelector('
')).toBe('BODY>DIV.bar') - }) + describe('class selector', () => { + it('should use the class selector when the element as classes', () => { + expect(getSelector('
')).toBe('BODY>DIV.foo') }) - describe('should escape CSS selectors', () => { - it('ID selector should take precedence over class selector', () => { - expect(getDefaultSelector('
')).toBe( - '#\\#bar>BUTTON.\\.foo' - ) - }) + it('should use the class selector when siblings have the same classes but different tags', () => { + expect(getSelector('
')).toBe('BODY>DIV.foo') }) - describe('attribute selector', () => { - it('uses a stable attribute if the element has one', () => { - expect(getDefaultSelector('
')).toBe('DIV[data-testid="foo"]') - }) - - it('escapes the attribute value', () => { - expect(getDefaultSelector('
')).toBe( - 'DIV[data-testid="\\"foo\\ bar\\""]' - ) - }) - - it('attribute selector with the custom action name attribute takes precedence over other stable attribute selectors', () => { - expect(getDefaultSelector('
', 'action-name')).toBe( - 'DIV[action-name="foo"]' - ) - }) - - it('stable attribute selector should take precedence over class selector', () => { - expect(getDefaultSelector('
')).toBe('DIV[data-testid="foo"]') - }) + it('should not use the class selector when siblings have the tag + classes', () => { + expect(getSelector('
')).not.toContain('DIV.foo') + expect(getSelector('
')).not.toContain('DIV.foo') + }) - it('stable attribute selector should take precedence over ID selector', () => { - expect(getDefaultSelector('
')).toBe('DIV[data-testid="foo"]') - }) + it('should not use the class selector for body elements', () => { + const element = isolatedDom.append('
') + element.ownerDocument.body.classList.add('foo') + expect(getSelector(element)).toBe('BODY>DIV') + }) - it("uses a stable attribute selector and continue recursing if it's not unique globally", () => { - expect( - getDefaultSelector(` - + it('should not use generated classes', () => { + expect(getSelector('
')).toBe('BODY>DIV') + }) -
- -
- `) - ).toBe( - supportScopeSelector() - ? 'BODY>BUTTON[data-testid="foo"]' - : // Degraded support for browsers not supporting scoped selector: the selector is still - // correct, but its quality is a bit worse, as using a stable attribute reduce the - // chances of matching a completely unrelated element. - 'BODY>BUTTON' - ) - }) + it('uses only the first class', () => { + expect(getSelector('
')).toBe('BODY>DIV.foo') }) }) - describe('using combined selectors to check for unicity', () => { - const getCombinedSelector = getSelector.bind(null, 'selector_combined') - - it('does not use the position selector if the combined selector is matching a single element', () => { + describe('position selector', () => { + it('should use nth-of-type when the selector matches multiple descendants', () => { expect( - getCombinedSelector(` -
-
- `) - ).toBe('BODY>DIV>BUTTON') + getSelector(` + +
+ +
+ `) + ).toBe('BODY>DIV:nth-of-type(2)>BUTTON') }) - it('uses the position selector if the combined selector matching more than one element', () => { + it('should not use nth-of-type when the selector is matching a single descendant', () => { expect( - getCombinedSelector(` -
+ getSelector(` +
`) - ).toBe('BODY>DIV:nth-of-type(2)>BUTTON') + ).toBe('BODY>DIV>BUTTON') }) - it('only consider direct descendants (>) of the parent element', () => { + it('should only consider direct descendants (>) of the parent element when checking for unicity', () => { expect( - getCombinedSelector(` + getSelector(`
@@ -183,52 +97,69 @@ describe('getSelectorFromElement', () => { }) }) - describe('stopping when the selector is globally unique', () => { - const getSelectorStoppingWhenUnique = getSelector.bind(null, 'selector_stopping_when_unique') + describe('strategies priority', () => { + it('ID selector should take precedence over class selector', () => { + expect(getSelector('
')).toBe('#foo') + }) - it('stops recursing when the selector is unique', () => { - expect( - getSelectorStoppingWhenUnique(` -
-
-
-
- `) - ).toBe('MAIN>DIV:nth-of-type(2)>BUTTON') + it('class selector should take precedence over position selector', () => { + expect(getSelector('
')).toBe('BODY>DIV.bar') }) }) - describe('using combined selectors + stopping when the selector is globally unique', () => { - const getSelectorAllTogether = getSelector.bind(null, 'selector_all_together') + describe('should escape CSS selectors', () => { + it('ID selector should take precedence over class selector', () => { + expect(getSelector('
')).toBe('#\\#bar>BUTTON.\\.foo') + }) + }) + + describe('attribute selector', () => { + it('uses a stable attribute if the element has one', () => { + expect(getSelector('
')).toBe('DIV[data-testid="foo"]') + }) + + it('escapes the attribute value', () => { + expect(getSelector('
')).toBe('DIV[data-testid="\\"foo\\ bar\\""]') + }) + + it('attribute selector with the custom action name attribute takes precedence over other stable attribute selectors', () => { + expect(getSelector('
', 'action-name')).toBe( + 'DIV[action-name="foo"]' + ) + }) + + it('stable attribute selector should take precedence over class selector', () => { + expect(getSelector('
')).toBe('DIV[data-testid="foo"]') + }) - it('stops recursing when the composed selector is unique', () => { + it('stable attribute selector should take precedence over ID selector', () => { + expect(getSelector('
')).toBe('DIV[data-testid="foo"]') + }) + + it("uses a stable attribute selector and continue recursing if it's not unique globally", () => { expect( - getSelectorAllTogether(` -
-
-
-
- `) + getSelector(` + + +
+ +
+ `) ).toBe( supportScopeSelector() - ? 'MAIN>DIV>BUTTON' + ? 'BODY>BUTTON[data-testid="foo"]' : // Degraded support for browsers not supporting scoped selector: the selector is still - // correct, but its quality is a bit worse, as using a `nth-of-type` selector is a bit - // too specific and might not match if an element is conditionally inserted before the - // target. - 'MAIN>DIV:nth-of-type(2)>BUTTON' + // correct, but its quality is a bit worse, as using a stable attribute reduce the + // chances of matching a completely unrelated element. + 'BODY>BUTTON' ) }) }) - function getSelector( - selectorName: keyof ReturnType, - htmlOrElement: string | Element, - actionNameAttribute?: string - ): string { - return getSelectorsFromElement( + function getSelector(htmlOrElement: string | Element, actionNameAttribute?: string): string { + return getSelectorFromElement( typeof htmlOrElement === 'string' ? isolatedDom.append(htmlOrElement) : htmlOrElement, actionNameAttribute - )[selectorName] + ) } }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts index d947512ec5..15a4b97196 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts @@ -22,57 +22,18 @@ export const STABLE_ATTRIBUTES = [ 'data-source-file', ] -export function getSelectorsFromElement(element: Element, actionNameAttribute: string | undefined) { +export function getSelectorFromElement(targetElement: Element, actionNameAttribute: string | undefined) { let attributeSelectors = getStableAttributeSelectors() + if (actionNameAttribute) { attributeSelectors = [(element: Element) => getAttributeSelector(actionNameAttribute, element)].concat( attributeSelectors ) } + const globallyUniqueSelectorStrategies = attributeSelectors.concat(getIDSelector) const uniqueAmongChildrenSelectorStrategies = attributeSelectors.concat([getClassSelector, getTagNameSelector]) - return { - selector: getSelectorFromElement(element, globallyUniqueSelectorStrategies, uniqueAmongChildrenSelectorStrategies), - selector_combined: getSelectorFromElement( - element, - globallyUniqueSelectorStrategies, - uniqueAmongChildrenSelectorStrategies, - { useCombinedSelectors: true } - ), - selector_stopping_when_unique: getSelectorFromElement( - element, - globallyUniqueSelectorStrategies.concat([getClassSelector, getTagNameSelector]), - uniqueAmongChildrenSelectorStrategies - ), - selector_all_together: getSelectorFromElement( - element, - globallyUniqueSelectorStrategies.concat([getClassSelector, getTagNameSelector]), - uniqueAmongChildrenSelectorStrategies, - { useCombinedSelectors: true } - ), - } -} - -type GetSelector = (element: Element) => string | undefined - -function isGeneratedValue(value: string) { - // To compute the "URL path group", the backend replaces every URL path parts as a question mark - // if it thinks the part is an identifier. The condition it uses is to checks whether a digit is - // present. - // - // Here, we use the same strategy: if a the value contains a digit, we consider it generated. This - // strategy might be a bit naive and fail in some cases, but there are many fallbacks to generate - // CSS selectors so it should be fine most of the time. We might want to allow customers to - // provide their own `isGeneratedValue` at some point. - return /[0-9]/.test(value) -} -function getSelectorFromElement( - targetElement: Element, - globallyUniqueSelectorStrategies: GetSelector[], - uniqueAmongChildrenSelectorStrategies: GetSelector[], - { useCombinedSelectors = false } = {} -): string { let targetElementSelector = '' let element: Element | null = targetElement @@ -81,7 +42,7 @@ function getSelectorFromElement( element, globallyUniqueSelectorStrategies, isSelectorUniqueGlobally, - useCombinedSelectors ? targetElementSelector : undefined + targetElementSelector ) if (globallyUniqueSelector) { return combineSelector(globallyUniqueSelector, targetElementSelector) @@ -91,7 +52,7 @@ function getSelectorFromElement( element, uniqueAmongChildrenSelectorStrategies, isSelectorUniqueAmongSiblings, - useCombinedSelectors ? targetElementSelector : undefined + targetElementSelector ) targetElementSelector = combineSelector( uniqueSelectorAmongChildren || getPositionSelector(element) || getTagNameSelector(element), @@ -104,6 +65,20 @@ function getSelectorFromElement( return targetElementSelector } +type GetSelector = (element: Element) => string | undefined + +function isGeneratedValue(value: string) { + // To compute the "URL path group", the backend replaces every URL path parts as a question mark + // if it thinks the part is an identifier. The condition it uses is to checks whether a digit is + // present. + // + // Here, we use the same strategy: if a the value contains a digit, we consider it generated. This + // strategy might be a bit naive and fail in some cases, but there are many fallbacks to generate + // CSS selectors so it should be fine most of the time. We might want to allow customers to + // provide their own `isGeneratedValue` at some point. + return /[0-9]/.test(value) +} + function getIDSelector(element: Element): string | undefined { if (element.id && !isGeneratedValue(element.id)) { return `#${cssEscape(element.id)}` 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 a0ab0aeb99..1d6adcc3ee 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.spec.ts @@ -132,9 +132,6 @@ describe('trackClickActions', () => { jasmine.objectContaining({ target: { selector: '#button', - selector_combined: '#button', - selector_stopping_when_unique: '#button', - selector_all_together: '#button', width: 100, height: 100, }, diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts index abf75f049d..7b9f8ebaf5 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts @@ -22,7 +22,7 @@ import { waitPageActivityEnd } from '../../waitPageActivityEnd' import type { ClickChain } from './clickChain' import { createClickChain } from './clickChain' import { getActionNameFromElement } from './getActionNameFromElement' -import { getSelectorsFromElement } from './getSelectorFromElement' +import { getSelectorFromElement } from './getSelectorFromElement' import type { MouseEventOnElement, GetUserActivity } from './listenActionEvents' import { listenActionEvents } from './listenActionEvents' import { computeFrustration } from './computeFrustration' @@ -211,13 +211,11 @@ function computeClickActionBase(event: MouseEventOnElement, actionNameAttribute? if (isExperimentalFeatureEnabled('clickmap')) { const rect = event.target.getBoundingClientRect() - target = assign( - { - width: Math.round(rect.width), - height: Math.round(rect.height), - }, - getSelectorsFromElement(event.target, actionNameAttribute) - ) + target = { + width: Math.round(rect.width), + height: Math.round(rect.height), + selector: getSelectorFromElement(event.target, actionNameAttribute), + } position = { // Use clientX and Y because for SVG element offsetX and Y are relatives to the element x: Math.round(event.clientX - rect.left), From 4ab0b7431f621e67e73c299c9e2465a887f554bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Mon, 7 Nov 2022 18:59:23 +0100 Subject: [PATCH 3/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20[RUMF-1379]=20refactor?= =?UTF-8?q?=20selector=20getter=20lists=20to=20avoid=20re-computing=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../action/getSelectorFromElement.ts | 82 +++++++++++-------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts index 15a4b97196..3db160de19 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts @@ -22,42 +22,48 @@ export const STABLE_ATTRIBUTES = [ 'data-source-file', ] -export function getSelectorFromElement(targetElement: Element, actionNameAttribute: string | undefined) { - let attributeSelectors = getStableAttributeSelectors() - - if (actionNameAttribute) { - attributeSelectors = [(element: Element) => getAttributeSelector(actionNameAttribute, element)].concat( - attributeSelectors - ) - } - - const globallyUniqueSelectorStrategies = attributeSelectors.concat(getIDSelector) - const uniqueAmongChildrenSelectorStrategies = attributeSelectors.concat([getClassSelector, getTagNameSelector]) +type SelectorGetter = (element: Element, actionNameAttribute: string | undefined) => string | undefined + +// Selectors to use if they target a single element on the whole document. Those selectors are +// considered as "stable" and uniquely identify an element regardless of the page state. If we find +// one, we should consider the selector "complete" and stop iterating over ancestors. +const GLOBALLY_UNIQUE_SELECTOR_GETTERS: SelectorGetter[] = [getStableAttributeSelector, getIDSelector] + +// Selectors to use if they target a single element among an element descendants. Those selectors +// are more brittle than "globally unique" selectors and should be combined with ancestor selectors +// to improve specificity. +const UNIQUE_AMONG_CHILDREN_SELECTOR_GETTERS: SelectorGetter[] = [ + getStableAttributeSelector, + getClassSelector, + getTagNameSelector, +] +export function getSelectorFromElement(targetElement: Element, actionNameAttribute: string | undefined) { let targetElementSelector = '' let element: Element | null = targetElement while (element && element.nodeName !== 'HTML') { const globallyUniqueSelector = findSelector( element, - globallyUniqueSelectorStrategies, + GLOBALLY_UNIQUE_SELECTOR_GETTERS, isSelectorUniqueGlobally, + actionNameAttribute, targetElementSelector ) if (globallyUniqueSelector) { - return combineSelector(globallyUniqueSelector, targetElementSelector) + return globallyUniqueSelector } const uniqueSelectorAmongChildren = findSelector( element, - uniqueAmongChildrenSelectorStrategies, + UNIQUE_AMONG_CHILDREN_SELECTOR_GETTERS, isSelectorUniqueAmongSiblings, + actionNameAttribute, targetElementSelector ) - targetElementSelector = combineSelector( - uniqueSelectorAmongChildren || getPositionSelector(element) || getTagNameSelector(element), - targetElementSelector - ) + targetElementSelector = + uniqueSelectorAmongChildren || + combineSelector(getPositionSelector(element) || getTagNameSelector(element), targetElementSelector) element = element.parentElement } @@ -65,8 +71,6 @@ export function getSelectorFromElement(targetElement: Element, actionNameAttribu return targetElementSelector } -type GetSelector = (element: Element) => string | undefined - function isGeneratedValue(value: string) { // To compute the "URL path group", the backend replaces every URL path parts as a question mark // if it thinks the part is an identifier. The condition it uses is to checks whether a digit is @@ -105,19 +109,21 @@ function getTagNameSelector(element: Element): string { return element.tagName } -let stableAttributeSelectorsCache: GetSelector[] | undefined -function getStableAttributeSelectors() { - if (!stableAttributeSelectorsCache) { - stableAttributeSelectorsCache = STABLE_ATTRIBUTES.map( - (attribute) => (element: Element) => getAttributeSelector(attribute, element) - ) +function getStableAttributeSelector(element: Element, actionNameAttribute: string | undefined): string | undefined { + if (actionNameAttribute) { + const selector = getAttributeSelector(actionNameAttribute) + if (selector) return selector } - return stableAttributeSelectorsCache -} -function getAttributeSelector(attributeName: string, element: Element): string | undefined { - if (element.hasAttribute(attributeName)) { - return `${element.tagName}[${attributeName}="${cssEscape(element.getAttribute(attributeName)!)}"]` + for (const attributeName of STABLE_ATTRIBUTES) { + const selector = getAttributeSelector(attributeName) + if (selector) return selector + } + + function getAttributeSelector(attributeName: string) { + if (element.hasAttribute(attributeName)) { + return `${element.tagName}[${attributeName}="${cssEscape(element.getAttribute(attributeName)!)}"]` + } } } @@ -148,15 +154,19 @@ function getPositionSelector(element: Element): string | undefined { function findSelector( element: Element, - selectorGetters: GetSelector[], + selectorGetters: SelectorGetter[], predicate: (element: Element, selector: string) => boolean, + actionNameAttribute: string | undefined, childSelector?: string ) { for (const selectorGetter of selectorGetters) { - const elementSelector = selectorGetter(element) - const fullSelector = elementSelector && combineSelector(elementSelector, childSelector) - if (fullSelector && predicate(element, fullSelector)) { - return elementSelector + const elementSelector = selectorGetter(element, actionNameAttribute) + if (!elementSelector) { + continue + } + const fullSelector = combineSelector(elementSelector, childSelector) + if (predicate(element, fullSelector)) { + return fullSelector } } } From 520f269de2b07bb2293b12725a781c43a4909d77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 8 Nov 2022 14:52:59 +0100 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=91=8C=20simplify=20getPositionSelect?= =?UTF-8?q?or?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the element is the only one from its type within its siblings, the `getTagNameSelector` strategy used in UNIQUE_AMONG_CHILDREN_SELECTOR_GETTERS will succeed and `uniqueSelectorAmongChildren` will be defined. Thus,`getPositionSelector` will only be used when the tag name is not unique, so we don't need to check for tag name unicity again. This logic has been removed. Finally, we don't need to use `getTagNameSelector` once again, since `getPositionSelector` will always return a selector. This useless call has been removed. --- .../action/getSelectorFromElement.ts | 26 +++++-------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts index 3db160de19..04526688ba 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts @@ -62,8 +62,7 @@ export function getSelectorFromElement(targetElement: Element, actionNameAttribu targetElementSelector ) targetElementSelector = - uniqueSelectorAmongChildren || - combineSelector(getPositionSelector(element) || getTagNameSelector(element), targetElementSelector) + uniqueSelectorAmongChildren || combineSelector(getPositionSelector(element), targetElementSelector) element = element.parentElement } @@ -127,29 +126,18 @@ function getStableAttributeSelector(element: Element, actionNameAttribute: strin } } -function getPositionSelector(element: Element): string | undefined { - const parent = element.parentElement! - let sibling = parent.firstElementChild - let currentIndex = 0 - let elementIndex: number | undefined +function getPositionSelector(element: Element): string { + let sibling = element.parentElement!.firstElementChild + let elementIndex = 1 - while (sibling) { + while (sibling && sibling !== element) { if (sibling.tagName === element.tagName) { - currentIndex += 1 - if (sibling === element) { - elementIndex = currentIndex - } - - if (elementIndex !== undefined && currentIndex > 1) { - // Performance improvement: avoid iterating over all children, stop as soon as we are sure - // the element is not alone - break - } + elementIndex += 1 } sibling = sibling.nextElementSibling } - return currentIndex > 1 ? `${element.tagName}:nth-of-type(${elementIndex!})` : undefined + return `${element.tagName}:nth-of-type(${elementIndex})` } function findSelector( From 2ca6209bc18c53f9ded038488d0e842ffe877569 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 8 Nov 2022 17:29:40 +0100 Subject: [PATCH 5/5] =?UTF-8?q?=E2=9C=85=20fix=20unit=20test=20for=20brows?= =?UTF-8?q?ers=20without=20scope=20selector=20support?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../rumEventsCollection/action/getSelectorFromElement.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts index e7f353acd6..c087b04120 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.spec.ts @@ -151,7 +151,7 @@ describe('getSelectorFromElement', () => { : // Degraded support for browsers not supporting scoped selector: the selector is still // correct, but its quality is a bit worse, as using a stable attribute reduce the // chances of matching a completely unrelated element. - 'BODY>BUTTON' + 'BODY>BUTTON:nth-of-type(1)' ) }) })