refactor Dialog to use <dialog> internally

Author: keithamus

src/ConfirmationDialog/ConfirmationDialog.test.tsx

@@ -72,6 +72,14 @@ const ShorthandHookFromActionMenu = () => {
 }
 
 describe('ConfirmationDialog', () => {
+  beforeEach(() => {
+    // Dialog showModal isn't implemented in JSDOM https://github.com/jsdom/jsdom/issues/3294
+    HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) {
+      // eslint-disable-next-line no-invalid-this
+      this.open = true
+    })
+  })
+
   behavesAsComponent({
     Component: ConfirmationDialog,
     toRender: () => <Basic />,

src/ConfirmationDialog/ConfirmationDialog.tsx

@@ -16,7 +16,7 @@ export interface ConfirmationDialogProps {
    * Required. This callback is invoked when a gesture to close the dialog
    * is performed. The first argument indicates the gesture.
    */
-  onClose: (gesture: 'confirm' | 'cancel' | 'close-button' | 'cancel' | 'escape') => void
+  onClose: (gesture: 'confirm' | 'cancel' | 'close-button' | 'escape') => void
 
   /**
    * Required. The title of the ConfirmationDialog. This is usually a brief
@@ -145,13 +145,16 @@ export const ConfirmationDialog: React.FC<React.PropsWithChildren<ConfirmationDi
   )
 }
 
+let hostElement: Element | null = null
 export type ConfirmOptions = Omit<ConfirmationDialogProps, 'onClose'> & {content: React.ReactNode}
 async function confirm(themeProps: ThemeProviderProps, options: ConfirmOptions): Promise<boolean> {
   const {content, ...confirmationDialogProps} = options
   return new Promise(resolve => {
-    const hostElement = document.createElement('div')
+    hostElement ||= document.createElement('div')
+    if (!hostElement.isConnected) document.body.append(hostElement)
     const onClose: ConfirmationDialogProps['onClose'] = gesture => {
-      ReactDOM.unmountComponentAtNode(hostElement)
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      ReactDOM.unmountComponentAtNode(hostElement!)
       if (gesture === 'confirm') {
         resolve(true)
       } else {

src/DataTable/__tests__/ErrorDialog.test.tsx

@@ -4,6 +4,14 @@ import React from 'react'
 import {ErrorDialog} from '../ErrorDialog'
 
 describe('Table.ErrorDialog', () => {
+  beforeEach(() => {
+    // Dialog showModal isn't implemented in JSDOM https://github.com/jsdom/jsdom/issues/3294
+    HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) {
+      // eslint-disable-next-line no-invalid-this
+      this.open = true
+    })
+  })
+
   it('should use a default title of "Error" if `title` is not provided', () => {
     render(<ErrorDialog />)
     expect(

src/Dialog.tsx

@@ -106,7 +106,7 @@ const Dialog = forwardRef<HTMLDivElement, InternalDialogProps>(
       }
     }
 
-    const {getDialogProps} = useDialog({
+    useDialog({
       modalRef,
       onDismiss: onCloseClick,
       isOpen,
@@ -118,7 +118,7 @@ const Dialog = forwardRef<HTMLDivElement, InternalDialogProps>(
     return isOpen ? (
       <>
         <Overlay ref={overlayRef} />
-        <DialogBase tabIndex={-1} ref={modalRef} role="dialog" aria-modal="true" {...props} {...getDialogProps()}>
+        <DialogBase tabIndex={-1} ref={modalRef} role="dialog" aria-modal="true" {...props}>
           <ButtonClose
             ref={closeButtonRef}
             onClick={onCloseClick}

src/Dialog/Dialog.tsx

@@ -1,18 +1,17 @@
-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 {useOnEscapePress} from '../hooks'
 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 */
 
@@ -125,6 +124,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 +148,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,8 +170,7 @@ 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')};
   box-shadow: ${get('shadows.overlay.shadow')};
@@ -197,10 +179,30 @@ const StyledDialog = styled.div<StyledDialogProps>`
   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')};
 
+  &[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% {
       opacity: 0;
@@ -252,7 +254,7 @@ 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 = '',
@@ -264,6 +266,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
     width = 'xlarge',
     height = 'auto',
     footerButtons = [],
+    initiallyOpen = true,
     sx,
   } = props
   const dialogLabelId = useId()
@@ -276,10 +279,8 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
   }
   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) => {
@@ -289,47 +290,30 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
     [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
-    }
-
-    document.body.style.overflow = 'hidden'
-
-    return () => {
-      document.body.style.overflow = bodyOverflowStyle
+  useIsomorphicLayoutEffect(() => {
+    if (initiallyOpen && !dialogRef.current?.open && dialogRef.current?.isConnected) {
+      dialogRef.current.showModal()
     }
-  }, [])
+  }, [initiallyOpen, dialogRef])
 
   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 === 'alertdialog' ? role : undefined}
+      aria-labelledby={dialogLabelId}
+      aria-describedby={dialogDescriptionId}
+      sx={sx}
+    >
+      {header}
+      {body}
+      {footer}
+    </StyledDialog>
   )
 })
 _Dialog.displayName = 'Dialog'
@@ -390,30 +374,13 @@ const buttonTypes = {
   danger: ButtonDanger,
 }
 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)
-    }
-  }, [autoFocusRef, hasRendered])
-
   return (
     <>
       {buttons.map((dialogButtonProps, index) => {
         const {content, buttonType = 'normal', autoFocus = false, ...buttonProps} = dialogButtonProps
         const ButtonElement = buttonTypes[buttonType]
         return (
-          <ButtonElement
-            key={index}
-            {...buttonProps}
-            variant={buttonType}
-            ref={autoFocus && autoFocusCount === 0 ? (autoFocusCount++, autoFocusRef) : null}
-          >
+          <ButtonElement key={index} {...buttonProps} variant={buttonType} autoFocus={autoFocus}>
             {content}
           </ButtonElement>
         )

src/Dialog/__snapshots__/Dialog.test.tsx.snap

@@ -139,7 +139,6 @@ exports[`Dialog renders consistently 1`] = `
     aria-labelledby="header"
     aria-modal="true"
     className="c0"
-    onKeyDown={[Function]}
     role="dialog"
     tabIndex={-1}
   >

src/TreeView/TreeView.test.tsx

@@ -1177,6 +1177,14 @@ describe('State', () => {
 })
 
 describe('Asyncronous loading', () => {
+  beforeEach(() => {
+    // Dialog showModal isn't implemented in JSDOM https://github.com/jsdom/jsdom/issues/3294
+    HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) {
+      // eslint-disable-next-line no-invalid-this
+      this.open = true
+    })
+  })
+
   it('updates aria live region when loading is done', () => {
     function TestTree() {
       const [state, setState] = React.useState<SubTreeState>('loading')

src/TreeView/useRovingTabIndex.ts

@@ -21,6 +21,22 @@ export function useRovingTabIndex({
     preventScroll: true,
     getNextFocusable: (direction, from, event) => {
       if (!(from instanceof HTMLElement)) return
+      // Skip top layer elements
+      try {
+        // These need to be separate if conditions as different
+        // browsers may or may not support them.
+        if (from.closest('dialog[open]')) {
+          return
+        }
+        if (from.closest(':popover-open')) {
+          return
+        }
+        if (from.closest('.:popover-open')) {
+          return
+        }
+      } catch {
+        // Don't return
+      }
 
       return getNextFocusableElement(from, event) ?? from
     },

src/drafts/Button2/Button.stories.tsx

@@ -60,7 +60,7 @@ export default {
 export const Playground: StoryFn = args => {
   const {trailingVisualCount, ...rest} = args
   return (
-    <Button {...rest}>
+    <Button {...rest} autoFocus={true}>
       Default
       {typeof trailingVisualCount === 'undefined' ? null : <Button.Counter>{trailingVisualCount}</Button.Counter>}
     </Button>