SelectPanel2: Fix bug calling onCancel (#4131)

* noob mistake: remove event listener as well!

* Esc calls internal close not cancel now

* wrap imperative change in an effect

* Create modern-files-suffer.md

* tiny optimisation

* add enough tests for this PR

* missed a spot!

* only need to apply effect when function changes

.changeset/modern-files-suffer.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+experimental/SelectPanel: Fix bug where onSubmit also called onCancel!

src/drafts/SelectPanel2/SelectPanel.test.tsx

@@ -0,0 +1,132 @@
+import React from 'react'
+import {ThemeProvider, ActionList} from '../../'
+import {render, RenderResult} from '@testing-library/react'
+import userEvent, {UserEvent} from '@testing-library/user-event'
+import data from './stories/mock-data'
+import {SelectPanel, SelectPanelProps} from './SelectPanel'
+
+const Fixture = ({onSubmit, onCancel}: Pick<SelectPanelProps, 'onSubmit' | 'onCancel'>) => {
+  const initialSelectedLabels = data.issue.labelIds // mock initial state: has selected labels
+  const [selectedLabelIds, setSelectedLabelIds] = React.useState<string[]>(initialSelectedLabels)
+
+  /* Selection */
+  const onLabelSelect = (labelId: string) => {
+    if (!selectedLabelIds.includes(labelId)) setSelectedLabelIds([...selectedLabelIds, labelId])
+    else setSelectedLabelIds(selectedLabelIds.filter(id => id !== labelId))
+  }
+
+  const itemsToShow = data.labels
+
+  return (
+    <ThemeProvider>
+      <SelectPanel title="Select labels" onSubmit={onSubmit} onCancel={onCancel}>
+        <SelectPanel.Button>Assign label</SelectPanel.Button>
+
+        <ActionList>
+          {itemsToShow.map(label => (
+            <ActionList.Item
+              key={label.id}
+              onSelect={() => onLabelSelect(label.id)}
+              selected={selectedLabelIds.includes(label.id)}
+            >
+              {label.name}
+              <ActionList.Description variant="block">{label.description}</ActionList.Description>
+            </ActionList.Item>
+          ))}
+        </ActionList>
+        <SelectPanel.Footer />
+      </SelectPanel>
+    </ThemeProvider>
+  )
+}
+
+describe('SelectPanel', () => {
+  it('renders Button by default', async () => {
+    const container = render(<Fixture />)
+
+    const trigger = container.getByRole('button')
+    expect(trigger).toBeInTheDocument()
+    expect(container.queryByRole('dialog')).toBeNull()
+  })
+
+  it('opens Dialog on Button click', async () => {
+    const container = render(<Fixture />)
+    const user = userEvent.setup()
+
+    expect(container.queryByRole('dialog')).toBeNull()
+
+    const trigger = container.getByRole('button')
+    await user.click(trigger)
+
+    expect(container.queryByRole('dialog')).toBeInTheDocument()
+  })
+
+  type MockFunctions = {mockOnSubmit?: () => void; mockOnCancel?: () => void}
+  const getFixtureWithOpenContainer = async ({mockOnSubmit, mockOnCancel}: MockFunctions = {}) => {
+    const container = render(<Fixture onSubmit={mockOnSubmit} onCancel={mockOnCancel} />)
+    const user = await userEvent.setup()
+
+    const trigger = container.getByRole('button')
+    await user.click(trigger)
+
+    return {container, user}
+  }
+
+  it('clicking an item selects it', async () => {
+    const {container, user} = await getFixtureWithOpenContainer()
+
+    const initiallyUnselectedOption = container.getAllByRole('option')[2]
+    expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'false')
+
+    await user.click(initiallyUnselectedOption)
+    expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'true')
+  })
+
+  const selectUnselectedOption = async (container: RenderResult, user: UserEvent) => {
+    const initiallyUnselectedOption = container.getAllByRole('option')[2]
+    expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'false')
+    await user.click(initiallyUnselectedOption)
+  }
+
+  it('submit closes the dialog and calls onSubmit', async () => {
+    const mockOnSubmit = jest.fn()
+    const mockOnCancel = jest.fn()
+    const {container, user} = await getFixtureWithOpenContainer({mockOnSubmit, mockOnCancel})
+    selectUnselectedOption(container, user)
+
+    const submitButton = container.getByRole('button', {name: 'Save'})
+    await user.click(submitButton)
+
+    expect(container.queryByRole('dialog')).toBeNull()
+    expect(mockOnSubmit).toHaveBeenCalledTimes(1)
+    expect(mockOnCancel).toHaveBeenCalledTimes(0)
+  })
+
+  it('cancel closes the dialog and calls onCancel', async () => {
+    const mockOnSubmit = jest.fn()
+    const mockOnCancel = jest.fn()
+    const {container, user} = await getFixtureWithOpenContainer({mockOnSubmit, mockOnCancel})
+    selectUnselectedOption(container, user)
+
+    const cancelButton = container.getByRole('button', {name: 'Cancel'})
+    await user.click(cancelButton)
+
+    expect(container.queryByRole('dialog')).toBeNull()
+    expect(mockOnCancel).toHaveBeenCalledTimes(1)
+    expect(mockOnSubmit).toHaveBeenCalledTimes(0)
+  })
+
+  it('close button closes the dialog and calls onCancel', async () => {
+    const mockOnSubmit = jest.fn()
+    const mockOnCancel = jest.fn()
+    const {container, user} = await getFixtureWithOpenContainer({mockOnSubmit, mockOnCancel})
+    selectUnselectedOption(container, user)
+
+    const closeButton = container.getByRole('button', {name: 'Close'})
+    await user.click(closeButton)
+
+    expect(container.queryByRole('dialog')).toBeNull()
+    expect(mockOnCancel).toHaveBeenCalledTimes(1)
+    expect(mockOnSubmit).toHaveBeenCalledTimes(0)
+  })
+})

