Graduate merged forwarded refs

.changeset/merged-forwarded-refs.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Ref handling: Preserve forwarded refs while maintaining internal component refs.

packages/react/src/ActionList/ActionList.test.tsx

@@ -615,32 +615,24 @@ describe('ActionList with role="tree"', () => {
   })
 })
 
-describe('ActionList forwarded ref (primer_react_merged_forwarded_refs)', () => {
-  for (const enabled of [true, false]) {
-    describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => {
-      it('forwards a ref object to the element', () => {
-        const ref = createRef<HTMLUListElement>()
-        HTMLRender(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <ActionList ref={ref}>
-              <ActionList.Item>Item</ActionList.Item>
-            </ActionList>
-          </FeatureFlags>,
-        )
-        expect(ref.current).toBeInstanceOf(HTMLElement)
-      })
-
-      it('calls a callback ref with the element', () => {
-        const refCallback = vi.fn()
-        HTMLRender(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <ActionList ref={refCallback}>
-              <ActionList.Item>Item</ActionList.Item>
-            </ActionList>
-          </FeatureFlags>,
-        )
-        expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLElement)).toBe(true)
-      })
-    })
-  }
+describe('ActionList forwarded ref', () => {
+  it('forwards a ref object to the element', () => {
+    const ref = createRef<HTMLUListElement>()
+    HTMLRender(
+      <ActionList ref={ref}>
+        <ActionList.Item>Item</ActionList.Item>
+      </ActionList>,
+    )
+    expect(ref.current).toBeInstanceOf(HTMLElement)
+  })
+
+  it('calls a callback ref with the element', () => {
+    const refCallback = vi.fn()
+    HTMLRender(
+      <ActionList ref={refCallback}>
+        <ActionList.Item>Item</ActionList.Item>
+      </ActionList>,
+    )
+    expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLElement)).toBe(true)
+  })
 })

packages/react/src/ActionList/List.tsx

@@ -5,7 +5,7 @@ import {useSlots} from '../hooks/useSlots'
 import {Heading} from './Heading'
 import {useId} from '../hooks/useId'
 import {ListContext, type ActionListProps} from './shared'
-import {useMergedRefs, useProvidedRefOrCreate} from '../hooks'
+import {useMergedRefs} from '../hooks'
 import {FocusKeys, useFocusZone} from '../hooks/useFocusZone'
 import {clsx} from 'clsx'
 import classes from './ActionList.module.css'
