fix(REACH-519, REACH-527): Fix embed buttons (#590)

Prevent default action when embed button is clicked to avoid unintentional submit of parent form. Close buttons are now <button> and not <a> to improve SEO.
Typeform · Jun 22, 2023 · 1018632 · 1018632
1 parent 8ebbaeb
commit 1018632
Show file tree

Hide file tree

Showing 16 changed files with 73 additions and 35 deletions.
diff --git a/packages/demo-html/public/popup-html.html b/packages/demo-html/public/popup-html.html
@@ -20,20 +20,22 @@
     </style>
   </head>
   <body>
-    <div class="element">I have z-index 10k</div>
-    <button
-      id="button"
-      data-tf-popup="HLjqXS5W"
-      data-tf-medium="demo-test"
-      data-tf-hidden="foo=foo value,bar=bar value"
-    >
-      toggle default (large)
-    </button>
-    <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-size="80">toggle medium</button>
-    <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-size="45">toggle small</button>
-    <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-width="380" data-tf-height="300">
-      toggle custom 380 x 300 pixels
-    </button>
-    <script src="./lib/embed.js"></script>
+    <form action="/404">
+      <div class="element">I have z-index 10k</div>
+      <button
+        id="button"
+        data-tf-popup="HLjqXS5W"
+        data-tf-medium="demo-test"
+        data-tf-hidden="foo=foo value,bar=bar value"
+      >
+        toggle default (large)
+      </button>
+      <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-size="80">toggle medium</button>
+      <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-size="45">toggle small</button>
+      <button data-tf-popup="HLjqXS5W" data-tf-medium="demo-test" data-tf-width="380" data-tf-height="300">
+        toggle custom 380 x 300 pixels
+      </button>
+      <script src="./lib/embed.js"></script>
+    </form>
   </body>
 </html>
diff --git a/packages/embed/e2e/spec/functional/callbacks.cy.ts b/packages/embed/e2e/spec/functional/callbacks.cy.ts
@@ -13,7 +13,7 @@ describe('Callbacks', () => {
         expect(stub).to.be.calledWith('onReady')
       })
 
-    cy.get('a.tf-v1-close')
+    cy.get('button.tf-v1-close')
       .click()
       .then(() => {
         expect(stub).to.be.calledWith('onClose')

diff --git a/packages/embed/e2e/spec/functional/keep-session.cy.ts b/packages/embed/e2e/spec/functional/keep-session.cy.ts
@@ -1,7 +1,7 @@
 describe('Keep session', () => {
   const scenarios = [
-    { name: 'popup', openSelector: '#popup', closeSelector: 'a.tf-v1-close' },
-    { name: 'slider', openSelector: '#slider', closeSelector: 'a.tf-v1-close' },
+    { name: 'popup', openSelector: '#popup', closeSelector: 'button.tf-v1-close' },
+    { name: 'slider', openSelector: '#slider', closeSelector: 'button.tf-v1-close' },
     { name: 'popover', openSelector: 'button.tf-v1-sidetab-button', closeSelector: 'button.tf-v1-sidetab-button' },
     { name: 'sidetab', openSelector: 'button.tf-v1-popover-button', closeSelector: 'button.tf-v1-popover-button' },
   ]

diff --git a/packages/embed/e2e/spec/functional/popup.cy.ts b/packages/embed/e2e/spec/functional/popup.cy.ts
@@ -31,7 +31,7 @@ function testPopup(path: string, title: string) {
     })
 
     it('should close popup', () => {
-      cy.get('a.tf-v1-close').click()
+      cy.get('button.tf-v1-close').click()
       cy.get('.tf-v1-popup').should('not.exist')
     })
   })

diff --git a/packages/embed/e2e/spec/functional/slider.cy.ts b/packages/embed/e2e/spec/functional/slider.cy.ts
@@ -27,7 +27,7 @@ function testSlider(path: string, title: string) {
     })
 
     it('should close slider', () => {
-      cy.get('a.tf-v1-close').click()
+      cy.get('button.tf-v1-close').click()
       cy.get('.tf-v1-slider').should('not.exist')
     })
   })

diff --git a/packages/embed/src/css/includes/_close.scss b/packages/embed/src/css/includes/_close.scss
@@ -14,6 +14,8 @@
   color: #000;
   top: $top;
   right: $right;
