From 64b370d0df2575fc26f7793b715ff5b76ecb04f7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 05:49:23 +0000 Subject: [PATCH 1/5] Initial plan From 7f6fc398f7c21c38819dfdfdf5e5f3b333c99716 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 06:03:04 +0000 Subject: [PATCH 2/5] feat: Add feature flag for Dialog scroll performance optimization - Add primer_react_css_has_selector_perf feature flag (default: false) - Implement ref counting for multiple dialogs (both optimized and legacy paths) - Add legacy :has() selector with :not() guard for backward compatibility - Add comprehensive tests for both flag ON and OFF states - Ensure proper cleanup on unmount with ref counting Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .changeset/perf-dialog-has-selector.md | 9 +- packages/react/src/Dialog/Dialog.module.css | 17 ++- packages/react/src/Dialog/Dialog.test.tsx | 112 +++++++++++++++++- packages/react/src/Dialog/Dialog.tsx | 57 ++++++++- .../src/FeatureFlags/DefaultFeatureFlags.ts | 1 + 5 files changed, 184 insertions(+), 12 deletions(-) diff --git a/.changeset/perf-dialog-has-selector.md b/.changeset/perf-dialog-has-selector.md index c90404ca952..631826c682e 100644 --- a/.changeset/perf-dialog-has-selector.md +++ b/.changeset/perf-dialog-has-selector.md @@ -2,7 +2,10 @@ '@primer/react': patch --- -perf(Dialog): Replace body:has() with direct class and scope footer selector +perf(Dialog): Add feature flag for CSS :has() selector performance optimization -- Replace `body:has(.Dialog.DisableScroll)` with direct `body.DialogScrollDisabled` class -- Scope `:has(.Footer)` to direct child with `>` combinator for O(1) lookup +- Add `primer_react_css_has_selector_perf` feature flag (default: false) +- When flag is OFF: uses legacy `body:has(.Dialog.DisableScroll)` selector +- When flag is ON: uses optimized direct `body.DialogScrollDisabled` class with ref counting +- Scope footer `:has(.Footer)` to direct child with `>` combinator for O(1) lookup +- Enables gradual rollout and easy rollback of performance optimization diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index 65bc9b6516e..89f39657e28 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -230,9 +230,20 @@ } /* - * PERFORMANCE: Using a direct class on body instead of body:has(.Dialog.DisableScroll) - * The :has() selector forces the browser to scan the entire DOM on every style recalc, - * which is O(n) where n is the number of DOM nodes. This is expensive on large pages. + * LEGACY: Scoped :has() selector with negation guard + * Only evaluates when data-dialog-scroll-optimized is NOT present on body. + * When the attribute IS present (flag ON), browser skips :has() evaluation + * because the :not() check fails first (O(1) attribute lookup). + */ +body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll) { + /* stylelint-disable-next-line primer/spacing */ + padding-right: var(--prc-dialog-scrollgutter) !important; + overflow: hidden !important; +} + +/* + * PERFORMANCE OPTIMIZATION: Direct class on body - O(1) lookup + * Active when primer_react_css_has_selector_perf flag is ON */ body:global(.DialogScrollDisabled) { /* stylelint-disable-next-line primer/spacing */ diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index fb693f07c15..b399422fce4 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -1,11 +1,12 @@ import React from 'react' import {render, fireEvent, waitFor} from '@testing-library/react' -import {describe, expect, it, vi} from 'vitest' +import {describe, expect, it, vi, beforeEach} from 'vitest' import userEvent from '@testing-library/user-event' import {Dialog} from './Dialog' import {Button} from '../Button' import {implementsClassName} from '../utils/testing' import classes from './Dialog.module.css' +import {FeatureFlags} from '../FeatureFlags' describe('Dialog', () => { implementsClassName(Dialog, classes.Dialog) @@ -338,4 +339,113 @@ describe('Footer button loading states', () => { expect(publishButton).toHaveAttribute('data-loading', 'true') expect(deleteButton).not.toHaveAttribute('data-loading', 'true') }) + + 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( + + {}}>Dialog content + , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + unmount() + + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + }) + + it('adds data-dialog-scroll-optimized attribute when flag is ON', () => { + const {unmount} = render( + + {}}>Dialog content + , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + }) + + it('handles multiple dialogs with ref counting when flag is ON', () => { + const {unmount: unmount1} = render( + + {}}>Dialog 1 + , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Render second dialog + const {unmount: unmount2} = render( + + {}}>Dialog 2 + , + ) + + // Attribute should still be present + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Unmount first dialog + unmount1() + + // Attribute and class should still be present (second dialog is still open) + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Unmount second dialog + unmount2() + + // Now both should be removed + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + }) + + it('handles multiple dialogs correctly when flag is OFF', () => { + const {unmount: unmount1} = render( + + {}}>Dialog 1 + , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Render second dialog + const {unmount: unmount2} = render( + + {}}>Dialog 2 + , + ) + + // Attribute should not be present, class should be present + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Unmount first dialog + unmount1() + + // Class should still be present (second dialog is still open) + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + + // Unmount second dialog + unmount2() + + // Class should be removed + expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + }) + }) }) diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 2fe0aac7dc3..b311062e6b2 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -16,9 +16,45 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti import classes from './Dialog.module.css' import {clsx} from 'clsx' import {useSlots} from '../hooks/useSlots' +import {useFeatureFlag} from '../FeatureFlags' /* Dialog Version 2 */ +// Ref counting to handle multiple dialogs open simultaneously +// This is particularly important in mixed Turbo/React architecture +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. @@ -289,6 +325,8 @@ const _Dialog = React.forwardRef { const scrollbarWidth = window.innerWidth - document.body.clientWidth // If the dialog is rendered, we add a class to the dialog element to disable @@ -297,14 +335,23 @@ const _Dialog = React.forwardRef { - document.body.classList.remove('DialogScrollDisabled') + if (usePerfOptimization) { + disableOptimizedScroll() + } else { + disableLegacyScroll() + } document.body.style.removeProperty('--prc-dialog-scrollgutter') } - }, []) + }, [usePerfOptimization]) const header = slots.header ?? (renderHeader ?? DefaultHeader)(defaultedProps) const body = slots.body ?? (renderBody ?? DefaultBody)({...defaultedProps, children: childrenWithoutSlots}) diff --git a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts index d5c1cf6ab92..4a2c0896f50 100644 --- a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts +++ b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts @@ -3,6 +3,7 @@ import {FeatureFlagScope} from './FeatureFlagScope' export const DefaultFeatureFlags = FeatureFlagScope.create({ primer_react_action_list_item_as_button: false, primer_react_breadcrumbs_overflow_menu: false, + primer_react_css_has_selector_perf: false, primer_react_overlay_overflow: false, primer_react_select_panel_fullscreen_on_narrow: false, primer_react_select_panel_order_selected_at_top: false, From 0bfcfbac009c24ef98a90d21b243c19d9180b9fc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 06:09:25 +0000 Subject: [PATCH 3/5] fix: Add DisableScroll CSS class and clarify thread safety - Add .DisableScroll class definition to Dialog.module.css for :has() selector - Add comment explaining thread safety in JavaScript's single-threaded environment - Ensure CSS selector .Dialog.DisableScroll works correctly Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- packages/react/src/Dialog/Dialog.module.css | 5 +++++ packages/react/src/Dialog/Dialog.tsx | 2 ++ 2 files changed, 7 insertions(+) diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index 89f39657e28..dbf6df6f12f 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -229,6 +229,11 @@ } } +/* DisableScroll class is added to Dialog when scroll should be disabled on body */ +.DisableScroll { + /* This class is used as a selector target for the legacy :has() CSS selector */ +} + /* * LEGACY: Scoped :has() selector with negation guard * Only evaluates when data-dialog-scroll-optimized is NOT present on body. diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index b311062e6b2..9e0ceb35a86 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -22,6 +22,8 @@ import {useFeatureFlag} from '../FeatureFlags' // 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. let optimizedScrollRefCount = 0 let legacyScrollRefCount = 0 From 5c7c5e5eae03bd2ffc3e3e775bbd8052cf41249a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 06:13:07 +0000 Subject: [PATCH 4/5] fix: Address code review feedback on cleanup and module state - Remove DisableScroll class from dialog element in cleanup function - Capture dialogRef.current in variable to satisfy react-hooks/exhaustive-deps - Add clarifying comments about module-level state safety in browser/test environments Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- packages/react/src/Dialog/Dialog.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 9e0ceb35a86..000b582838c 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -24,6 +24,10 @@ import {useFeatureFlag} from '../FeatureFlags' // 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 @@ -331,8 +335,9 @@ 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 - dialogRef.current?.classList.add(classes.DisableScroll) + 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`) @@ -351,6 +356,7 @@ const _Dialog = React.forwardRef Date: Thu, 25 Dec 2025 15:58:33 -0500 Subject: [PATCH 5/5] Refactor Dialog scroll optimization to use DOM queries instead of ref counting (#7368) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> 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])