Add text content, clean up innerText and textContent, make Button Clicks more reliable (#1151)

silesky · web-flow · commit 571386f5d388 · 2024-09-19T16:15:24.000-05:00
diff --git a/.changeset/weak-mice-judge.md b/.changeset/weak-mice-judge.md
@@ -0,0 +1,7 @@
+---
+'@segment/analytics-signals': patch
+---
+
+- Clean up up innerText AND textContent artifacts to make easier to parse.
+- Add textContent field
+- Make button Clicks more reliable 
diff --git a/packages/signals/signals-example/src/components/ComplexForm.tsx b/packages/signals/signals-example/src/components/ComplexForm.tsx
@@ -57,6 +57,11 @@ const ComplexForm = () => {
         </div>
         <button type="submit">Submit</button>
       </form>
+      <button>
+        <div>
+          Other Example Button with <h1>Nested Text</h1>
+        </div>
+      </button>
     </div>
   )
 }
diff --git a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/basic.test.ts b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/basic.test.ts
@@ -123,7 +123,6 @@ test('interaction signals', async () => {
       labels: [],
       name: '',
       nodeName: 'BUTTON',
-      nodeValue: null,
       tagName: 'BUTTON',
       title: '',
       type: 'submit',
diff --git a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/button-click-complex.test.ts b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/button-click-complex.test.ts
@@ -0,0 +1,59 @@
+import { test, expect } from '@playwright/test'
+import type { SegmentEvent } from '@segment/analytics-next'
+import { IndexPage } from './index-page'
+
+const indexPage = new IndexPage()
+
+const basicEdgeFn = `
+    // this is a process signal function
+    const processSignal = (signal) => {}`
+
+test.beforeEach(async ({ page }) => {
+  await indexPage.loadAndWait(page, basicEdgeFn)
+})
+
+test('button click (complex, with nested items)', async () => {
+  /**
+   * Click a button with nested text, ensure that that correct text shows up
+   */
+  await Promise.all([
+    indexPage.clickComplexButton(),
+    indexPage.waitForSignalsApiFlush(),
+  ])
+
+  const signalsReqJSON = indexPage.lastSignalsApiReq.postDataJSON()
+  const interactionSignals = signalsReqJSON.batch.filter(
+    (el: SegmentEvent) => el.properties!.type === 'interaction'
+  )
+  expect(interactionSignals).toHaveLength(1)
+  const data = {
+    eventType: 'click',
+    target: {
+      attributes: {
+        id: 'complex-button',
+      },
+      classList: [],
+      id: 'complex-button',
+      labels: [],
+      name: '',
+      nodeName: 'BUTTON',
+      tagName: 'BUTTON',
+      title: '',
+      type: 'submit',
+      innerText: expect.any(String),
+      textContent: expect.stringContaining(
+        'Other Example Button with Nested Text'
+      ),
+      value: '',
+    },
+  }
+
+  expect(interactionSignals[0]).toMatchObject({
+    event: 'Segment Signal Generated',
+    type: 'track',
+    properties: {
+      type: 'interaction',
+      data,
+    },
+  })
+})
diff --git a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/index-page.ts b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/index-page.ts
@@ -36,4 +36,8 @@ export class IndexPage extends BasePage {
   async clickButton() {
     return this.page.click('#some-button')
   }
+
+  async clickComplexButton() {
+    return this.page.click('#complex-button')
+  }
 }
diff --git a/packages/signals/signals-integration-tests/src/tests/signals-vanilla/index.html b/packages/signals/signals-integration-tests/src/tests/signals-vanilla/index.html
@@ -9,6 +9,13 @@
 
 <body>
   <button id="some-button">Click me</button>
+  <button id="complex-button">
+    <img id="some-image" src="https://via.placeholder.com/150" alt="Placeholder Image">
+    <div>
+      Other Example Button with <h1>Nested Text</h1>
+    </div>
+  </button>
+
   <form>
     <label for="name">Name:</label>
     <input type="text" id="name" name="name"><br><br>
@@ -19,5 +26,4 @@
     <input type="submit" value="Submit">
   </form>
 </body>
-
 </html>
diff --git a/packages/signals/signals/src/core/signal-generators/__tests__/dom-gen-helpers.test.ts b/packages/signals/signals/src/core/signal-generators/__tests__/dom-gen-helpers.test.ts
@@ -0,0 +1,51 @@
+import { cleanText } from '../dom-gen'
+
+describe(cleanText, () => {
+  test('should remove newline characters', () => {
+    const input = 'Hello\nWorld\n'
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should remove tab characters', () => {
+    const input = 'Hello\tWorld\t'
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should replace multiple spaces with a single space', () => {
+    const input = 'Hello    World'
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should replace non-breaking spaces with regular spaces', () => {
+    const input = 'Hello\u00A0World'
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should trim leading and trailing spaces', () => {
+    const input = '   Hello World   '
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should handle a combination of special characters', () => {
+    const input = ' \n\tHello\u00A0   World\n\t '
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should return an empty string if input is empty', () => {
+    const input = ''
+    const expected = ''
+    expect(cleanText(input)).toBe(expected)
+  })
+
+  test('should return the same string if there are no special characters', () => {
+    const input = 'Hello World'
+    const expected = 'Hello World'
+    expect(cleanText(input)).toBe(expected)
+  })
+})
diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen.ts
@@ -23,6 +23,14 @@ const parseNodeMap = (nodeMap: NamedNodeMap): Record<string, unknown> => {
   }, {} as Record<string, unknown>)
 }
 
