From 4580808a2aecdb5b0638cb4f6ca70942e57fceb3 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 15 Dec 2025 14:53:56 +0000 Subject: [PATCH 1/5] perf(Dialog): Replace body:has() with direct class and scope footer selector - Replace body:has(.Dialog.DisableScroll) with direct body.DialogScrollDisabled class - Scope :has(.Footer) to direct child with > combinator for O(1) lookup Part of #7312 --- .changeset/perf-dialog-has-selector.md | 8 ++++++++ packages/react/src/Dialog/Dialog.module.css | 10 ++++++++-- packages/react/src/Dialog/Dialog.tsx | 8 ++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 .changeset/perf-dialog-has-selector.md diff --git a/.changeset/perf-dialog-has-selector.md b/.changeset/perf-dialog-has-selector.md new file mode 100644 index 00000000000..c90404ca952 --- /dev/null +++ b/.changeset/perf-dialog-has-selector.md @@ -0,0 +1,8 @@ +--- +'@primer/react': patch +--- + +perf(Dialog): Replace body:has() with direct class and scope footer selector + +- Replace `body:has(.Dialog.DisableScroll)` with direct `body.DialogScrollDisabled` class +- Scope `:has(.Footer)` to direct child with `>` combinator for O(1) lookup diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index d750854c158..65bc9b6516e 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -229,7 +229,12 @@ } } -body:has(.Dialog.DisableScroll) { +/* + * 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. + */ +body:global(.DialogScrollDisabled) { /* stylelint-disable-next-line primer/spacing */ padding-right: var(--prc-dialog-scrollgutter) !important; overflow: hidden !important; @@ -244,8 +249,9 @@ Add a border between the body and footer if: - the dialog has a footer - the dialog has a body that can scroll - the browser supports the `animation-timeline` property and its `scroll()` function +PERFORMANCE: Footer is a direct child of Dialog, scope with > for O(1) lookup */ -.Dialog:has(.Footer) { +.Dialog:has(> .Footer) { --can-scroll: 0; .DialogOverflowWrapper { diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 0381c445e58..2fe0aac7dc3 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -296,6 +296,14 @@ const _Dialog = React.forwardRef { + document.body.classList.remove('DialogScrollDisabled') + document.body.style.removeProperty('--prc-dialog-scrollgutter') + } }, []) const header = slots.header ?? (renderHeader ?? DefaultHeader)(defaultedProps) From a305fedcc75fa25b67e1e01a4e2cd5d5e9050640 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 16:08:08 -0500 Subject: [PATCH 2/5] feat: Add feature flag for Dialog scroll performance optimization (#7366) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .changeset/perf-dialog-has-selector.md | 9 +- packages/react/src/Dialog/Dialog.module.css | 22 +++- packages/react/src/Dialog/Dialog.test.tsx | 104 ++++++++++++++++++ packages/react/src/Dialog/Dialog.tsx | 41 +++++-- .../src/FeatureFlags/DefaultFeatureFlags.ts | 1 + 5 files changed, 161 insertions(+), 16 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..dbf6df6f12f 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -229,10 +229,26 @@ } } +/* 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. + * 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: 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. + * 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..2d41fe267b4 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -6,6 +6,7 @@ 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,107 @@ 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', () => { + 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..39d1a58bb9d 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -16,6 +16,7 @@ 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 */ @@ -289,22 +290,42 @@ 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 - dialogRef.current?.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. + const dialog = dialogRef.current + + // Add DisableScroll class to this dialog + dialog?.classList.add(classes.DisableScroll) document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`) - // Add class directly to body for scroll disabling (instead of using CSS :has() selector) - // This avoids expensive DOM-wide style recalcs on every interaction - document.body.classList.add('DialogScrollDisabled') + if (usePerfOptimization) { + // Optimized path: set attribute and class on body + document.body.setAttribute('data-dialog-scroll-optimized', '') + document.body.classList.add('DialogScrollDisabled') + } else { + // Legacy path: only add class (CSS :has() selector handles the rest) + document.body.classList.add('DialogScrollDisabled') + } + return () => { - document.body.classList.remove('DialogScrollDisabled') - document.body.style.removeProperty('--prc-dialog-scrollgutter') + // Remove DisableScroll class from this dialog + dialog?.classList.remove(classes.DisableScroll) + + // 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]) 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 8d95278f8fe79757361da11b7e8081814b9f71f5 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Fri, 26 Dec 2025 13:26:39 -0500 Subject: [PATCH 3/5] Use data-dialog-scroll-disabled attribute instead of class when perf flag is enabled (#7376) 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.module.css | 5 +++-- packages/react/src/Dialog/Dialog.test.tsx | 12 ++++++------ packages/react/src/Dialog/Dialog.tsx | 8 +++++--- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index dbf6df6f12f..c67177f1f5c 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -247,10 +247,11 @@ body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll) { } /* - * PERFORMANCE OPTIMIZATION: Direct class on body - O(1) lookup + * PERFORMANCE OPTIMIZATION: Direct attribute on body - O(1) lookup * Active when primer_react_css_has_selector_perf flag is ON */ -body:global(.DialogScrollDisabled) { +/* stylelint-disable-next-line selector-no-qualifying-type */ +body[data-dialog-scroll-disabled] { /* stylelint-disable-next-line primer/spacing */ padding-right: var(--prc-dialog-scrollgutter) !important; overflow: hidden !important; diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index 2d41fe267b4..b6d14ca9809 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -364,12 +364,12 @@ describe('Footer button loading states', () => { ) expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) unmount() expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) }) it('handles multiple dialogs with ref counting when flag is ON', () => { @@ -380,7 +380,7 @@ describe('Footer button loading states', () => { ) expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) // Render second dialog const {unmount: unmount2} = render( @@ -391,21 +391,21 @@ describe('Footer button loading states', () => { // Attribute should still be present expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).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) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).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) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) }) it('handles multiple dialogs correctly when flag is OFF', () => { diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 39d1a58bb9d..630e5b60a28 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -301,9 +301,9 @@ const _Dialog = React.forwardRef Date: Mon, 29 Dec 2025 23:50:19 -0500 Subject: [PATCH 4/5] perf(Dialog): Move data-dialog-scroll-optimized to FeatureFlags provider level (#7393) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .changeset/perf-dialog-has-selector.md | 3 +- packages/react/src/Dialog/Dialog.module.css | 3 +- packages/react/src/Dialog/Dialog.test.tsx | 54 +++-- packages/react/src/Dialog/Dialog.tsx | 15 +- .../react/src/FeatureFlags/FeatureFlags.tsx | 42 +++- .../__tests__/FeatureFlags.test.tsx | 192 +++++++++++++++++- 6 files changed, 277 insertions(+), 32 deletions(-) diff --git a/.changeset/perf-dialog-has-selector.md b/.changeset/perf-dialog-has-selector.md index 631826c682e..bc6b83d2b5e 100644 --- a/.changeset/perf-dialog-has-selector.md +++ b/.changeset/perf-dialog-has-selector.md @@ -6,6 +6,5 @@ perf(Dialog): Add feature flag for CSS :has() selector performance optimization - 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 +- When flag is ON: uses optimized direct `body[data-dialog-scroll-disabled]` data attribute with ref counting - 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 c67177f1f5c..a81c278bdbe 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -266,9 +266,8 @@ Add a border between the body and footer if: - the dialog has a footer - the dialog has a body that can scroll - the browser supports the `animation-timeline` property and its `scroll()` function -PERFORMANCE: Footer is a direct child of Dialog, scope with > for O(1) lookup */ -.Dialog:has(> .Footer) { +.Dialog:has(.Footer) { --can-scroll: 0; .DialogOverflowWrapper { diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index b6d14ca9809..3ce178375ab 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -1,14 +1,19 @@ 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' +import {__resetDialogScrollOptimizedCount} from '../FeatureFlags/FeatureFlags' describe('Dialog', () => { + beforeEach(() => { + __resetDialogScrollOptimizedCount() + }) + implementsClassName(Dialog, classes.Dialog) it('renders with role "dialog" by default', () => { const {getByRole} = render( {}}>Pay attention to me) @@ -341,7 +346,7 @@ describe('Footer button loading states', () => { }) describe('primer_react_css_has_selector_perf feature flag', () => { - it('does not add data-dialog-scroll-optimized attribute when flag is OFF', () => { + it('does not add data-dialog-scroll-optimized or data-dialog-scroll-disabled when flag is OFF', () => { const {unmount} = render( {}}>Dialog content @@ -349,29 +354,48 @@ describe('Footer button loading states', () => { ) expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) unmount() - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) }) - it('adds data-dialog-scroll-optimized attribute when flag is ON', () => { + it('adds data-dialog-scroll-optimized at provider level and data-dialog-scroll-disabled when dialog mounts', () => { const {unmount} = render( {}}>Dialog content , ) + // Provider sets data-dialog-scroll-optimized, Dialog sets data-dialog-scroll-disabled expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) unmount() + // Both should be removed on unmount expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) }) + it('sets data-dialog-scroll-optimized even when no dialogs are open', () => { + const {unmount} = render( + +
No dialogs here
+ , + ) + + // Provider sets the attribute even without dialogs + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + it('handles multiple dialogs with ref counting when flag is ON', () => { const {unmount: unmount1} = render( @@ -389,14 +413,14 @@ describe('Footer button loading states', () => { , ) - // Attribute should still be present + // Attributes should still be present expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) // Unmount first dialog unmount1() - // Attribute and class should still be present (second dialog is still open) + // Attributes should still be present (second dialog and provider are still mounted) expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) @@ -416,7 +440,7 @@ describe('Footer button loading states', () => { ) expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) // Render second dialog const {unmount: unmount2} = render( @@ -425,21 +449,23 @@ describe('Footer button loading states', () => { , ) - // Attribute should not be present, class should be present + // Attributes should not be present expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) // Unmount first dialog unmount1() - // Class should still be present (second dialog is still open) - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true) + // Attributes should still not be present + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) // Unmount second dialog unmount2() - // Class should be removed - expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false) + // Attributes should still not be present + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) }) }) }) diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 630e5b60a28..0f86f6b51de 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -296,34 +296,25 @@ const _Dialog = React.forwardRef { - // Remove DisableScroll class from this dialog dialog?.classList.remove(classes.DisableScroll) - // 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') if (usePerfOptimization) { - document.body.removeAttribute('data-dialog-scroll-optimized') document.body.removeAttribute('data-dialog-scroll-disabled') - } else { - document.body.classList.remove('DialogScrollDisabled') } } } diff --git a/packages/react/src/FeatureFlags/FeatureFlags.tsx b/packages/react/src/FeatureFlags/FeatureFlags.tsx index 2df681f5234..3cf0806cb92 100644 --- a/packages/react/src/FeatureFlags/FeatureFlags.tsx +++ b/packages/react/src/FeatureFlags/FeatureFlags.tsx @@ -1,5 +1,5 @@ import type React from 'react' -import {useContext, useMemo} from 'react' +import {useContext, useMemo, useEffect} from 'react' import {FeatureFlagContext} from './FeatureFlagContext' import {FeatureFlagScope, type FeatureFlags} from './FeatureFlagScope' @@ -7,11 +7,51 @@ export type FeatureFlagsProps = React.PropsWithChildren<{ flags: FeatureFlags }> +/** + * Ref count for data-dialog-scroll-optimized attribute management. + * + * NOTE: This is temporary infrastructure while we feature flag the CSS :has() + * performance optimization (primer_react_css_has_selector_perf). Once the flag + * is removed and the optimization is the default behavior, this ref counting + * can be removed - the attribute can simply always be present. + * + * @internal - Not part of the public API + */ +let dialogScrollOptimizedCount = 0 + +/** + * Reset the ref count for testing purposes only. + * + * @internal - Not part of the public API. Only exported for test isolation. + */ +export function __resetDialogScrollOptimizedCount(): void { + dialogScrollOptimizedCount = 0 + document.body.removeAttribute('data-dialog-scroll-optimized') +} + export function FeatureFlags({children, flags}: FeatureFlagsProps) { const parentFeatureFlags = useContext(FeatureFlagContext) const value = useMemo(() => { const scope = FeatureFlagScope.merge(parentFeatureFlags, FeatureFlagScope.create(flags)) return scope }, [parentFeatureFlags, flags]) + + const isOptimizationEnabled = value.enabled('primer_react_css_has_selector_perf') + + // Set body attribute for CSS :has() optimization when flag is enabled + useEffect(() => { + if (isOptimizationEnabled) { + dialogScrollOptimizedCount++ + document.body.setAttribute('data-dialog-scroll-optimized', '') + + return () => { + dialogScrollOptimizedCount-- + if (dialogScrollOptimizedCount === 0) { + document.body.removeAttribute('data-dialog-scroll-optimized') + } + } + } + }, [isOptimizationEnabled]) + return {children} } diff --git a/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx b/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx index cc7b69e802f..035b43bf0eb 100644 --- a/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx +++ b/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx @@ -1,8 +1,14 @@ -import {describe, expect, it} from 'vitest' +import {describe, expect, it, beforeEach} from 'vitest' import {render} from '@testing-library/react' import {FeatureFlags, useFeatureFlag} from '../../FeatureFlags' +import {__resetDialogScrollOptimizedCount} from '../FeatureFlags' describe('FeatureFlags', () => { + beforeEach(() => { + // Reset module state between tests for isolation + __resetDialogScrollOptimizedCount() + }) + it('should allow a component to check if a feature flag is enabled', () => { const calls: Array = [] @@ -42,4 +48,188 @@ describe('FeatureFlags', () => { expect(calls).toEqual([false]) }) + + describe('data-dialog-scroll-optimized attribute management', () => { + it('should set data-dialog-scroll-optimized attribute when primer_react_css_has_selector_perf is enabled', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + const {unmount} = render( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should not set data-dialog-scroll-optimized attribute when primer_react_css_has_selector_perf is disabled', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + const {unmount} = render( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should handle ref counting correctly with multiple FeatureFlags providers', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + // Mount first provider + const {unmount: unmount1} = render( + +
Provider 1
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Mount second provider + const {unmount: unmount2} = render( + +
Provider 2
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Unmount first provider - attribute should still be present + unmount1() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Unmount second provider - attribute should be removed + unmount2() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should handle nested providers with different flag values correctly', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + const {unmount} = render( + +
Outer provider with flag enabled
+ +
Inner provider with flag disabled
+ + , + ) + + // Outer provider sets the attribute, inner provider inherits but doesn't override + // (inner provider flag is false, so it won't add to count) + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should handle nested providers where parent has flag disabled and child has flag enabled', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + const {unmount} = render( + +
Outer provider with flag disabled
+ +
Inner provider with flag enabled
+ + , + ) + + // Inner provider enables the flag, so attribute should be set + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + unmount() + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should only remove attribute when all providers with flag enabled have unmounted', () => { + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + // Mount three providers with flag enabled + const {unmount: unmount1} = render( + +
Provider 1
+ , + ) + + const {unmount: unmount2} = render( + +
Provider 2
+ , + ) + + const {unmount: unmount3} = render( + +
Provider 3
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Unmount first provider + unmount1() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Unmount second provider + unmount2() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + // Unmount third provider - now attribute should be removed + unmount3() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should handle flag value changing from false to true', () => { + const {rerender, unmount} = render( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + rerender( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + unmount() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + + it('should handle flag value changing from true to false', () => { + const {rerender, unmount} = render( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) + + rerender( + +
Content
+ , + ) + + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + + unmount() + expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) + }) + }) }) From 36ecba67bdebf4c87fe29ed5c695d2331f6a4623 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Dec 2025 11:29:35 -0500 Subject: [PATCH 5/5] Refactor Dialog to check DOM attribute instead of useFeatureFlag hook (#7398) 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.tsx | 6 ++---- packages/react/src/FeatureFlags/FeatureFlags.tsx | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 0f86f6b51de..41bfa481dd0 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -16,7 +16,6 @@ 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 */ @@ -290,11 +289,10 @@ const _Dialog = React.forwardRef { const scrollbarWidth = window.innerWidth - document.body.clientWidth const dialog = dialogRef.current + const usePerfOptimization = document.body.hasAttribute('data-dialog-scroll-optimized') // Add DisableScroll class to this dialog (for legacy :has() selector path) dialog?.classList.add(classes.DisableScroll) @@ -318,7 +316,7 @@ const _Dialog = React.forwardRef { + useIsomorphicLayoutEffect(() => { if (isOptimizationEnabled) { dialogScrollOptimizedCount++ document.body.setAttribute('data-dialog-scroll-optimized', '')