Skip to content

Commit a305fed

Browse files
Copilotmattcosta7
andauthored
feat: Add feature flag for Dialog scroll performance optimization (#7366)
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mattcosta7 <[email protected]>
1 parent be7ab8e commit a305fed

File tree

5 files changed

+161
-16
lines changed

5 files changed

+161
-16
lines changed

.changeset/perf-dialog-has-selector.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
'@primer/react': patch
33
---
44

5-
perf(Dialog): Replace body:has() with direct class and scope footer selector
5+
perf(Dialog): Add feature flag for CSS :has() selector performance optimization
66

7-
- Replace `body:has(.Dialog.DisableScroll)` with direct `body.DialogScrollDisabled` class
8-
- Scope `:has(.Footer)` to direct child with `>` combinator for O(1) lookup
7+
- Add `primer_react_css_has_selector_perf` feature flag (default: false)
8+
- When flag is OFF: uses legacy `body:has(.Dialog.DisableScroll)` selector
9+
- When flag is ON: uses optimized direct `body.DialogScrollDisabled` class with ref counting
10+
- Scope footer `:has(.Footer)` to direct child with `>` combinator for O(1) lookup
11+
- Enables gradual rollout and easy rollback of performance optimization

packages/react/src/Dialog/Dialog.module.css

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,26 @@
229229
}
230230
}
231231

232+
/* DisableScroll class is added to Dialog when scroll should be disabled on body */
233+
.DisableScroll {
234+
/* This class is used as a selector target for the legacy :has() CSS selector */
235+
}
236+
237+
/*
238+
* LEGACY: Scoped :has() selector with negation guard
239+
* Only evaluates when data-dialog-scroll-optimized is NOT present on body.
240+
* When the attribute IS present (flag ON), browser skips :has() evaluation
241+
* because the :not() check fails first (O(1) attribute lookup).
242+
*/
243+
body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll) {
244+
/* stylelint-disable-next-line primer/spacing */
245+
padding-right: var(--prc-dialog-scrollgutter) !important;
246+
overflow: hidden !important;
247+
}
248+
232249
/*
233-
* PERFORMANCE: Using a direct class on body instead of body:has(.Dialog.DisableScroll)
234-
* The :has() selector forces the browser to scan the entire DOM on every style recalc,
235-
* which is O(n) where n is the number of DOM nodes. This is expensive on large pages.
250+
* PERFORMANCE OPTIMIZATION: Direct class on body - O(1) lookup
251+
* Active when primer_react_css_has_selector_perf flag is ON
236252
*/
237253
body:global(.DialogScrollDisabled) {
238254
/* stylelint-disable-next-line primer/spacing */

packages/react/src/Dialog/Dialog.test.tsx

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {Dialog} from './Dialog'
66
import {Button} from '../Button'
77
import {implementsClassName} from '../utils/testing'
88
import classes from './Dialog.module.css'
9+
import {FeatureFlags} from '../FeatureFlags'
910

1011
describe('Dialog', () => {
1112
implementsClassName(Dialog, classes.Dialog)
@@ -338,4 +339,107 @@ describe('Footer button loading states', () => {
338339
expect(publishButton).toHaveAttribute('data-loading', 'true')
339340
expect(deleteButton).not.toHaveAttribute('data-loading', 'true')
340341
})
342+
343+
describe('primer_react_css_has_selector_perf feature flag', () => {
344+
it('does not add data-dialog-scroll-optimized attribute when flag is OFF', () => {
345+
const {unmount} = render(
346+
<FeatureFlags flags={{primer_react_css_has_selector_perf: false}}>
347+
<Dialog onClose={() => {}}>Dialog content</Dialog>
348+
</FeatureFlags>,
349+
)
350+
351+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)
352+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
353+
354+
unmount()
355+
356+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false)
357+
})
358+
359+
it('adds data-dialog-scroll-optimized attribute when flag is ON', () => {
360+
const {unmount} = render(
361+
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
362+
<Dialog onClose={() => {}}>Dialog content</Dialog>
363+
</FeatureFlags>,
364+
)
365+
366+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)
367+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
368+
369+
unmount()
370+
371+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)
372+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false)
373+
})
374+
375+
it('handles multiple dialogs with ref counting when flag is ON', () => {
376+
const {unmount: unmount1} = render(
377+
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
378+
<Dialog onClose={() => {}}>Dialog 1</Dialog>
379+
</FeatureFlags>,
380+
)
381+
382+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)
383+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
384+
385+
// Render second dialog
386+
const {unmount: unmount2} = render(
387+
<FeatureFlags flags={{primer_react_css_has_selector_perf: true}}>
388+
<Dialog onClose={() => {}}>Dialog 2</Dialog>
389+
</FeatureFlags>,
390+
)
391+
392+
// Attribute should still be present
393+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)
394+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
395+
396+
// Unmount first dialog
397+
unmount1()
398+
399+
// Attribute and class should still be present (second dialog is still open)
400+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(true)
401+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
402+
403+
// Unmount second dialog
404+
unmount2()
405+
406+
// Now both should be removed
407+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)
408+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false)
409+
})
410+
411+
it('handles multiple dialogs correctly when flag is OFF', () => {
412+
const {unmount: unmount1} = render(
413+
<FeatureFlags flags={{primer_react_css_has_selector_perf: false}}>
414+
<Dialog onClose={() => {}}>Dialog 1</Dialog>
415+
</FeatureFlags>,
416+
)
417+
418+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)
419+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
420+
421+
// Render second dialog
422+
const {unmount: unmount2} = render(
423+
<FeatureFlags flags={{primer_react_css_has_selector_perf: false}}>
424+
<Dialog onClose={() => {}}>Dialog 2</Dialog>
425+
</FeatureFlags>,
426+
)
427+
428+
// Attribute should not be present, class should be present
429+
expect(document.body.hasAttribute('data-dialog-scroll-optimized')).toBe(false)
430+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
431+
432+
// Unmount first dialog
433+
unmount1()
434+
435+
// Class should still be present (second dialog is still open)
436+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(true)
437+
438+
// Unmount second dialog
439+
unmount2()
440+
441+
// Class should be removed
442+
expect(document.body.classList.contains('DialogScrollDisabled')).toBe(false)
443+
})
444+
})
341445
})

