bugfix(Dialog): Fixing issue where page shifts when dialog is open

.changeset/empty-experts-glow.md

@@ -0,0 +1,4 @@
+---
+"@primer/react": patch
+---
+Bug fix: Fixing issue where page shifts when Dialog is open

packages/react/jest.config.js

@@ -28,6 +28,7 @@ module.exports = {
     '<rootDir>/src/CircleBadge/',
     '<rootDir>/src/CircleOcticon/',
     '<rootDir>/src/DataTable/',
+    '<rootDir>/src/Dialog/',
     '<rootDir>/src/FeatureFlags/',
     '<rootDir>/src/Flash/',
     '<rootDir>/src/FormControl/__tests__/FormControl.Validation.test.tsx',

packages/react/src/Dialog/Dialog.dev.stories.tsx

@@ -1,5 +1,5 @@
 import type React from 'react'
-import {useState, useCallback} from 'react'
+import {useState, useCallback, useRef} from 'react'
 import {Text, Button} from '..'
 import type {DialogProps, DialogWidth, DialogHeight} from './Dialog'
 import {Dialog} from './Dialog'
@@ -193,3 +193,42 @@ export const WithSxAndCss = ({width, height, subtitle}: DialogStoryProps) => {
     </>
   )
 }
+
+export const WithScrollBody = () => {
+  const [isOpen, setIsOpen] = useState(false)
+  const buttonRef = useRef<HTMLButtonElement>(null)
+  const onDialogClose = useCallback(() => setIsOpen(false), [])
+  return (
+    <>
+      <Button ref={buttonRef} onClick={() => setIsOpen(!isOpen)}>
+        Show dialog
+      </Button>
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {lipsum}
+      {isOpen && (
+        <Dialog
+          title="My Dialog"
+          onClose={onDialogClose}
+          footerButtons={[
+            {buttonType: 'danger', content: 'Delete the universe', onClick: onDialogClose},
+            {buttonType: 'primary', content: 'Proceed', onClick: onDialogClose},
+          ]}
+        >
+          {lipsum}
+        </Dialog>
+      )}
+    </>
+  )
+}
+WithScrollBody.parameters = {
+  layout: 'fullscreen',
+}

packages/react/src/Dialog/Dialog.module.css

@@ -1,3 +1,16 @@
+/* The --dialog-scrollgutter property is used only on the body element to
+ * simulate scrollbar-gutter:stable. This property is not and should not
+ * be used elsewhere in the DOM. There is a performance penalty to
+ * setting inherited properties which can cause a large style recalc to
+ * occur, so it benefits us to prevent inheritance for this property.
+ * See https://web.dev/blog/at-property-performance
+ */
+@property --dialog-scrollgutter {
+  initial-value: 0;
+  inherits: false;
+  syntax: '<length>';
+}
+
 @keyframes dialog-backdrop-appear {
   0% {
     opacity: 0;
@@ -216,6 +229,12 @@
   }
 }
 
+body:has(.Dialog.DisableScroll) {
+  /* stylelint-disable-next-line primer/spacing */
+  padding-right: var(--dialog-scrollgutter) !important;
+  overflow: hidden !important;
+}
+
 .DialogOverflowWrapper {
   flex-grow: 1;
 }

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

@@ -1,10 +1,9 @@
 import React from 'react'
-import {fireEvent, render, waitFor} from '@testing-library/react'
+import {render, fireEvent, waitFor} from '@testing-library/react'
+import {describe, expect, it, beforeEach, afterEach, vi} from 'vitest'
 import userEvent from '@testing-library/user-event'
 import {Dialog} from './Dialog'
 import MatchMediaMock from 'jest-matchmedia-mock'
-import {behavesAsComponent, checkExports} from '../utils/testing'
-import axe from 'axe-core'
 import {Button} from '../Button'
 
 let matchMedia: MatchMediaMock
@@ -18,21 +17,6 @@ describe('Dialog', () => {
     matchMedia.clear()
   })
 
