From b3697bf41242e9037bb16a5642fe6517ce30ea39 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 19:07:51 +0000 Subject: [PATCH 1/2] Initial plan From c00c86048af9141043fd516ce0bceef290a79104 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 19:17:48 +0000 Subject: [PATCH 2/2] Refactor Dialog scroll optimization to use DOM queries - Remove module-level ref counting variables (optimizedScrollRefCount, legacyScrollRefCount) - Remove helper functions (enableOptimizedScroll, disableOptimizedScroll, enableLegacyScroll, disableLegacyScroll) - Update useEffect to use DOM query (querySelectorAll) to check for remaining dialogs - Remove beforeEach cleanup in tests as it's no longer needed - Tests pass: DOM automatically resets between tests, eliminating test pollution Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- packages/react/src/Dialog/Dialog.test.tsx | 8 +-- packages/react/src/Dialog/Dialog.tsx | 74 ++++++----------------- 2 files changed, 21 insertions(+), 61 deletions(-) diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index b399422fce4..2d41fe267b4 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -1,6 +1,6 @@ import React from 'react' import {render, fireEvent, waitFor} from '@testing-library/react' -import {describe, expect, it, vi, beforeEach} from 'vitest' +import {describe, expect, it, vi} from 'vitest' import userEvent from '@testing-library/user-event' import {Dialog} from './Dialog' import {Button} from '../Button' @@ -341,12 +341,6 @@ describe('Footer button loading states', () => { }) describe('primer_react_css_has_selector_perf feature flag', () => { - beforeEach(() => { - // Clean up body attributes and classes before each test - document.body.removeAttribute('data-dialog-scroll-optimized') - document.body.classList.remove('DialogScrollDisabled') - }) - it('does not add data-dialog-scroll-optimized attribute when flag is OFF', () => { const {unmount} = render( diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 000b582838c..39d1a58bb9d 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -20,47 +20,6 @@ import {useFeatureFlag} from '../FeatureFlags' /* Dialog Version 2 */ -// Ref counting to handle multiple dialogs open simultaneously -// This is particularly important in mixed Turbo/React architecture -// Note: These functions are safe in JavaScript's single-threaded event loop. -// React effects execute synchronously within each render cycle. -// Module-level state is appropriate here because: -// 1. This code only runs in browser environments (document.body is required) -// 2. Ref counting needs to be shared across all Dialog instances -// 3. Tests run in isolated environments with fresh module state -let optimizedScrollRefCount = 0 -let legacyScrollRefCount = 0 - -function enableOptimizedScroll() { - optimizedScrollRefCount++ - if (optimizedScrollRefCount === 1) { - document.body.setAttribute('data-dialog-scroll-optimized', '') - document.body.classList.add('DialogScrollDisabled') - } -} - -function disableOptimizedScroll() { - optimizedScrollRefCount-- - if (optimizedScrollRefCount === 0) { - document.body.classList.remove('DialogScrollDisabled') - document.body.removeAttribute('data-dialog-scroll-optimized') - } -} - -function enableLegacyScroll() { - legacyScrollRefCount++ - if (legacyScrollRefCount === 1) { - document.body.classList.add('DialogScrollDisabled') - } -} - -function disableLegacyScroll() { - legacyScrollRefCount-- - if (legacyScrollRefCount === 0) { - document.body.classList.remove('DialogScrollDisabled') - } -} - /** * Props that characterize a button to be rendered into the footer of * a Dialog. @@ -336,28 +295,35 @@ const _Dialog = React.forwardRef { const scrollbarWidth = window.innerWidth - document.body.clientWidth const dialog = dialogRef.current - // If the dialog is rendered, we add a class to the dialog element to disable + + // Add DisableScroll class to this dialog dialog?.classList.add(classes.DisableScroll) - // and set a CSS variable to the scrollbar width so that the dialog can - // account for the scrollbar width when calculating its width. document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`) if (usePerfOptimization) { - enableOptimizedScroll() + // Optimized path: set attribute and class on body + document.body.setAttribute('data-dialog-scroll-optimized', '') + document.body.classList.add('DialogScrollDisabled') } else { - // Legacy behavior: add class with ref counting for scroll disabling - // This is used when the feature flag is OFF and CSS :has() selector is active - enableLegacyScroll() + // Legacy path: only add class (CSS :has() selector handles the rest) + document.body.classList.add('DialogScrollDisabled') } return () => { - if (usePerfOptimization) { - disableOptimizedScroll() - } else { - disableLegacyScroll() - } + // Remove DisableScroll class from this dialog dialog?.classList.remove(classes.DisableScroll) - document.body.style.removeProperty('--prc-dialog-scrollgutter') + + // Query DOM to check if any other dialogs with DisableScroll remain + const remainingDialogs = document.querySelectorAll(`.${classes.DisableScroll}`) + + if (remainingDialogs.length === 0) { + // No more dialogs open, clean up body + document.body.style.removeProperty('--prc-dialog-scrollgutter') + document.body.classList.remove('DialogScrollDisabled') + if (usePerfOptimization) { + document.body.removeAttribute('data-dialog-scroll-optimized') + } + } } }, [usePerfOptimization])