Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .changeset/perf-dialog-has-selector.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 1 addition & 2 deletions packages/react/src/Dialog/Dialog.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
54 changes: 40 additions & 14 deletions packages/react/src/Dialog/Dialog.test.tsx
Original file line number Diff line number Diff line change
@@ -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(<Dialog onClose={() => {}}>Pay attention to me</Dialog>)
Expand Down Expand Up @@ -341,37 +346,56 @@ 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(
<FeatureFlags flags={{primer_react_css_has_selector_perf: false}}>
<Dialog onClose={() => {}}>Dialog content</Dialog>
</FeatureFlags>,
)

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(
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
<Dialog onClose={() => {}}>Dialog content</Dialog>
</FeatureFlags>,
)

// 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(
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
<div>No dialogs here</div>
</FeatureFlags>,
)

// 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(
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
Expand All @@ -389,14 +413,14 @@ describe('Footer button loading states', () => {
</FeatureFlags>,
)

// 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)

Expand All @@ -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(
Expand All @@ -425,21 +449,23 @@ describe('Footer button loading states', () => {
</FeatureFlags>,
)

// 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)
})
})
})
15 changes: 3 additions & 12 deletions packages/react/src/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -296,34 +296,25 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
const scrollbarWidth = window.innerWidth - document.body.clientWidth
const dialog = dialogRef.current

// Add DisableScroll class to this dialog
// Add DisableScroll class to this dialog (for legacy :has() selector path)
dialog?.classList.add(classes.DisableScroll)
document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)

if (usePerfOptimization) {
// Optimized path: set attributes on body
document.body.setAttribute('data-dialog-scroll-optimized', '')
// Optimized path: set attribute on body for direct CSS targeting
document.body.setAttribute('data-dialog-scroll-disabled', '')
} else {
// Legacy path: only add class (CSS :has() selector handles the rest)
document.body.classList.add('DialogScrollDisabled')
}
// Legacy path: no action needed - CSS :has(.Dialog.DisableScroll) handles it

return () => {
// 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')
}
}
}
Expand Down
42 changes: 41 additions & 1 deletion packages/react/src/FeatureFlags/FeatureFlags.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,57 @@
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'

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

Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global ref counter dialogScrollOptimizedCount could potentially leak state between test runs if a test fails before properly unmounting components. Consider adding a beforeEach or afterEach hook in the Dialog test file that resets this counter to 0 and removes any lingering body attributes to ensure test isolation.

Example:

beforeEach(() => {
  dialogScrollOptimizedCount = 0
  document.body.removeAttribute('data-dialog-scroll-optimized')
  document.body.removeAttribute('data-dialog-scroll-disabled')
})

However, this would require exporting the counter or creating a reset function, which may not be desirable. An alternative is to use a WeakMap keyed by FeatureFlags instances instead of a global counter, though that has its own tradeoffs.

Suggested change
// Test helper: reset global counter and related body attributes between tests
// Not intended for production use.
export function __resetDialogScrollOptimizationForTests() {
dialogScrollOptimizedCount = 0
if (typeof document !== 'undefined' && document.body) {
document.body.removeAttribute('data-dialog-scroll-optimized')
document.body.removeAttribute('data-dialog-scroll-disabled')
}
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented WeakMap-based ref counting in 06aefc9. Each component instance uses a unique ref object tracked in a WeakMap, ensuring proper garbage collection and test isolation without exposing internal state or requiring test helpers.

/**
* 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 <FeatureFlagContext.Provider value={value}>{children}</FeatureFlagContext.Provider>
}
Loading
Loading