refactor Dialog to use <dialog> internally

keithamus · keithamus · commit 953f2bca7087 · 2023-12-08T14:29:15.000Z
diff --git a/src/ConfirmationDialog/ConfirmationDialog.tsx b/src/ConfirmationDialog/ConfirmationDialog.tsx
@@ -7,6 +7,7 @@ import {FocusKeys} from '@primer/behaviors'
 import {get} from '../constants'
 import {Dialog, DialogProps, DialogHeaderProps, DialogButtonProps} from '../Dialog/Dialog'
 import {useFocusZone} from '../hooks/useFocusZone'
+import BaseStyles from '../BaseStyles'
 
 /**
  * Props to customize the ConfirmationDialog.
@@ -150,11 +151,12 @@ export type ConfirmOptions = Omit<ConfirmationDialogProps, 'onClose'> & {content
 async function confirm(themeProps: ThemeProviderProps, options: ConfirmOptions): Promise<boolean> {
   const {content, ...confirmationDialogProps} = options
   return new Promise(resolve => {
-    hostElement = document.createElement('div')
+    hostElement ||= document.createElement('div')
     if (!hostElement.isConnected) document.body.append(hostElement)
     const root = createRoot(hostElement)
     const onClose: ConfirmationDialogProps['onClose'] = gesture => {
       root.unmount()
+
       if (gesture === 'confirm') {
         resolve(true)
       } else {
@@ -163,9 +165,11 @@ async function confirm(themeProps: ThemeProviderProps, options: ConfirmOptions):
     }
     root.render(
       <ThemeProvider {...themeProps}>
-        <ConfirmationDialog {...confirmationDialogProps} onClose={onClose}>
-          {content}
-        </ConfirmationDialog>
+        <BaseStyles>
+          <ConfirmationDialog {...confirmationDialogProps} onClose={onClose}>
+            {content}
+          </ConfirmationDialog>
+        </BaseStyles>
       </ThemeProvider>,
     )
   })
diff --git a/src/DataTable/__tests__/ErrorDialog.test.tsx b/src/DataTable/__tests__/ErrorDialog.test.tsx
@@ -4,6 +4,13 @@ import React from 'react'
 import {ErrorDialog} from '../ErrorDialog'
 
 describe('Table.ErrorDialog', () => {
+  beforeEach(() => {
+    // In React `autoFocus` calls `.focus()` on mount, but does not set the `autofocus` attribute.
+    // Setting `autofocus=` in JSX will console.error saying "Did you mean autofocus" which fails
+    // the Jest Test, unless we mock console.error...
+    jest.spyOn(console, 'error').mockImplementation()
+  })
+
   it('should use a default title of "Error" if `title` is not provided', () => {
     render(<ErrorDialog />)
     expect(
diff --git a/src/Dialog/Dialog.tsx b/src/Dialog/Dialog.tsx
@@ -1,18 +1,16 @@
-import React, {useCallback, useEffect, useRef, useState} from 'react'
+import React, {useCallback, useRef} from 'react'
 import styled from 'styled-components'
 import Button, {ButtonPrimary, ButtonDanger, ButtonProps} from '../deprecated/Button'
 import Box from '../Box'
 import {get} from '../constants'
-import {useOnEscapePress, useProvidedRefOrCreate} from '../hooks'
-import {useFocusTrap} from '../hooks/useFocusTrap'
 import sx, {SxProp} from '../sx'
 import Octicon from '../Octicon'
 import {XIcon} from '@primer/octicons-react'
 import {useFocusZone} from '../hooks/useFocusZone'
 import {FocusKeys} from '@primer/behaviors'
-import Portal from '../Portal'
 import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef'
 import {useId} from '../hooks/useId'
+import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect'
 
 /* Dialog Version 2 */
 
