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
9 changes: 6 additions & 3 deletions .changeset/perf-dialog-has-selector.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
22 changes: 19 additions & 3 deletions packages/react/src/Dialog/Dialog.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
104 changes: 104 additions & 0 deletions packages/react/src/Dialog/Dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(
<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)

unmount()

expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false)
})

it('adds data-dialog-scroll-optimized attribute when flag is ON', () => {
const {unmount} = render(
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
<Dialog onClose={() => {}}>Dialog content</Dialog>
</FeatureFlags>,
)

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

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

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

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

// 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)
})
})
})
41 changes: 31 additions & 10 deletions packages/react/src/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand Down Expand Up @@ -289,22 +290,42 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
[onClose],
)

const usePerfOptimization = useFeatureFlag('primer_react_css_has_selector_perf')

React.useEffect(() => {
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})
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/FeatureFlags/DefaultFeatureFlags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading