⚗️✨ [RUMF-1379] heatmaps: enable descendant combined selectors

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

@@ -0,0 +1,165 @@
+import type { IsolatedDom } from '../../../../test/createIsolatedDom'
+import { createIsolatedDom } from '../../../../test/createIsolatedDom'
+import { getSelectorFromElement, supportScopeSelector } from './getSelectorFromElement'
+
+describe('getSelectorFromElement', () => {
+  let isolatedDom: IsolatedDom
+
+  beforeEach(() => {
+    isolatedDom = createIsolatedDom()
+  })
+
+  afterEach(() => {
+    isolatedDom.clear()
+  })
+
+  describe('ID selector', () => {
+    it('should use the ID selector when the element as an ID', () => {
+      expect(getSelector('<div id="foo"></div>')).toBe('#foo')
+    })
+
+    it('should not use the ID selector when the ID is not unique', () => {
+      expect(getSelector('<div id="foo"></div><div id="foo"></div>')).not.toContain('#foo')
+    })
+
+    it('should not use generated IDs', () => {
+      expect(getSelector('<div id="foo4"></div>')).toBe('BODY>DIV')
+    })
+  })
+
+  describe('class selector', () => {
+    it('should use the class selector when the element as classes', () => {
+      expect(getSelector('<div class="foo"></div>')).toBe('BODY>DIV.foo')
+    })
+
+    it('should use the class selector when siblings have the same classes but different tags', () => {
+      expect(getSelector('<div target class="foo"></div><span class="foo"></span>')).toBe('BODY>DIV.foo')
+    })
+
+    it('should not use the class selector when siblings have the tag + classes', () => {
+      expect(getSelector('<div target class="foo"></div><div class="foo"></div>')).not.toContain('DIV.foo')
+      expect(getSelector('<div target class="foo bar"></div><div class="bar foo baz"></div>')).not.toContain('DIV.foo')
+    })
+
+    it('should not use the class selector for body elements', () => {
+      const element = isolatedDom.append('<div></div>')
+      element.ownerDocument.body.classList.add('foo')
+      expect(getSelector(element)).toBe('BODY>DIV')
+    })
+
+    it('should not use generated classes', () => {
+      expect(getSelector('<div class="foo4"></div>')).toBe('BODY>DIV')
+    })
+
+    it('uses only the first class', () => {
+      expect(getSelector('<div class="foo bar baz baa"></div>')).toBe('BODY>DIV.foo')
+    })
+  })
+
+  describe('position selector', () => {
+    it('should use nth-of-type when the selector matches multiple descendants', () => {
+      expect(
+        getSelector(`
+            <span></span>
+            <div><button></button></div>
+            <span></span>
+            <div><button target></button></div>
+          `)
+      ).toBe('BODY>DIV:nth-of-type(2)>BUTTON')
+    })
+
+    it('should not use nth-of-type when the selector is matching a single descendant', () => {
+      expect(
+        getSelector(`
+          <div></div>
+          <div><button target></button></div>
+        `)
+      ).toBe('BODY>DIV>BUTTON')
+    })
+
+    it('should only consider direct descendants (>) of the parent element when checking for unicity', () => {
+      expect(
+        getSelector(`
+          <main>
+            <div><div><button></button></div></div>
+            <div><button target></button></div>
+          </main>
+        `)
+      ).toBe(
+        supportScopeSelector()
+          ? 'BODY>MAIN>DIV>BUTTON'
+          : // 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.
+            'BODY>MAIN>DIV:nth-of-type(2)>BUTTON'
+      )
+    })
+  })
+
+  describe('strategies priority', () => {
+    it('ID selector should take precedence over class selector', () => {
+      expect(getSelector('<div id="foo" class="bar"></div>')).toBe('#foo')
+    })
+
+    it('class selector should take precedence over position selector', () => {
+      expect(getSelector('<div class="bar"></div><div></div>')).toBe('BODY>DIV.bar')
+    })
+  })
+
+  describe('should escape CSS selectors', () => {
+    it('ID selector should take precedence over class selector', () => {
+      expect(getSelector('<div id="#bar"><button target class=".foo"></button></div>')).toBe('#\\#bar>BUTTON.\\.foo')
+    })
+  })
+
+  describe('attribute selector', () => {
+    it('uses a stable attribute if the element has one', () => {
+      expect(getSelector('<div data-testid="foo"></div>')).toBe('DIV[data-testid="foo"]')
+    })
+
+    it('escapes the attribute value', () => {
+      expect(getSelector('<div data-testid="&quot;foo bar&quot;"></div>')).toBe('DIV[data-testid="\\"foo\\ bar\\""]')
+    })
+
+    it('attribute selector with the custom action name attribute takes precedence over other stable attribute selectors', () => {
+      expect(getSelector('<div action-name="foo" data-testid="bar"></div>', 'action-name')).toBe(
+        'DIV[action-name="foo"]'
+      )
+    })
+
+    it('stable attribute selector should take precedence over class selector', () => {
+      expect(getSelector('<div class="foo" data-testid="foo"></div>')).toBe('DIV[data-testid="foo"]')
+    })
+
+    it('stable attribute selector should take precedence over ID selector', () => {
+      expect(getSelector('<div id="foo" data-testid="foo"></div>')).toBe('DIV[data-testid="foo"]')
+    })
+
+    it("uses a stable attribute selector and continue recursing if it's not unique globally", () => {
+      expect(
+        getSelector(`
+            <button target data-testid="foo"></button>
+
+            <div>
+              <button data-testid="foo"></button>
+            </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:nth-of-type(1)'
+      )
+    })
+  })
+
+  function getSelector(htmlOrElement: string | Element, actionNameAttribute?: string): string {
+    return getSelectorFromElement(
+      typeof htmlOrElement === 'string' ? isolatedDom.append(htmlOrElement) : htmlOrElement,
+      actionNameAttribute
+    )
+  }
+})

packages/rum-core/src/domain/rumEventsCollection/action/getSelectorsFromElement.ts → packages/rum-core/src/domain/rumEventsCollection/action/getSelectorFromElement.ts

@@ -22,88 +22,66 @@ export const STABLE_ATTRIBUTES = [
   'data-source-file',
 ]
 
-export function getSelectorsFromElement(element: 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)
-}
+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,
+]
 
-function getSelectorFromElement(
-  targetElement: Element,
-  globallyUniqueSelectorStrategies: GetSelector[],
-  uniqueAmongChildrenSelectorStrategies: GetSelector[],
-  { useCombinedSelectors = false } = {}
-): string {
+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,
-      useCombinedSelectors ? targetElementSelector : undefined
+      actionNameAttribute,
+      targetElementSelector
     )
     if (globallyUniqueSelector) {
-      return combineSelector(globallyUniqueSelector, targetElementSelector)
+      return globallyUniqueSelector
     }
 
     const uniqueSelectorAmongChildren = findSelector(
       element,
-      uniqueAmongChildrenSelectorStrategies,
+      UNIQUE_AMONG_CHILDREN_SELECTOR_GETTERS,
       isSelectorUniqueAmongSiblings,
-      useCombinedSelectors ? targetElementSelector : undefined
-    )
-    targetElementSelector = combineSelector(
-      uniqueSelectorAmongChildren || getPositionSelector(element) || getTagNameSelector(element),
+      actionNameAttribute,
       targetElementSelector
     )
+    targetElementSelector =
+      uniqueSelectorAmongChildren || combineSelector(getPositionSelector(element), targetElementSelector)
 
     element = element.parentElement
   }
 
   return targetElementSelector
 }
 
+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)}`
@@ -130,58 +108,53 @@ 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)!)}"]`
+    }
   }
 }
 
-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(
   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
     }
   }
 }