src/drafts/SelectPanel2/SelectPanel.tsx

@@ -98,7 +98,7 @@ const Panel: React.FC<SelectPanelProps> = ({
 
   const onAnchorClick = () => {
     if (!internalOpen) setInternalOpen(true)
-    else onInternalClose()
+    else onInternalCancel()
   }
 
   const contents = React.Children.map(props.children, child => {
@@ -115,14 +115,19 @@ const Panel: React.FC<SelectPanelProps> = ({
     return child
   })
 
-  const onInternalClose = () => {
+  const onInternalClose = React.useCallback(() => {
+    if (internalOpen === false) return // nothing to do here
     if (propsOpen === undefined) setInternalOpen(false)
+  }, [internalOpen, propsOpen])
+
+  const onInternalCancel = () => {
+    onInternalClose()
     if (typeof propsOnCancel === 'function') propsOnCancel()
   }
 
   const onInternalSubmit = (event?: React.FormEvent<HTMLFormElement>) => {
     event?.preventDefault() // there is no event with selectionVariant=instant
-    if (propsOpen === undefined) setInternalOpen(false)
+    onInternalClose()
     if (typeof propsOnSubmit === 'function') propsOnSubmit(event)
   }
 
@@ -152,17 +157,32 @@ const Panel: React.FC<SelectPanelProps> = ({
 
   /* Dialog */
   const dialogRef = React.useRef<HTMLDialogElement>(null)
-  if (internalOpen) dialogRef.current?.showModal()
-  else dialogRef.current?.close()
+
+  // sync dialog open state (imperative) with internal component state
+  React.useEffect(() => {
+    if (internalOpen) dialogRef.current?.showModal()
+    else if (dialogRef.current?.open) dialogRef.current.close()
+  }, [internalOpen])
 
   // dialog handles Esc automatically, so we have to sync internal state
+  // but it doesn't call onCancel, so have another effect for that!
+  React.useEffect(() => {
+    const dialogEl = dialogRef.current
+    dialogEl?.addEventListener('close', onInternalClose)
+    return () => dialogEl?.removeEventListener('close', onInternalClose)
+  }, [onInternalClose])
+
+  // Esc handler
   React.useEffect(() => {
-    const dialog = dialogRef.current
-    dialog?.addEventListener('close', onInternalClose)
-    return () => dialog?.removeEventListener('close', onInternalClose)
+    const dialogEl = dialogRef.current
+    const handler = (event: KeyboardEvent) => {
+      if (event.key === 'Escape') onInternalCancel()
+    }
+    dialogEl?.addEventListener('keydown', handler)
+    return () => dialogEl?.removeEventListener('keydown', handler)
   })
 
-  // React doesn't support autoFocus for dialog: https://github.com/facebook/react/issues/23301
+  // Autofocus hack: React doesn't support autoFocus for dialog: https://github.com/facebook/react/issues/23301
   // tl;dr: react takes over autofocus instead of letting the browser handle it,
   // but not for dialogs, so we have to do it
   React.useEffect(() => {
@@ -232,7 +252,7 @@ const Panel: React.FC<SelectPanelProps> = ({
             panelId,
             title,
             description,
-            onCancel: onInternalClose,
+            onCancel: onInternalCancel,
             onClearSelection: propsOnClearSelection ? onInternalClearSelection : undefined,
             searchQuery,
             setSearchQuery,

src/utils/test-helpers.tsx

@@ -18,3 +18,22 @@ global.CSS = {
 }
 
 global.TextEncoder = TextEncoder
+
+/**
+ * Required for internal usage of dialog in primer/react
+ * this is not implemented in JSDOM, and until it is we'll need to polyfill
+ * https://github.com/jsdom/jsdom/issues/3294
+ * https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom
+ * bonus: we only want to mock browser globals in DOM (or js-dom) environments – not in SSR / node
+ */
+if (typeof document !== 'undefined') {
+  global.HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) {
+    // eslint-disable-next-line no-invalid-this
+    this.open = true
+  })
+
+  global.HTMLDialogElement.prototype.close = jest.fn(function mock(this: HTMLDialogElement) {
+    // eslint-disable-next-line no-invalid-this
+    this.open = false
+  })
+}