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) + }) + }) })