Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 25, 2025

Adds primer_react_css_has_selector_perf feature flag to gate the Dialog scroll optimization, enabling gradual rollout and easy rollback.

Changelog

New

  • primer_react_css_has_selector_perf feature flag (defaults to false)
  • Ref counting for multiple simultaneous dialogs (both optimized and legacy paths)
  • .DisableScroll CSS class for :has() selector target
  • Tests verifying both flag states and multiple dialog scenarios

Changed

  • Dialog scroll disabling now conditionally applies optimization based on feature flag
  • CSS uses scoped :has() selector with :not([data-dialog-scroll-optimized]) guard
  • When flag is ON: adds data-dialog-scroll-optimized attribute to body, short-circuits :has() evaluation via O(1) attribute check

Removed

  • N/A

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Rollout plan:

  1. Ship with flag OFF (legacy :has() behavior)
  2. Enable in staging/canary
  3. Gradually enable in production
  4. Make ON the default once validated

Testing & Reviewing

Flag OFF (legacy): Body selector body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll) matches because attribute is absent. Uses expensive :has().

Flag ON (optimized): Attribute data-dialog-scroll-optimized present on body causes :not() to fail immediately. Browser skips :has() evaluation. Direct class body.DialogScrollDisabled handles scroll disabling.

Both paths use ref counting for multiple dialogs (critical for mixed Turbo/React).

Merge checklist

Original prompt

Summary

Add a feature flag primer_react_css_has_selector_perf to safely gate the Dialog scroll performance optimization introduced in #7329. This allows for gradual rollout and easy rollback if issues arise.

Problem

The current implementation in perf/dialog-has-selector branch directly applies the performance optimization without a feature flag. While the optimization is valuable (replacing expensive body:has(.Dialog.DisableScroll) with a direct class), we need a way to:

  1. Gradually roll out the change
  2. Easily rollback if regressions occur
  3. Handle mixed Turbo/React architecture where multiple dialogs may be open

Solution

Use a body:not([data-dialog-scroll-optimized]) scoping approach:

When feature flag is OFF (default - legacy behavior):

  • No attribute added to <body>
  • CSS selector body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll) matches
  • Legacy :has() behavior is used

When feature flag is ON (optimized):

  • Add data-dialog-scroll-optimized attribute to <body>
  • Add DialogScrollDisabled class to <body>
  • CSS selector body:not([data-dialog-scroll-optimized]):has(...) does NOT match because the attribute is present
  • Browser skips :has() evaluation entirely (O(1) attribute check fails first)
  • The body:global(.DialogScrollDisabled) rule handles scroll disabling

Changes Required

1. Add feature flag to packages/react/src/FeatureFlags/DefaultFeatureFlags.ts

export const DefaultFeatureFlags = FeatureFlagScope.create({
  // ... existing flags ...
  primer_react_css_has_selector_perf: false,
})

2. Update packages/react/src/Dialog/Dialog.tsx

  • Import useFeatureFlag from ../FeatureFlags
  • Add ref counting to handle multiple dialogs (for Turbo/mixed architecture)
  • When flag is ON: add data-dialog-scroll-optimized attribute and DialogScrollDisabled class to body
  • When flag is OFF: no attribute (legacy :has() selector works)
  • Ensure proper cleanup on unmount with ref counting
import {useFeatureFlag} from '../FeatureFlags'

// Ref counting to handle multiple dialogs
let optimizedScrollRefCount = 0

function enableOptimizedScroll() {
  optimizedScrollRefCount++
  if (optimizedScrollRefCount === 1) {
    document.body.setAttribute('data-dialog-scroll-optimized', '')
  }
  document.body.classList.add('DialogScrollDisabled')
}

function disableOptimizedScroll() {
  optimizedScrollRefCount--
  document.body.classList.remove('DialogScrollDisabled')
  if (optimizedScrollRefCount === 0) {
    document.body.removeAttribute('data-dialog-scroll-optimized')
  }
}

In the _Dialog component's useEffect:

const usePerfOptimization = useFeatureFlag('primer_react_css_has_selector_perf')

React.useEffect(() => {
  const scrollbarWidth = window.innerWidth - document.body.clientWidth
  dialogRef.current?.classList.add(classes.DisableScroll)
  document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)

  if (usePerfOptimization) {
    enableOptimizedScroll()
  }

  return () => {
    if (usePerfOptimization) {
      disableOptimizedScroll()
    }
    document.body.style.removeProperty('--prc-dialog-scrollgutter')
  }
}, [usePerfOptimization])

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

Add back the legacy :has() selector, but scoped with :not():

/*
 * 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 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 */
  padding-right: var(--prc-dialog-scrollgutter) !important;
  overflow: hidden !important;
}

4. Update the changeset

Update .changeset/perf-dialog-has-selector.md to reflect the feature flag approach.

Testing

  • Add tests to verify both code paths work correctly
  • Test with feature flag ON and OFF
  • Test multiple dialogs open simultaneously
  • Test cleanup on unmount

Rollout Plan

  1. Ship with flag defaulting to false
  2. Enable in staging/canary environments
  3. Gradually enable in production
  4. Once validated, consider making true the default

This pull request was created from Copilot chat.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@changeset-bot
Copy link

changeset-bot bot commented Dec 25, 2025

⚠️ No Changeset found

Latest commit: 5c7c5e5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copilot AI and others added 3 commits December 25, 2025 06:03
- Add primer_react_css_has_selector_perf feature flag (default: false)
- Implement ref counting for multiple dialogs (both optimized and legacy paths)
- Add legacy :has() selector with :not() guard for backward compatibility
- Add comprehensive tests for both flag ON and OFF states
- Ensure proper cleanup on unmount with ref counting

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
- Add .DisableScroll class definition to Dialog.module.css for :has() selector
- Add comment explaining thread safety in JavaScript's single-threaded environment
- Ensure CSS selector .Dialog.DisableScroll works correctly

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
- Remove DisableScroll class from dialog element in cleanup function
- Capture dialogRef.current in variable to satisfy react-hooks/exhaustive-deps
- Add clarifying comments about module-level state safety in browser/test environments

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Copilot AI changed the title [WIP] Add feature flag for dialog scroll optimization feat: Add feature flag for Dialog scroll performance optimization Dec 25, 2025
Copilot AI requested a review from mattcosta7 December 25, 2025 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants