diff --git a/.changeset/graduate-css-has-selector-perf-flag.md b/.changeset/graduate-css-has-selector-perf-flag.md new file mode 100644 index 00000000000..0aa702353a7 --- /dev/null +++ b/.changeset/graduate-css-has-selector-perf-flag.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Graduate `primer_react_css_has_selector_perf` feature flag: the CSS `:has()` performance optimization (`body[data-dialog-scroll-disabled]`) is now the default behavior for Dialog scroll disabling diff --git a/packages/react/src/Dialog/Dialog.module.css b/packages/react/src/Dialog/Dialog.module.css index ac5a664c148..d46a02d81f0 100644 --- a/packages/react/src/Dialog/Dialog.module.css +++ b/packages/react/src/Dialog/Dialog.module.css @@ -285,27 +285,8 @@ } } -/* 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). - */ -/* stylelint-disable-next-line selector-no-qualifying-type */ -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 attribute on body - O(1) lookup - * Active when primer_react_css_has_selector_perf flag is ON */ /* stylelint-disable-next-line selector-no-qualifying-type */ body[data-dialog-scroll-disabled] { diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx index 65297c762c7..b7dc38d5f7e 100644 --- a/packages/react/src/Dialog/Dialog.test.tsx +++ b/packages/react/src/Dialog/Dialog.test.tsx @@ -1,19 +1,13 @@ 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' 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) @@ -376,126 +370,32 @@ describe('Footer button loading states', () => { expect(deleteButton).not.toHaveAttribute('data-loading', 'true') }) - describe('primer_react_css_has_selector_perf feature flag', () => { - it('does not add data-dialog-scroll-optimized or data-dialog-scroll-disabled when flag is OFF', () => { - const {unmount} = render( - - {}}>Dialog content - , - ) - - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) + describe('scroll disable behavior', () => { + it('sets data-dialog-scroll-disabled on body when dialog mounts', () => { + const {unmount} = render( {}}>Dialog content) - unmount() - - 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 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( - - {}}>Dialog 1 - , - ) + it('handles multiple dialogs with ref counting', () => { + const {unmount: unmount1} = render( {}}>Dialog 1) - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true) expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(true) - // Render second dialog - const {unmount: unmount2} = render( - - {}}>Dialog 2 - , - ) + const {unmount: unmount2} = render( {}}>Dialog 2) - // 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 + // Unmount first dialog - attribute should still be present unmount1() - - // 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) - // Unmount second dialog - unmount2() - - // Now both should be removed - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).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.hasAttribute('data-dialog-scroll-disabled')).toBe(false) - - // Render second dialog - const {unmount: unmount2} = render( - - {}}>Dialog 2 - , - ) - - // Attributes should not be present - expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false) - expect(document.body.hasAttribute('data-dialog-scroll-disabled')).toBe(false) - - // Unmount first dialog - unmount1() - - // 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 + // Unmount second dialog - attribute should be removed unmount2() - - // 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 2a070a88d66..77791ebae11 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -19,6 +19,13 @@ import {useSlots} from '../hooks/useSlots' /* Dialog Version 2 */ +/** + * Ref count for data-dialog-scroll-disabled attribute management. + * Tracks how many dialogs are currently open to know when to remove the attribute. + * This is client-only: it is only accessed inside useEffect, which never runs on the server. + */ +let dialogScrollDisabledCount = 0 + /** * Props that characterize a button to be rendered into the footer of * a Dialog. @@ -267,7 +274,6 @@ const _Dialog = React.forwardRef(null) for (const footerButton of footerButtons) { if (footerButton.autoFocus) { - // eslint-disable-next-line react-hooks/immutability footerButton.ref = autoFocusedFooterButtonRef } } @@ -308,29 +314,16 @@ 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) + dialogScrollDisabledCount++ document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`) - - if (usePerfOptimization) { - // Optimized path: set attribute on body for direct CSS targeting - document.body.setAttribute('data-dialog-scroll-disabled', '') - } - // Legacy path: no action needed - CSS :has(.Dialog.DisableScroll) handles it + document.body.setAttribute('data-dialog-scroll-disabled', '') return () => { - dialog?.classList.remove(classes.DisableScroll) - - const remainingDialogs = document.querySelectorAll(`.${classes.DisableScroll}`) - - if (remainingDialogs.length === 0) { + dialogScrollDisabledCount-- + if (dialogScrollDisabledCount === 0) { document.body.style.removeProperty('--prc-dialog-scrollgutter') - if (usePerfOptimization) { - document.body.removeAttribute('data-dialog-scroll-disabled') - } + document.body.removeAttribute('data-dialog-scroll-disabled') } } }, []) diff --git a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts index 796448887e8..95bf167f84a 100644 --- a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts +++ b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts @@ -2,7 +2,6 @@ import {FeatureFlagScope} from './FeatureFlagScope' export const DefaultFeatureFlags = FeatureFlagScope.create({ primer_react_breadcrumbs_overflow_menu: false, - primer_react_css_has_selector_perf: false, primer_react_select_panel_fullscreen_on_narrow: false, primer_react_select_panel_order_selected_at_top: false, primer_react_select_panel_remove_active_descendant: false, diff --git a/packages/react/src/FeatureFlags/FeatureFlags.tsx b/packages/react/src/FeatureFlags/FeatureFlags.tsx index c0e2ac4798f..a3f34efeb7d 100644 --- a/packages/react/src/FeatureFlags/FeatureFlags.tsx +++ b/packages/react/src/FeatureFlags/FeatureFlags.tsx @@ -2,34 +2,11 @@ import type React from 'react' import {useContext, useMemo} from 'react' import {FeatureFlagContext} from './FeatureFlagContext' import {FeatureFlagScope, type FeatureFlags} from './FeatureFlagScope' -import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' 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(() => { @@ -37,22 +14,5 @@ export function FeatureFlags({children, flags}: FeatureFlagsProps) { 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 - useIsomorphicLayoutEffect(() => { - 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 035b43bf0eb..cc7b69e802f 100644 --- a/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx +++ b/packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx @@ -1,14 +1,8 @@ -import {describe, expect, it, beforeEach} from 'vitest' +import {describe, expect, it} 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 = [] @@ -48,188 +42,4 @@ 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) - }) - }) })