feat(replay): Improve click target detection (#8026)

getsentry · May 8, 2023 · be129db · be129db
1 parent d8cf8d3
commit be129db
Show file tree

Hide file tree

Showing 4 changed files with 164 additions and 19 deletions.
diff --git a/packages/browser-integration-tests/suites/replay/customEvents/template.html b/packages/browser-integration-tests/suites/replay/customEvents/template.html
@@ -5,13 +5,9 @@
   </head>
   <body>
     <div role="button" id="error" class="btn btn-error" aria-label="An Error">An Error</div>
-    <button>
-      <img id="img"
-        alt="Alt Text"
-      />
-    </button>
-    <button class="sentry-unmask" aria-label="Unmasked label">
-      Unmasked
+    <button title="Button title">
+      <img id="img" alt="Alt Text" />
     </button>
+    <button class="sentry-unmask" aria-label="Unmasked label">Unmasked</button>
   </body>
 </html>
diff --git a/packages/browser-integration-tests/suites/replay/customEvents/test.ts b/packages/browser-integration-tests/suites/replay/customEvents/test.ts
@@ -135,16 +135,15 @@ sentryTest(
       expect.arrayContaining([
         {
           ...expectedClickBreadcrumb,
-          message: 'body > button > img#img[alt="Alt Text"]',
+          message: 'body > button[title="Button title"]',
           data: {
             nodeId: expect.any(Number),
             node: {
               attributes: {
-                alt: 'Alt Text',
-                id: 'img',
+                title: '****** *****',
               },
               id: expect.any(Number),
-              tagName: 'img',
+              tagName: 'button',
               textContent: '',
             },
           },

diff --git a/packages/replay/src/coreHandlers/handleDom.ts b/packages/replay/src/coreHandlers/handleDom.ts
@@ -8,7 +8,7 @@ import { createBreadcrumb } from '../util/createBreadcrumb';
 import { addBreadcrumbEvent } from './util/addBreadcrumbEvent';
 import { getAttributesToRecord } from './util/getAttributesToRecord';
 
-interface DomHandlerData {
+export interface DomHandlerData {
   name: string;
   event: Node | { target: Node };
 }
@@ -31,15 +31,18 @@ export const handleDomListener: (replay: ReplayContainer) => (handlerData: DomHa
 
 /**
  * An event handler to react to DOM events.
+ * Exported for tests only.
  */
-function handleDom(handlerData: DomHandlerData): Breadcrumb | null {
+export function handleDom(handlerData: DomHandlerData): Breadcrumb | null {
   let target;
   let targetNode: Node | INode | undefined;
 
+  const isClick = handlerData.name === 'click';
+
   // Accessing event.target can throw (see getsentry/raven-js#838, #768)
   try {
-    targetNode = getTargetNode(handlerData);
-    target = htmlTreeAsString(targetNode);
+    targetNode = isClick ? getClickTargetNode(handlerData.event) : getTargetNode(handlerData.event);
+    target = htmlTreeAsString(targetNode, { maxStringLength: 200 });
   } catch (e) {
     target = '<unknown>';
   }
@@ -73,12 +76,29 @@ function handleDom(handlerData: DomHandlerData): Breadcrumb | null {
   });
 }
 
-function getTargetNode(handlerData: DomHandlerData): Node {
-  if (isEventWithTarget(handlerData.event)) {
-    return handlerData.event.target;
+function getTargetNode(event: DomHandlerData['event']): Node {
+  if (isEventWithTarget(event)) {
+    return event.target;
+  }
+
+  return event;
+}
+
+const INTERACTIVE_SELECTOR = 'button,a';
+
+// For clicks, we check if the target is inside of a button or link
+// If so, we use this as the target instead
+// This is useful because if you click on the image in <button><img></button>,
+// The target will be the image, not the button, which we don't want here
+function getClickTargetNode(event: DomHandlerData['event']): Node {
+  const target = getTargetNode(event);
+
+  if (!target || !(target instanceof Element)) {
+    return target;
   }
 
-  return handlerData.event;
+  const closestInteractive = target.closest(INTERACTIVE_SELECTOR);
+  return closestInteractive || target;
 }
 
 function isEventWithTarget(event: unknown): event is { target: Node } {

diff --git a/packages/replay/test/unit/coreHandlers/handleDom.test.ts b/packages/replay/test/unit/coreHandlers/handleDom.test.ts
@@ -0,0 +1,130 @@
+import type { DomHandlerData } from '../../../src/coreHandlers/handleDom';
+import { handleDom } from '../../../src/coreHandlers/handleDom';
+
+describe('Unit | coreHandlers | handleDom', () => {
+  test('it works with a basic click event on a div', () => {
+    const parent = document.createElement('body');
+    const target = document.createElement('div');
+    target.classList.add('my-class', 'other-class');
+    parent.appendChild(target);
+
+    const handlerData: DomHandlerData = {
+      name: 'click',
+      event: {
+        target,
+      },
+    };
+    const actual = handleDom(handlerData);
+    expect(actual).toEqual({
+      category: 'ui.click',
+      data: {},
+      message: 'body > div.my-class.other-class',
+      timestamp: expect.any(Number),
+      type: 'default',
+    });
+  });
+
+  test('it works with a basic click event on a button', () => {
+    const parent = document.createElement('body');
+    const target = document.createElement('button');
+    target.classList.add('my-class', 'other-class');
+    parent.appendChild(target);
+
+    const handlerData: DomHandlerData = {
+      name: 'click',
+      event: {
+        target,
+      },
+    };
+    const actual = handleDom(handlerData);
+    expect(actual).toEqual({
+      category: 'ui.click',
+      data: {},
+      message: 'body > button.my-class.other-class',
+      timestamp: expect.any(Number),
+      type: 'default',
+    });
+  });
+
+  test('it works with a basic click event on a span inside of <button>', () => {
+    const parent = document.createElement('body');
+    const interactive = document.createElement('button');
+    interactive.classList.add('my-class', 'other-class');
+    parent.appendChild(interactive);
+
+    const target = document.createElement('span');
+    interactive.appendChild(target);
+
+    const handlerData: DomHandlerData = {
+      name: 'click',
+      event: {
+        target,
+      },
+    };
+    const actual = handleDom(handlerData);
+    expect(actual).toEqual({
+      category: 'ui.click',
+      data: {},
+      message: 'body > button.my-class.other-class',
+      timestamp: expect.any(Number),
+      type: 'default',
+    });
+  });
+
+  test('it works with a basic click event on a span inside of <a>', () => {
+    const parent = document.createElement('body');
+    const interactive = document.createElement('a');
+    interactive.classList.add('my-class', 'other-class');
+    parent.appendChild(interactive);
+
+    const target = document.createElement('span');
+    interactive.appendChild(target);
+
+    const handlerData: DomHandlerData = {
+      name: 'click',
+      event: {
+        target,
+      },
+    };
+    const actual = handleDom(handlerData);
+    expect(actual).toEqual({
+      category: 'ui.click',
+      data: {},
+      message: 'body > a.my-class.other-class',
+      timestamp: expect.any(Number),
+      type: 'default',
+    });
+  });
+
+  test('it works with very deep nesting', () => {
+    const parent = document.createElement('body');
+
+    let current: HTMLElement = parent;
+    for (let i = 0; i < 20; i++) {
+      const next = document.createElement('div');
+      next.classList.add(`level-${i}`, `level-other-${i}`);
+      current.appendChild(next);
+      current = next;
+    }
+
+    const target = document.createElement('div');
+    target.classList.add('my-class', 'other-class');
+    current.appendChild(target);
+
+    const handlerData: DomHandlerData = {
+      name: 'click',
+      event: {
+        target,
+      },
+    };
+    const actual = handleDom(handlerData);
+    expect(actual).toEqual({
+      category: 'ui.click',
+      data: {},
+      message:
+        'div.level-16.level-other-16 > div.level-17.level-other-17 > div.level-18.level-other-18 > div.level-19.level-other-19 > div.my-class.other-class',
+      timestamp: expect.any(Number),
+      type: 'default',
+    });
+  });
+});