diff --git a/.changeset/react-compiler-dialog.md b/.changeset/react-compiler-dialog.md new file mode 100644 index 00000000000..20575932173 --- /dev/null +++ b/.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. diff --git a/packages/react/script/react-compiler.mjs b/packages/react/script/react-compiler.mjs index 294557261bc..95a9ee98b36 100644 --- a/packages/react/script/react-compiler.mjs +++ b/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', diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index 21f1fc2447c..b40fc2f308f 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/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(null) + + return ( + <> + + {showReturnTarget && } + {isOpen && ( + { + setIsOpen(false) + }} + returnFocusRef={returnFocusRef} + > + + + )} + + ) + } + + const user = userEvent.setup() + const {getByRole, queryByRole} = render() + 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( + + {}} + footerButtons={[ + { + content: 'Not auto-focused', + }, + { + content: 'First auto-focus', + autoFocus: true, + }, + { + content: 'Second auto-focus', + autoFocus: true, + }, + ]} + > + Body + + , + ) + + 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() + const footerButton = Object.freeze({ + content: 'Footer button', + autoFocus: true, + ref: suppliedRef, + }) + const {getByRole} = render( + + {}} footerButtons={[footerButton]}> + Body + + , + ) + + 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( diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index ef7b8d888d4..938c6723e62 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/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(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(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 { - 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,7 +501,10 @@ Footer.displayName = 'Dialog.Footer' const Buttons: React.FC> = ({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(null) const mergedRef = useMergedRefs(autoFocusRef, providedButtonRef) // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`. @@ -497,7 +512,6 @@ const Buttons: React.FC> const providedOrCreatedRef = useProvidedRefOrCreate(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> // '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}