+export const cleanText = (str: string): string => {
+  return str
+    .replace(/[\r\n\t]+/g, ' ') // Replace newlines and tabs with a space
+    .replace(/\s\s+/g, ' ') // Replace multiple spaces with a single space
+    .replace(/\u00A0/g, ' ') // Replace non-breaking spaces with a regular space
+    .trim() // Trim leading and trailing spaces
+}
+
 const parseElement = (el: HTMLElement) => {
   const base = {
     // adding a bunch of fields that are not on _all_ elements, but are on enough that it's useful to have them here.
@@ -32,11 +40,12 @@ const parseElement = (el: HTMLElement) => {
     labels: parseLabels((el as HTMLInputElement).labels),
     name: (el as HTMLInputElement).name,
     nodeName: el.nodeName,
-    nodeValue: el.nodeValue,
     tagName: el.tagName,
     title: el.title,
     type: (el as HTMLInputElement).type,
     value: (el as HTMLInputElement).value,
+    textContent: el.textContent && cleanText(el.textContent),
+    innerText: el.innerText && cleanText(el.innerText),
   }
 
   if (el instanceof HTMLSelectElement) {
@@ -67,11 +76,6 @@ const parseElement = (el: HTMLElement) => {
       src: el.src,
       volume: el.volume,
     }
-  } else if (el instanceof HTMLButtonElement) {
-    return {
-      ...base,
-      innerText: el.innerText,
-    }
   }
   return base
 }
@@ -81,12 +85,14 @@ export class ClickSignalsGenerator implements SignalGenerator {
 
   register(emitter: SignalEmitter) {
     const handleClick = (ev: MouseEvent) => {
-      const target = (ev.target as HTMLElement) ?? {}
-      if (this.isClickableElement(target)) {
+      const target = ev.target as HTMLElement | null
+      if (!target) return
+      const el = this.getClosestClickableElement(target)
+      if (el) {
         emitter.emit(
           createInteractionSignal({
             eventType: 'click',
-            target: parseElement(target),
+            target: parseElement(el),
           })
         )
       }
@@ -95,12 +101,9 @@ export class ClickSignalsGenerator implements SignalGenerator {
     return () => document.removeEventListener('click', handleClick)
   }
 
-  private isClickableElement(el: HTMLElement): boolean {
-    return (
-      el instanceof HTMLAnchorElement ||
-      el instanceof HTMLButtonElement ||
-      ['button', 'link'].includes(el.getAttribute('role') ?? '')
-    )
+  private getClosestClickableElement(el: HTMLElement): HTMLElement | null {
+    // if you click on a nested element, we want to get the closest clickable ancestor. Useful for things like buttons with nested text or images
+    return el.closest<HTMLElement>('button, a, [role="button"], [role="link"]')
   }
 }
 
diff --git a/packages/signals/signals/src/types/signals.ts b/packages/signals/signals/src/types/signals.ts
@@ -18,7 +18,6 @@ export type InteractionData = ClickData | SubmitData | ChangeData
 interface SerializedTarget {
   // nodeName: Node['nodeName']
   // textContent: Node['textContent']
-  // nodeValue: Node['nodeValue']
   // nodeType: Node['nodeType']
   [key: string]: any
 }

Original file line number	Diff line number	Diff line change
`@@ -57,6 +57,11 @@ const ComplexForm = () => {`
`57`	`57`	`</div>`
`58`	`58`	`<button type="submit">Submit</button>`
`59`	`59`	`</form>`
	`60`	`+ <button>`
	`61`	`+ <div>`
	`62`	`+ Other Example Button with <h1>Nested Text</h1>`
	`63`	`+ </div>`
	`64`	`+ </button>`
`60`	`65`	`</div>`
`61`	`66`	`)`
`62`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,4 +36,8 @@ export class IndexPage extends BasePage {`
`36`	`36`	`async clickButton() {`
`37`	`37`	`return this.page.click('#some-button')`
`38`	`38`	`}`
	`39`	`+`
	`40`	`+ async clickComplexButton() {`
	`41`	`+ return this.page.click('#complex-button')`
	`42`	`+ }`
`39`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,6 @@ export type InteractionData = ClickData \| SubmitData \| ChangeData`
`18`	`18`	`interface SerializedTarget {`
`19`	`19`	`// nodeName: Node['nodeName']`
`20`	`20`	`// textContent: Node['textContent']`
`21`		`- // nodeValue: Node['nodeValue']`
`22`	`21`	`// nodeType: Node['nodeType']`
`23`	`22`	`[key: string]: any`
`24`	`23`	`}`