perf(Dialog): enable React Compiler

.changeset/react-compiler-dialog.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+Dialog: Improve rendering performance with React Compiler support. Restore focus to the previous element when the return-focus target disappears.

packages/react/script/react-compiler.mjs

@@ -14,7 +14,6 @@ const files = glob
 const unsupportedPatterns = [
   'src/ActionMenu/ActionMenu.test.tsx',
   'src/AnchoredOverlay/AnchoredOverlay.tsx',
-  'src/Dialog/Dialog.tsx',
   'src/FilteredActionList/FilteredActionList.tsx',
   'src/LabelGroup/LabelGroup.tsx',
   'src/Overlay/Overlay.figma.tsx',

packages/react/src/Dialog/Dialog.test.tsx

@@ -361,6 +361,55 @@ describe('Dialog', () => {
     expect(getByRole('button', {name: 'return focus to (button 2)'})).toHaveFocus()
   })
 
+  it('falls back to the previously focused element when the return focus target disappears before closing', async () => {
+    const Fixture = () => {
+      const [isOpen, setIsOpen] = React.useState(false)
+      const [showReturnTarget, setShowReturnTarget] = React.useState(true)
+      const returnFocusRef = React.useRef<HTMLButtonElement>(null)
+
+      return (
+        <>
+          <Button
+            onClick={() => {
+              setIsOpen(true)
+            }}
+          >
+            Open dialog
+          </Button>
+          {showReturnTarget && <Button ref={returnFocusRef}>Return target</Button>}
+          {isOpen && (
+            <Dialog
+              onClose={() => {
+                setIsOpen(false)
+              }}
+              returnFocusRef={returnFocusRef}
+            >
+              <Button
+                onClick={() => {
+                  setShowReturnTarget(false)
+                }}
+              >
+                Remove return target
+              </Button>
+            </Dialog>
+          )}
+        </>
+      )
+    }
+
+    const user = userEvent.setup()
+    const {getByRole, queryByRole} = render(<Fixture />)
+    const triggerButton = getByRole('button', {name: 'Open dialog'})
+
+    await user.click(triggerButton)
+    await user.click(getByRole('button', {name: 'Remove return target'}))
+    expect(queryByRole('button', {name: 'Return target'})).not.toBeInTheDocument()
+    await user.click(getByRole('button', {name: 'Close'}))
+
+    expect(queryByRole('dialog')).not.toBeInTheDocument()
+    expect(triggerButton).toHaveFocus()
+  })
+
   it('should support `className` on the Dialog element', async () => {
     const Fixture = () => {
       const [isOpen, setIsOpen] = React.useState(true)
@@ -570,6 +619,58 @@ describe('Footer button loading states', () => {
 
 describe('Dialog auto-focus button forwarded ref (primer_react_merged_forwarded_refs)', () => {
   for (const enabled of [true, false]) {
+    it(`focuses only the first auto-focus footer button with the flag ${enabled ? 'enabled' : 'disabled'}`, async () => {
+      const {getByRole} = render(
+        <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
+          <Dialog
+            onClose={() => {}}
+            footerButtons={[
+              {
+                content: 'Not auto-focused',
+              },
+              {
+                content: 'First auto-focus',
+                autoFocus: true,
+              },
+              {
+                content: 'Second auto-focus',
+                autoFocus: true,
+              },
+            ]}
+          >
+            Body
+          </Dialog>
+        </FeatureFlags>,
+      )
+
+      await waitFor(() => {
+        expect(getByRole('button', {name: 'First auto-focus'})).toHaveFocus()
+      })
+    })
+
+    it(`does not mutate caller footer button props with the flag ${enabled ? 'enabled' : 'disabled'}`, async () => {
+      const suppliedRef = React.createRef<HTMLButtonElement>()
+      const footerButton = Object.freeze({
+        content: 'Footer button',
+        autoFocus: true,
+        ref: suppliedRef,
+      })
+      const {getByRole} = render(
+        <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
+          <Dialog onClose={() => {}} footerButtons={[footerButton]}>
+            Body
+          </Dialog>
+        </FeatureFlags>,
+      )
+
+      await waitFor(() => {
+        expect(getByRole('button', {name: 'Footer button'})).toHaveFocus()
+      })
+      expect(footerButton.ref).toBe(suppliedRef)
+      // Dialog's internal auto-focus ref still takes precedence over a supplied footer ref.
+      expect(suppliedRef.current).toBeNull()
+    })
+
     it(`focuses the auto-focus footer button via the gated ref with the flag ${enabled ? 'enabled' : 'disabled'}`, async () => {
       const {getByRole} = render(
         <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>

packages/react/src/Dialog/Dialog.tsx

@@ -28,6 +28,22 @@ import {DialogContext} from './DialogContext'
  */
 let dialogScrollDisabledCount = 0
 
+function disableDialogScroll() {
+  const scrollbarWidth = window.innerWidth - document.body.clientWidth
+
+  dialogScrollDisabledCount++
+  document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)
+  document.body.setAttribute('data-dialog-scroll-disabled', '')
+
+  return () => {
+    dialogScrollDisabledCount--
+    if (dialogScrollDisabledCount === 0) {
+      document.body.style.removeProperty('--prc-dialog-scrollgutter')
+      document.body.removeAttribute('data-dialog-scroll-disabled')
+    }
+  }
+}
+
 /**
  * Props that characterize a button to be rendered into the footer of
  * a Dialog.
@@ -286,7 +302,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
     role = 'dialog',
     width = 'xlarge',
     height = 'auto',
-    footerButtons = defaultFooterButtons,
+    footerButtons: providedFooterButtons = defaultFooterButtons,
     position = defaultPosition,
     align,
     returnFocusRef,
@@ -297,15 +313,26 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
   const dialogLabelId = useId()
   const dialogDescriptionId = useId()
   const autoFocusedFooterButtonRef = useRef<HTMLButtonElement>(null)
-  for (const footerButton of footerButtons) {
+  const footerButtons = providedFooterButtons.map(footerButton => {
     if (footerButton.autoFocus) {
-      // eslint-disable-next-line react-hooks/immutability
-      footerButton.ref = autoFocusedFooterButtonRef
+      return {
+        ...footerButton,
+        ref: autoFocusedFooterButtonRef,
+      }
     }
-  }
+    return footerButton
+  })
   const [lastMouseDownIsBackdrop, setLastMouseDownIsBackdrop] = useState<boolean>(false)
   const [footerButtonLayout, setFooterButtonLayout] = useState<'scroll' | 'wrap'>('wrap')
-  const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId}
+  const defaultedProps = {
+    ...props,
+    title,
+    subtitle,
+    role,
+    dialogLabelId,
+    dialogDescriptionId,
+    ...(props.footerButtons && {footerButtons}),
+  }
   const onBackdropClick = useCallback(
     (e: SyntheticEvent) => {
       if (e.target === e.currentTarget && lastMouseDownIsBackdrop) {
@@ -327,8 +354,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
   useFocusTrap({
     containerRef: dialogRef,
     initialFocusRef: initialFocusRef ?? autoFocusedFooterButtonRef,
-    // eslint-disable-next-line react-hooks/refs
-    restoreFocusOnCleanUp: returnFocusRef?.current ? false : true,
+    restoreFocusOnCleanUp: true,
     returnFocusRef,
   })
 
@@ -340,21 +366,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
     [onClose],
   )
 
-  React.useEffect(() => {
-    const scrollbarWidth = window.innerWidth - document.body.clientWidth
-
-    dialogScrollDisabledCount++
-    document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)
-    document.body.setAttribute('data-dialog-scroll-disabled', '')
-
-    return () => {
-      dialogScrollDisabledCount--
-      if (dialogScrollDisabledCount === 0) {
-        document.body.style.removeProperty('--prc-dialog-scrollgutter')
-        document.body.removeAttribute('data-dialog-scroll-disabled')
-      }
-    }
-  }, [])
+  React.useEffect(disableDialogScroll, [])
 
   const header = slots.header ?? (renderHeader ?? DefaultHeader)(defaultedProps)
   const body = slots.body ?? (renderBody ?? DefaultBody)({...defaultedProps, children: childrenWithoutSlots})
@@ -489,15 +501,17 @@ Footer.displayName = 'Dialog.Footer'
 
 const Buttons: React.FC<React.PropsWithChildren<{buttons: DialogButtonProps[]}>> = ({buttons}) => {
   const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs')
-  const providedButtonRef = buttons.find(button => button.autoFocus)?.ref
+  const autoFocusIndex = buttons.findIndex(button => {
+    return button.autoFocus
+  })
+  const providedButtonRef = buttons[autoFocusIndex]?.ref
   const autoFocusRef = useRef<HTMLButtonElement>(null)
   const mergedRef = useMergedRefs(autoFocusRef, providedButtonRef)
   // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`.
   // At graduation: remove the three declarations below, and replace all instances of `readRef` with `autoFocusRef` and `appliedRef` with `mergedRef`.
   const providedOrCreatedRef = useProvidedRefOrCreate<HTMLButtonElement>(providedButtonRef)
   const readRef = mergedRefEnabled ? autoFocusRef : providedOrCreatedRef
   const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef
-  let autoFocusCount = 0
   const [hasRendered, setHasRendered] = useState(0)
   useEffect(() => {
     // hack to work around dialogs originating from other focus traps.
@@ -521,7 +535,7 @@ const Buttons: React.FC<React.PropsWithChildren<{buttons: DialogButtonProps[]}>>
             // 'normal' value is equivalent to 'default', this is used for backwards compatibility
             variant={buttonType === 'normal' ? 'default' : buttonType}
             // @ts-expect-error it needs a non nullable ref
-            ref={autoFocus && autoFocusCount === 0 ? (autoFocusCount++, appliedRef) : null}
+            ref={autoFocus && index === autoFocusIndex ? appliedRef : null}
           >
             {content}
           </Button>