@@ -44,14 +44,8 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>(
 
   const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy
   const listRole = role || listRoleFromContainer
-  const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs')
   const listRef = useRef<HTMLElement>(null)
   const mergedRef = useMergedRefs(listRef, forwardedRef)
-  // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`.
-  // At graduation: remove the three declarations below, and replace all instances of `readRef` with `listRef` and `appliedRef` with `mergedRef`.
-  const providedOrCreatedRef = useProvidedRefOrCreate(forwardedRef as React.RefObject<HTMLElement>)
-  const readRef = mergedRefEnabled ? listRef : providedOrCreatedRef
-  const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef
   const itemGapFlag = useFeatureFlag('primer_react_action_list_item_gap')
   const itemGapEnabled = itemGapFlag && !disableItemGap && container === 'NavList'
 
@@ -61,7 +55,7 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>(
 
   useFocusZone({
     disabled: !enableFocusZone,
-    containerRef: readRef,
+    containerRef: listRef,
     bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown,
     focusOutBehavior:
       listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined,
@@ -93,7 +87,7 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>(
   // Two querySelector calls after render is trivially cheap compared to what the browser
   // was doing on every DOM mutation with `:has()`.
   useIsomorphicLayoutEffect(() => {
-    const list = readRef.current
+    const list = listRef.current
     if (!list) return
     const hasMixed =
       list.querySelector('[data-has-description="true"]') !== null &&
@@ -109,12 +103,11 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>(
   return (
     <ListContext.Provider value={listContextValue}>
       {slots.heading}
-      {/* @ts-expect-error ref needs a non nullable ref */}
       <Component
         className={clsx(classes.ActionList, className)}
         role={listRole}
         aria-labelledby={ariaLabelledBy}
-        ref={appliedRef}
+        ref={mergedRef}
         data-component="ActionList"
         data-dividers={showDividers}
         data-variant={variant}

packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx

@@ -706,6 +706,33 @@ describe('AnchoredOverlay CSS anchor positioning viewport handling', () => {
 })
 
 describe('AnchoredOverlay anchor element replacement', () => {
+  it('passes the provided ref object to the anchor renderer', () => {
+    const anchorRef = createRef<HTMLElement>()
+    let renderedRef: React.Ref<HTMLElement> | undefined
+
+    render(
+      <AnchoredOverlay
+        open={false}
+        onOpen={() => {}}
+        onClose={() => {}}
+        anchorRef={anchorRef}
+        renderAnchor={props => {
+          renderedRef = (props as typeof props & {ref?: React.Ref<HTMLElement>}).ref
+          return (
+            <button type="button" {...props}>
+              Anchor
+            </button>
+          )
+        }}
+      >
+        Content
+      </AnchoredOverlay>,
+    )
+
+    expect(renderedRef).toBe(anchorRef)
+    expect(anchorRef.current).toBeInstanceOf(HTMLButtonElement)
+  })
+
   it('should re-apply anchor-name to a new anchor DOM element when the overlay reopens', () => {
     function TestComponent() {
       const anchorRef = useRef<HTMLButtonElement>(null)
@@ -766,48 +793,40 @@ describe('AnchoredOverlay anchor element replacement', () => {
   })
 })
 
-describe('AnchoredOverlay overlay forwarded ref (primer_react_merged_forwarded_refs)', () => {
-  for (const enabled of [true, false]) {
-    describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => {
-      it('forwards a ref object to the overlay', () => {
-        const ref = createRef<HTMLDivElement>()
-        render(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <BaseStyles>
-              <AnchoredOverlay
-                open
-                onClose={() => {}}
-                onOpen={() => {}}
-                renderAnchor={props => <Button {...props}>Anchor</Button>}
-                overlayProps={{ref}}
-              >
-                <button type="button">Child</button>
-              </AnchoredOverlay>
-            </BaseStyles>
-          </FeatureFlags>,
-        )
-        expect(ref.current).toBeInstanceOf(HTMLDivElement)
-      })
+describe('AnchoredOverlay overlay forwarded ref', () => {
+  it('forwards a ref object to the overlay', () => {
+    const ref = createRef<HTMLDivElement>()
+    render(
+      <BaseStyles>
+        <AnchoredOverlay
+          open
+          onClose={() => {}}
+          onOpen={() => {}}
+          renderAnchor={props => <Button {...props}>Anchor</Button>}
+          overlayProps={{ref}}
+        >
+          <button type="button">Child</button>
+        </AnchoredOverlay>
+      </BaseStyles>,
+    )
+    expect(ref.current).toBeInstanceOf(HTMLDivElement)
+  })
 
-      it('calls a callback ref with the overlay', () => {
-        const refCallback = vi.fn()
-        render(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <BaseStyles>
-              <AnchoredOverlay
-                open
-                onClose={() => {}}
-                onOpen={() => {}}
-                renderAnchor={props => <Button {...props}>Anchor</Button>}
-                overlayProps={{ref: refCallback}}
-              >
-                <button type="button">Child</button>
-              </AnchoredOverlay>
-            </BaseStyles>
-          </FeatureFlags>,
-        )
-        expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true)
-      })
-    })
-  }
+  it('calls a callback ref with the overlay', () => {
+    const refCallback = vi.fn()
+    render(
+      <BaseStyles>
+        <AnchoredOverlay
+          open
+          onClose={() => {}}
+          onOpen={() => {}}
+          renderAnchor={props => <Button {...props}>Anchor</Button>}
+          overlayProps={{ref: refCallback}}
+        >
+          <button type="button">Child</button>
+        </AnchoredOverlay>
+      </BaseStyles>,
+    )
+    expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true)
+  })
 })

packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx

@@ -202,12 +202,12 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr
   const shouldRenderAsPopover = cssAnchorPositioning && renderAs === 'popover'
   const anchorRef = useProvidedRefOrCreate(externalAnchorRef)
   const [anchorElement, setAnchorElement] = useState<HTMLElement | null>(null)
-  // eslint-disable-next-line react-hooks/refs
-  if (anchorRef.current !== anchorElement) {
+  // The object ref can point to a replacement element without changing identity.
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  useEffect(() => {
     setAnchorElement(anchorRef.current)
-  }
+  })
   const [overlayRef, updateOverlayRef] = useRenderForcingRef<HTMLDivElement>()
-  const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs')
   const [overlayElement, setOverlayElement] = useState<HTMLDivElement | null>(null)
   const mergedOverlayRef = useMergedRefs(updateOverlayRef, useMergedRefs(overlayProps?.ref, setOverlayElement))
   const anchorId = useId(externalAnchorId)
@@ -438,17 +438,7 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr
           {...(shouldRenderAsPopover ? {popover: 'manual'} : {})}
           {...restOverlayProps}
           {...(shouldRenderAsPopover ? {id: popoverId} : {})}
-          ref={
-            mergedRefEnabled
-              ? mergedOverlayRef
-              : node => {
-                  setOverlayElement(node)
-                  if (overlayProps?.ref) {
-                    assignRef(overlayProps.ref, node)
-                  }
-                  updateOverlayRef(node)
-                }
-          }
+          ref={mergedOverlayRef}
           data-anchor-position={cssAnchorPositioning}
           data-side={cssAnchorPositioning ? side : position?.anchorSide}
         >
@@ -556,15 +546,4 @@ function getCSSAnchorPositionTryFallbacks(
   return undefined
 }
 
-function assignRef<T>(
-  ref: React.MutableRefObject<T | null> | ((instance: T | null) => void) | null | undefined,
-  value: T | null,
-) {
-  if (typeof ref === 'function') {
-    ref(value)
-  } else if (ref) {
-    ref.current = value
-  }
-}
-
 AnchoredOverlay.displayName = 'AnchoredOverlay'

packages/react/src/ButtonGroup/ButtonGroup.test.tsx

@@ -1,7 +1,6 @@
 import {render, screen} from '@testing-library/react'
 import ButtonGroup from './ButtonGroup'
 import {createRef} from 'react'
-import {FeatureFlags} from '../FeatureFlags'
 import {describe, expect, it, vi} from 'vitest'
 import {implementsClassName} from '../utils/testing'
 import classes from './ButtonGroup.module.css'
@@ -25,32 +24,24 @@ describe('ButtonGroup', () => {
   })
 })
 
-describe('ButtonGroup forwarded ref (primer_react_merged_forwarded_refs)', () => {
-  for (const enabled of [true, false]) {
-    describe(`with the flag ${enabled ? 'enabled' : 'disabled'}`, () => {
-      it('forwards a ref object to the element', () => {
-        const ref = createRef<HTMLDivElement>()
-        render(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <ButtonGroup ref={ref}>
-              <button type="button">A</button>
-            </ButtonGroup>
-          </FeatureFlags>,
-        )
-        expect(ref.current).toBeInstanceOf(HTMLDivElement)
-      })
+describe('ButtonGroup forwarded ref', () => {
+  it('forwards a ref object to the element', () => {
+    const ref = createRef<HTMLDivElement>()
+    render(
+      <ButtonGroup ref={ref}>
+        <button type="button">A</button>
+      </ButtonGroup>,
+    )
+    expect(ref.current).toBeInstanceOf(HTMLDivElement)
+  })
 
-      it('calls a callback ref with the element', () => {
-        const refCallback = vi.fn()
-        render(
-          <FeatureFlags flags={{primer_react_merged_forwarded_refs: enabled}}>
-            <ButtonGroup ref={refCallback}>
-              <button type="button">A</button>
-            </ButtonGroup>
-          </FeatureFlags>,
-        )
-        expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true)
-      })
-    })
-  }
+  it('calls a callback ref with the element', () => {
+    const refCallback = vi.fn()
+    render(
+      <ButtonGroup ref={refCallback}>
+        <button type="button">A</button>
+      </ButtonGroup>,
+    )
+    expect(refCallback.mock.calls.some(([el]) => el instanceof HTMLDivElement)).toBe(true)
+  })
 })

packages/react/src/ButtonGroup/ButtonGroup.tsx

@@ -2,8 +2,7 @@ import React, {useRef, type PropsWithChildren} from 'react'
 import classes from './ButtonGroup.module.css'
 import {clsx} from 'clsx'
 import {FocusKeys, useFocusZone} from '../hooks/useFocusZone'
-import {useMergedRefs, useProvidedRefOrCreate} from '../hooks'
-import {useFeatureFlag} from '../FeatureFlags'
+import {useMergedRefs} from '../hooks'
 import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
 
 export type ButtonGroupProps = PropsWithChildren<{
@@ -17,31 +16,24 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(
   {as: BaseComponent = 'div', children, className, role, ...rest},
   forwardRef,
 ) {
-  const mergedRefEnabled = useFeatureFlag('primer_react_merged_forwarded_refs')
   const buttonRef = useRef<HTMLDivElement>(null)
   const mergedRef = useMergedRefs(buttonRef, forwardRef)
-  // Feature-flag scaffolding for `primer_react_merged_forwarded_refs`.
-  // At graduation: remove the three declarations below, and replace all instances of `readRef` with `buttonRef` and `appliedRef` with `mergedRef`.
-  const providedOrCreatedRef = useProvidedRefOrCreate(forwardRef as React.RefObject<HTMLDivElement | null>)
-  const readRef = mergedRefEnabled ? buttonRef : providedOrCreatedRef
-  const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef
   const buttons = React.Children.map(children, (child, index) => (
     <div key={index} className={classes.Item}>
       {child}
     </div>
   ))
 
   useFocusZone({
-    containerRef: readRef,
+    containerRef: buttonRef,
     disabled: role !== 'toolbar',
     bindKeys: FocusKeys.ArrowHorizontal,
     focusOutBehavior: 'wrap',
   })
 
   return (
     <BaseComponent
-      // @ts-expect-error it needs a non nullable ref
-      ref={appliedRef}
+      ref={mergedRef}
       className={clsx(className, classes.ButtonGroup)}
       role={role}
       {...rest}