+  background: none;
+  border: none;
 
   &:hover {
     opacity: 1;

diff --git a/packages/embed/src/factories/create-popover/create-popover.ts b/packages/embed/src/factories/create-popover/create-popover.ts
@@ -9,6 +9,7 @@ import {
   isOpen,
   isInPage,
   makeAutoResize,
+  invokeWithoutDefault,
 } from '../../utils'
 import type { RemoveHandler } from '../../utils'
 import { EmbedPopup } from '../../base'
@@ -33,7 +34,7 @@ const buildPopover = (width?: number, height?: number) => {
   return setElementSize(popover, { width, height })
 }
 
-const buildCloseIcon = (element = 'div', className = 'tf-v1-popover-button-icon') => {
+const buildCloseIcon = (element = 'span', className = 'tf-v1-popover-button-icon') => {
   const icon = document.createElement(element)
   icon.className = `${className} tf-v1-close-icon`
   icon.innerHTML = '&times;'
@@ -124,7 +125,7 @@ export const createPopover = (formId: string, userOptions: PopoverOptions = {}):
   const icon = buildIcon(options.customIcon, options.buttonColor || defaultOptions.buttonColor)
   const spinner = buildSpinner()
   const closeIcon = buildCloseIcon()
-  const closeModal = buildCloseIcon('a', 'tf-v1-popover-close')
+  const closeModal = buildCloseIcon('button', 'tf-v1-popover-close')
   const button = buildTriggerButton(options.buttonColor || defaultOptions.buttonColor)
 
   const container = options.container || document.body
@@ -237,8 +238,8 @@ export const createPopover = (formId: string, userOptions: PopoverOptions = {}):
     }
   }
 
-  button.onclick = toggle
-  closeModal.onclick = close
+  button.onclick = invokeWithoutDefault(toggle)
+  closeModal.onclick = invokeWithoutDefault(close)
 
   if (options.open && !isOpen(wrapper)) {
     openHandler = handleCustomOpen(open, options.open, options.openValue)

diff --git a/packages/embed/src/factories/create-popup/create-popup.ts b/packages/embed/src/factories/create-popup/create-popup.ts
@@ -7,6 +7,7 @@ import {
   unmountElement,
   setAutoClose,
   addCustomKeyboardListener,
+  invokeWithoutDefault,
 } from '../../utils'
 import type { RemoveHandler } from '../../utils'
 import { POPUP_SIZE } from '../../constants'
@@ -47,10 +48,10 @@ const buildWrapper = (width?: number, height?: number, size?: number) => {
 }
 
 const buildCloseButton = (close: () => void) => {
-  const closeButton = document.createElement('a')
+  const closeButton = document.createElement('button')
   closeButton.className = 'tf-v1-close tf-v1-close-icon'
   closeButton.innerHTML = '&times;'
-  closeButton.onclick = close
+  closeButton.onclick = invokeWithoutDefault(close)
   return closeButton
 }
 

diff --git a/packages/embed/src/factories/create-sidetab/create-sidetab.ts b/packages/embed/src/factories/create-sidetab/create-sidetab.ts
@@ -9,6 +9,7 @@ import {
   isOpen,
   isInPage,
   makeAutoResize,
+  invokeWithoutDefault,
 } from '../../utils'
 import type { RemoveHandler } from '../../utils'
 import { EmbedPopup } from '../../base'
@@ -106,7 +107,7 @@ export const createSidetab = (formId: string, userOptions: SidetabOptions = {}):
   const buttonText = buildTriggerButtonText(options.buttonText || defaultOptions.buttonText)
   const icon = buildIcon(options.customIcon, options.buttonColor || defaultOptions.buttonColor)
   const closeIcon = buildCloseIcon()
-  const closeModal = buildCloseIcon('a', 'tf-v1-sidetab-close')
+  const closeModal = buildCloseIcon('button', 'tf-v1-sidetab-close')
 
   const container = options.container || document.body
   let openHandler: RemoveHandler
@@ -166,8 +167,8 @@ export const createSidetab = (formId: string, userOptions: SidetabOptions = {}):
     isOpen(wrapper) ? close() : open()
   }
 
-  button.onclick = toggle
-  closeModal.onclick = close
+  button.onclick = invokeWithoutDefault(toggle)
+  closeModal.onclick = invokeWithoutDefault(close)
 
   if (options.open && !isOpen(wrapper)) {
     openHandler = handleCustomOpen(open, options.open, options.openValue)

diff --git a/packages/embed/src/factories/create-slider/create-slider.ts b/packages/embed/src/factories/create-slider/create-slider.ts
@@ -9,6 +9,7 @@ import {
   isOpen,
   isInPage,
   makeAutoResize,
+  invokeWithoutDefault,
 } from '../../utils'
 import type { RemoveHandler } from '../../utils'
 import { SLIDER_POSITION, SLIDER_WIDTH } from '../../constants'
@@ -42,10 +43,10 @@ const buildWrapper = (position: Position, width: number) => {
 }
 
 const buildCloseButton = (close: () => void) => {
-  const closeButton = document.createElement('a')
+  const closeButton = document.createElement('button')
   closeButton.className = 'tf-v1-close tf-v1-close-icon'
   closeButton.innerHTML = '&times;'
-  closeButton.onclick = close
+  closeButton.onclick = invokeWithoutDefault(close)
   return closeButton
 }
 

diff --git a/packages/embed/src/factories/create-widget/create-widget.ts b/packages/embed/src/factories/create-widget/create-widget.ts
@@ -6,6 +6,7 @@ import {
   lazyInitialize,
   makeAutoResize,
   changeColorOpacity,
+  invokeWithoutDefault,
 } from '../../utils'
 import {
   getFormHeightChangedHandler,
@@ -22,7 +23,7 @@ import { overrideFullScreenStyles } from './elements/override-full-screen-styles
 export type Widget = EmbedWidget
 
 const buildCloseButton = () => {
-  const closeButton = document.createElement('a')
+  const closeButton = document.createElement('button')
   closeButton.className = 'tf-v1-widget-close tf-v1-close-icon'
   closeButton.innerHTML = '&times;'
   return closeButton
@@ -138,7 +139,7 @@ export const createWidget = (formId: string, options: WidgetOptions): Widget =>
       }
     }
 
-    closeButton.onclick = close
+    closeButton.onclick = invokeWithoutDefault(close)
     container.append(closeButton)
   }
 

diff --git a/packages/embed/src/initializers/initialize-popups.ts b/packages/embed/src/initializers/initialize-popups.ts
@@ -1,11 +1,12 @@
 import { createPopup } from '../factories/create-popup'
 import { POPUP_ATTRIBUTE } from '../constants'
+import { invokeWithoutDefault } from '../utils'
 
 import { initialize } from './initialize'
 
 export const initializePopups = (forceReload: boolean = false) => {
   initialize(POPUP_ATTRIBUTE, 'popup.css', forceReload, (formId, options, button) => {
     const { toggle } = createPopup(formId, options)
-    button.onclick = toggle
+    button.onclick = invokeWithoutDefault(toggle)
   })
 }
diff --git a/packages/embed/src/initializers/initialize-sliders.ts b/packages/embed/src/initializers/initialize-sliders.ts
@@ -1,11 +1,12 @@
 import { createSlider } from '../factories/create-slider'
 import { SLIDER_ATTRIBUTE } from '../constants'
+import { invokeWithoutDefault } from '../utils'
 
 import { initialize } from './initialize'
 
 export const initializeSliders = (forceReload: boolean = false) => {
   initialize(SLIDER_ATTRIBUTE, 'slider.css', forceReload, (formId, options, button) => {
     const { toggle } = createSlider(formId, options)
-    button.onclick = toggle
+    button.onclick = invokeWithoutDefault(toggle)
   })
 }
diff --git a/packages/embed/src/utils/index.ts b/packages/embed/src/utils/index.ts
@@ -17,3 +17,4 @@ export * from './is-open'
 export * from './make-auto-resize'
 export * from './change-color-opacity'
 export * from './hubspot'
+export * from './invoke-without-default'
diff --git a/packages/embed/src/utils/invoke-without-default.spec.ts b/packages/embed/src/utils/invoke-without-default.spec.ts
@@ -0,0 +1,22 @@
+import { invokeWithoutDefault } from './invoke-without-default'
+
+describe('#invokeWithoutDefault', () => {
+  it('should invoke the function passed as argument but call preventDefault() on event', () => {
+    const func = jest.fn()
+    const event = { preventDefault: jest.fn() } as any
+    const handler = invokeWithoutDefault(func)
+    expect(func).not.toHaveBeenCalled()
+    expect(event.preventDefault).not.toHaveBeenCalled()
+    handler(event)
+    expect(func).toHaveBeenCalled()
+    expect(event.preventDefault).toHaveBeenCalled()
+  })
+
+  it('should invoke the function passed as argument when called without event', () => {
+    const func = jest.fn()
+    const handler = invokeWithoutDefault(func)
+    expect(func).not.toHaveBeenCalled()
+    handler()
+    expect(func).toHaveBeenCalled()
+  })
+})
diff --git a/packages/embed/src/utils/invoke-without-default.ts b/packages/embed/src/utils/invoke-without-default.ts
@@ -0,0 +1,4 @@
+export const invokeWithoutDefault = (func: () => void) => (event?: MouseEvent) => {
+  event?.preventDefault()
+  func()
+}