@@ -95,13 +93,23 @@ export interface DialogProps extends SxProp {
   footerButtons?: DialogButtonProps[]
 
   /**
-   * This method is invoked when a gesture to close the dialog is used (either
-   * an Escape key press or clicking the "X" in the top-right corner). The
-   * gesture argument indicates the gesture that was used to close the dialog
-   * (either 'close-button' or 'escape').
+   * This method is invoked when the dialog has been closed. This could
+   * be from the Dialog.
+   * @param gesture - Deprecated: The gesture argument was used to
+   * indicate if the gesture was from the close-button or escape button.
+   * It will always be `'close-button'`, to check for _cancelations_ of
+   * the dialog using the Esc key, use onCancel.
    */
   onClose: (gesture: 'close-button' | 'escape') => void
 
+  /**
+   * This method is invoked when the user instructs the browser they wish to
+   * dismiss the dialog. Typically this means they have pressed the `Esc` key.
+   *
+   * @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/cancel_event
+   */
+  onCancel?: () => void
+
   /**
    * Default: "dialog". The ARIA role to assign to this dialog.
    * @see https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal
@@ -125,6 +133,11 @@ export interface DialogProps extends SxProp {
    * auto: variable based on contents
    */
   height?: DialogHeight
+
+  /**
+   * Whether or not the Dialog is rendered initially open
+   */
+  initiallyOpen?: boolean
 }
 
 /**
@@ -144,32 +157,11 @@ export interface DialogHeaderProps extends DialogProps {
   dialogDescriptionId: string
 }
 
-const Backdrop = styled('div')`
-  position: fixed;
-  top: 0;
-  left: 0;
-  bottom: 0;
-  right: 0;
-  display: flex;
-  align-items: center;
-  justify-content: center;
-  background-color: ${get('colors.primer.canvas.backdrop')};
-  animation: dialog-backdrop-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')};
-
-  @keyframes dialog-backdrop-appear {
-    0% {
-      opacity: 0;
-    }
-    100% {
-      opacity: 1;
-    }
-  }
-`
-
 const heightMap = {
   small: '480px',
   large: '640px',
   auto: 'auto',
+  'min-content': 'min-content',
 } as const
 
 const widthMap = {
@@ -187,19 +179,40 @@ type StyledDialogProps = {
   height?: DialogHeight
 } & SxProp
 
-const StyledDialog = styled.div<StyledDialogProps>`
-  display: flex;
+const StyledDialog = styled.dialog<StyledDialogProps>`
   flex-direction: column;
   background-color: ${get('colors.canvas.overlay')};
+  color: ${get('colors.fg.default')};
   box-shadow: ${get('shadows.overlay.shadow')};
   min-width: 296px;
   max-width: calc(100vw - 64px);
   max-height: calc(100vh - 64px);
   width: ${props => widthMap[props.width ?? ('xlarge' as const)]};
   height: ${props => heightMap[props.height ?? ('auto' as const)]};
+  border: 0;
   border-radius: 12px;
+  padding: 0;
   opacity: 1;
   animation: overlay--dialog-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')};
+  overflow: initial;
+
+  &[open] {
+    display: flex;
+  }
+
+  &::backdrop {
+    background-color: ${get('colors.primer.canvas.backdrop')};
+    animation: dialog-backdrop-appear ${ANIMATION_DURATION} ${get('animation.easeOutCubic')};
+  }
+
+  @keyframes dialog-backdrop-appear {
+    0% {
+      opacity: 0;
+    }
+    100% {
+      opacity: 1;
+    }
+  }
 
   @keyframes overlay--dialog-appear {
     0% {
@@ -252,84 +265,56 @@ const DefaultFooter: React.FC<React.PropsWithChildren<DialogProps>> = ({footerBu
   ) : null
 }
 
-const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogProps>>((props, forwardedRef) => {
+const _Dialog = React.forwardRef<HTMLDialogElement, React.PropsWithChildren<DialogProps>>((props, forwardedRef) => {
   const {
     title = 'Dialog',
     subtitle = '',
     renderHeader,
     renderBody,
     renderFooter,
     onClose,
+    onCancel,
     role = 'dialog',
     width = 'xlarge',
     height = 'auto',
-    footerButtons = [],
+    initiallyOpen = true,
     sx,
   } = props
   const dialogLabelId = useId()
   const dialogDescriptionId = useId()
-  const autoFocusedFooterButtonRef = useRef<HTMLButtonElement>(null)
-  for (const footerButton of footerButtons) {
-    if (footerButton.autoFocus) {
-      footerButton.ref = autoFocusedFooterButtonRef
-    }
-  }
   const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId}
 
-  const dialogRef = useRef<HTMLDivElement>(null)
+  const dialogRef = useRef<HTMLDialogElement>(null)
   useRefObjectAsForwardedRef(forwardedRef, dialogRef)
-  const backdropRef = useRef<HTMLDivElement>(null)
-  useFocusTrap({containerRef: dialogRef, restoreFocusOnCleanUp: true, initialFocusRef: autoFocusedFooterButtonRef})
-
-  useOnEscapePress(
-    (event: KeyboardEvent) => {
-      onClose('escape')
-      event.preventDefault()
-    },
-    [onClose],
-  )
 
-  React.useEffect(() => {
-    const bodyOverflowStyle = document.body.style.overflow || ''
-    // If the body is already set to overflow: hidden, it likely means
-    // that there is already a modal open. In that case, we should bail
-    // so we don't re-enable scroll after the second dialog is closed.
-    if (bodyOverflowStyle === 'hidden') {
-      return
+  useIsomorphicLayoutEffect(() => {
+    if (initiallyOpen && !dialogRef.current?.open && dialogRef.current?.isConnected) {
+      dialogRef.current.showModal()
     }
+  }, [initiallyOpen, dialogRef])
 
-    document.body.style.overflow = 'hidden'
-
-    return () => {
-      document.body.style.overflow = bodyOverflowStyle
-    }
-  }, [])
+  const onCloseHandler = useCallback(() => onClose('close-button'), [onClose])
 
   const header = (renderHeader ?? DefaultHeader)(defaultedProps)
   const body = (renderBody ?? DefaultBody)(defaultedProps)
   const footer = (renderFooter ?? DefaultFooter)(defaultedProps)
 
   return (
-    <>
-      <Portal>
-        <Backdrop ref={backdropRef}>
-          <StyledDialog
-            width={width}
-            height={height}
-            ref={dialogRef}
-            role={role}
-            aria-labelledby={dialogLabelId}
-            aria-describedby={dialogDescriptionId}
-            aria-modal
-            sx={sx}
-          >
-            {header}
-            {body}
-            {footer}
-          </StyledDialog>
-        </Backdrop>
-      </Portal>
-    </>
+    <StyledDialog
+      width={width}
+      height={height === 'auto' ? 'min-content' : height}
+      ref={dialogRef}
+      role={role}
+      aria-labelledby={dialogLabelId}
+      aria-describedby={dialogDescriptionId}
+      sx={sx}
+      onCancel={onCancel}
+      onClose={onCloseHandler}
+    >
+      {header}
+      {body}
+      {footer}
+    </StyledDialog>
   )
 })
 _Dialog.displayName = 'Dialog'
@@ -385,17 +370,10 @@ const buttonTypes = {
 }
 const Buttons: React.FC<React.PropsWithChildren<{buttons: DialogButtonProps[]}>> = ({buttons}) => {
   const autoFocusRef = useProvidedRefOrCreate<HTMLButtonElement>(buttons.find(button => button.autoFocus)?.ref)
-  let autoFocusCount = 0
   const [hasRendered, setHasRendered] = useState(0)
-  useEffect(() => {
-    // hack to work around dialogs originating from other focus traps.
-    if (hasRendered === 1) {
-      autoFocusRef.current?.focus()
-    } else {
-      setHasRendered(hasRendered + 1)
-    }
+  useIsomorphicLayoutEffect(() => {
+    autoFocusRef.current?.setAttribute('autofocus')
   }, [autoFocusRef, hasRendered])
-
   return (
     <>
       {buttons.map((dialogButtonProps, index) => {
diff --git a/src/utils/test-helpers.tsx b/src/utils/test-helpers.tsx
@@ -18,3 +18,12 @@ global.CSS = {
 }
 
 global.TextEncoder = TextEncoder
+
+// Dialog showModal isn't implemented in JSDOM https://github.com/jsdom/jsdom/issues/3294
+global.HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) {
+  // eslint-disable-next-line no-invalid-this
+  this.open = true
+  // eslint-disable-next-line no-invalid-this
+  const element: HTMLElement | null = this.querySelector('[autofocus]')
+  element?.focus()
+})