packages/react/src/Dialog/Dialog.tsx

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti
1616
import classes from './Dialog.module.css'
1717
import {clsx} from 'clsx'
1818
import {useSlots} from '../hooks/useSlots'
19+
import {useFeatureFlag} from '../FeatureFlags'
1920

2021
/* Dialog Version 2 */
2122

@@ -289,22 +290,42 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
289290
[onClose],
290291
)
291292

293+
const usePerfOptimization = useFeatureFlag('primer_react_css_has_selector_perf')
294+
292295
React.useEffect(() => {
293296
const scrollbarWidth = window.innerWidth - document.body.clientWidth
294-
// If the dialog is rendered, we add a class to the dialog element to disable
295-
dialogRef.current?.classList.add(classes.DisableScroll)
296-
// and set a CSS variable to the scrollbar width so that the dialog can
297-
// account for the scrollbar width when calculating its width.
297+
const dialog = dialogRef.current
298+
299+
// Add DisableScroll class to this dialog
300+
dialog?.classList.add(classes.DisableScroll)
298301
document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)
299302

300-
// Add class directly to body for scroll disabling (instead of using CSS :has() selector)
301-
// This avoids expensive DOM-wide style recalcs on every interaction
302-
document.body.classList.add('DialogScrollDisabled')
303+
if (usePerfOptimization) {
304+
// Optimized path: set attribute and class on body
305+
document.body.setAttribute('data-dialog-scroll-optimized', '')
306+
document.body.classList.add('DialogScrollDisabled')
307+
} else {
308+
// Legacy path: only add class (CSS :has() selector handles the rest)
309+
document.body.classList.add('DialogScrollDisabled')
310+
}
311+
303312
return () => {
304-
document.body.classList.remove('DialogScrollDisabled')
305-
document.body.style.removeProperty('--prc-dialog-scrollgutter')
313+
// Remove DisableScroll class from this dialog
314+
dialog?.classList.remove(classes.DisableScroll)
315+
316+
// Query DOM to check if any other dialogs with DisableScroll remain
317+
const remainingDialogs = document.querySelectorAll(`.${classes.DisableScroll}`)
318+
319+
if (remainingDialogs.length === 0) {
320+
// No more dialogs open, clean up body
321+
document.body.style.removeProperty('--prc-dialog-scrollgutter')
322+
document.body.classList.remove('DialogScrollDisabled')
323+
if (usePerfOptimization) {
324+
document.body.removeAttribute('data-dialog-scroll-optimized')
325+
}
326+
}
306327
}
307-
}, [])
328+
}, [usePerfOptimization])
308329

309330
const header = slots.header ?? (renderHeader ?? DefaultHeader)(defaultedProps)
310331
const body = slots.body ?? (renderBody ?? DefaultBody)({...defaultedProps, children: childrenWithoutSlots})

packages/react/src/FeatureFlags/DefaultFeatureFlags.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {FeatureFlagScope} from './FeatureFlagScope'
33
export const DefaultFeatureFlags = FeatureFlagScope.create({
44
primer_react_action_list_item_as_button: false,
55
primer_react_breadcrumbs_overflow_menu: false,
6+
primer_react_css_has_selector_perf: false,
67
primer_react_overlay_overflow: false,
78
primer_react_select_panel_fullscreen_on_narrow: false,
89
primer_react_select_panel_order_selected_at_top: false,

0 commit comments

Comments
 (0)