-  behavesAsComponent({
-    Component: Dialog,
-    options: {skipAs: true, skipSx: true, skipClassName: true},
-    toRender: () => (
-      <Dialog onClose={() => {}}>
-        <div>Hidden when narrow</div>
-      </Dialog>
-    ),
-  })
-
-  checkExports('Dialog/Dialog', {
-    default: undefined,
-    Dialog,
-  })
-
   it('renders with role "dialog" by default', () => {
     const {getByRole} = render(<Dialog onClose={() => {}}>Pay attention to me</Dialog>)
 
@@ -60,7 +44,7 @@ describe('Dialog', () => {
 
   it('calls `onClose` when clicking the close button', async () => {
     const user = userEvent.setup()
-    const onClose = jest.fn()
+    const onClose = vi.fn()
     const {getByLabelText} = render(<Dialog onClose={onClose}>Pay attention to me</Dialog>)
 
     expect(onClose).not.toHaveBeenCalled()
@@ -73,7 +57,7 @@ describe('Dialog', () => {
 
   it('calls `onClose` when clicking the backdrop', async () => {
     const user = userEvent.setup()
-    const onClose = jest.fn()
+    const onClose = vi.fn()
     const {getByRole} = render(<Dialog onClose={onClose}>Pay attention to me</Dialog>)
 
     expect(onClose).not.toHaveBeenCalled()
@@ -86,7 +70,7 @@ describe('Dialog', () => {
   })
 
   it('does not call `onClose` when click was not originated from backdrop', async () => {
-    const onClose = jest.fn()
+    const onClose = vi.fn()
 
     const {getByRole} = render(<Dialog onClose={onClose}>Pay attention to me</Dialog>)
 
@@ -105,13 +89,14 @@ describe('Dialog', () => {
 
   it('calls `onClose` when keying "Escape"', async () => {
     const user = userEvent.setup()
-    const onClose = jest.fn()
+    const onClose = vi.fn()
 
     render(<Dialog onClose={onClose}>Pay attention to me</Dialog>)
 
     expect(onClose).not.toHaveBeenCalled()
 
-    await user.keyboard('{Escape}')
+    await user.keyboard('{Escape}') // escape once to remove focus from the close button
+    await user.keyboard('{Escape}') // escape again to trigger the onClose
 
     expect(onClose).toHaveBeenCalledWith('escape')
   })
@@ -121,21 +106,15 @@ describe('Dialog', () => {
 
     const {container} = render(<Dialog onClose={() => {}}>Pay attention to me</Dialog>)
 
-    expect(container.ownerDocument.body.style.overflow).toBe('hidden')
+    expect(container.ownerDocument.body).toHaveStyle('overflow: hidden')
   })
 
   it('does not attempt to change the <body> style for `overflow` if it is already set to "hidden"', () => {
     document.body.style.overflow = 'hidden'
 
     const {container} = render(<Dialog onClose={() => {}}>Pay attention to me</Dialog>)
 
-    expect(container.ownerDocument.body.style.overflow).toBe('hidden')
-  })
-
-  it('should have no axe violations', async () => {
-    const {container} = render(<Dialog onClose={() => {}}>Pay attention to me</Dialog>)
-    const results = await axe.run(container)
-    expect(results).toHaveNoViolations()
+    expect(container.ownerDocument.body).toHaveStyle('overflow: hidden')
   })
 
   it('renders with data-position-regular="left" when position="left"', () => {

packages/react/src/Dialog/Dialog.tsx

@@ -288,19 +288,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
   )
 
   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
-    }
+    document.body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - document.body.clientWidth}px`)
   }, [])
 
   const header = (renderHeader ?? DefaultHeader)(defaultedProps)
@@ -339,7 +327,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
             data-width={width}
             data-height={height}
             sx={sx}
-            className={clsx(className, classes.Dialog)}
+            className={clsx(className, classes.Dialog, classes.DisableScroll)}
           >
             {header}
             <ScrollableRegion aria-labelledby={dialogLabelId} className={classes.DialogOverflowWrapper}>

packages/react/vitest.config.browser.mts

@@ -41,6 +41,7 @@ export default defineConfig({
       'src/CircleBadge/**/*.test.?(c|m)[jt]s?(x)',
       'src/CircleOcticon/**/*.test.?(c|m)[jt]s?(x)',
       'src/DataTable/**/*.test.?(c|m)[jt]s?(x)',
+      'src/Dialog/**/*.test.?(c|m)[jt]s?(x)',
       'src/FeatureFlags/**/*.test.?(c|m)[jt]s?(x)',
       'src/Flash/**/*.test.?(c|m)[jt]s?(x)',
       'src/FormControl/__tests__/FormControl.Validation.test.tsx',