diff --git a/packages/ui/src/stepper/root.tsx b/packages/ui/src/stepper/root.tsx index 787a60a32bfe..cf7f8c976179 100644 --- a/packages/ui/src/stepper/root.tsx +++ b/packages/ui/src/stepper/root.tsx @@ -72,22 +72,14 @@ export const StepperRoot = forwardRef< StepperRef, StepperRootProps >( function }, } ) ); - // Dev warnings: missing accessibility label, duplicate values, missing step + // Dev warnings: missing accessibility label, missing step. + // Duplicate step values are detected at registration time in useStepRegistration. useEffect( () => { if ( ! ariaLabel && ! ariaLabelledBy ) { warning( "[Stepper] Stepper requires either 'aria-label' or 'aria-labelledby' for accessibility." ); } - const seenValues = new Set< string >(); - for ( const s of steps ) { - if ( seenValues.has( s.value ) ) { - warning( - `[Stepper] Two steps share value '${ s.value }'. Each step must have a unique value.` - ); - } - seenValues.add( s.value ); - } if ( value && steps.length > 0 && ! steps.some( ( s ) => s.value === value ) ) { warning( `[Stepper] No step found with value '${ value }'. Check that the value matches one of the registered step values.` diff --git a/packages/ui/src/stepper/step.tsx b/packages/ui/src/stepper/step.tsx index c56c53a45b01..12bac1371a1d 100644 --- a/packages/ui/src/stepper/step.tsx +++ b/packages/ui/src/stepper/step.tsx @@ -1,6 +1,6 @@ // packages/ui/src/stepper/step.tsx import { Accordion } from '@base-ui/react/accordion'; -import { forwardRef, useContext, useEffect, useLayoutEffect } from '@wordpress/element'; +import { forwardRef, useContext, useEffect, useId, useLayoutEffect } from '@wordpress/element'; import { StepContext, StepperContext } from './context'; import type { StepContextValue, StepStatus } from './types'; import type { ReactNode } from 'react'; @@ -33,6 +33,10 @@ export const StepperStep = forwardRef< HTMLDivElement, StepperStepProps >( funct orientation, } = rootCtx; + // Stable per-mount id so each instance owns its own registration, even when + // two steps transiently share the same `value`. + const id = useId(); + // Compute derived state const isCurrent = value === activeValue; const index = steps.findIndex( ( s ) => s.value === value ); @@ -47,17 +51,17 @@ export const StepperStep = forwardRef< HTMLDivElement, StepperStepProps >( funct // useLayoutEffect ensures registration happens before the first browser paint, // so totalSteps and index are correct on the first visible render. useLayoutEffect( () => { - const deregister = registerStep( { value, status, disabled: isDisabled } ); + const deregister = registerStep( id, { value, status, disabled: isDisabled } ); return deregister; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ value ] ); + }, [ id, value ] ); // useEffect (not useLayoutEffect) is intentional: syncing metadata after // commit does not affect the initial render. The redundant call on first // mount is harmless — updateStep is a no-op when props have not changed. useEffect( () => { - updateStep( { value, status, disabled: isDisabled } ); - }, [ value, status, isDisabled, updateStep ] ); + updateStep( id, { value, status, disabled: isDisabled } ); + }, [ id, value, status, isDisabled, updateStep ] ); const stepCtx: StepContextValue = { value, diff --git a/packages/ui/src/stepper/test/index.tsx b/packages/ui/src/stepper/test/index.tsx index 146f8dfe5929..1c2747b4faed 100644 --- a/packages/ui/src/stepper/test/index.tsx +++ b/packages/ui/src/stepper/test/index.tsx @@ -592,12 +592,95 @@ describe( 'Stepper dev warnings', () => { spy.mockRestore(); } ); - // NOTE: A test for the duplicate-value warning ("Two steps share value '...'") is - // deliberately omitted. useStepRegistration prevents duplicate entries at registration - // time (it bails out if a step with the same value is already in state), so the - // steps array that the useEffect in root.tsx inspects will never contain duplicates. - // The warning is unreachable in a JSDOM environment via normal JSX rendering, making - // any such test a never-fails assertion that proves nothing about real behaviour. + // NOTE: warning() dedupes messages process-wide, so these tests use step values + // unique to this file to make sure the expected message has not fired elsewhere. + it( 'warns when two steps share the same value', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + render( + + + + + + + + + ); + await waitFor( () => { + expect( spy ).toHaveBeenCalledWith( + expect.stringContaining( "Two steps share value 'dup-warn'" ) + ); + } ); + spy.mockRestore(); + } ); + + it( 'keeps the original step registered when a duplicate step unmounts', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + function Dynamic() { + const [ showDuplicate, setShowDuplicate ] = useState( true ); + return ( + <> + + + + + + { showDuplicate && ( + + + + ) } + + + + + + ); + } + const user = userEvent.setup(); + render( ); + // The duplicate is rejected at registration time, so only two steps count. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + await user.click( screen.getByRole( 'button', { name: /remove duplicate/i } ) ); + // Unmounting the rejected duplicate must not deregister the original step. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + expect( screen.getByText( 'Step 1 of 2' ) ).toBeInTheDocument(); + spy.mockRestore(); + } ); + + it( 'keeps the step registered when the original unmounts but a duplicate is still mounted', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + function Dynamic() { + const [ showOriginal, setShowOriginal ] = useState( true ); + return ( + <> + + + { showOriginal && ( + + + + ) } + + + + + + + + + ); + } + const user = userEvent.setup(); + render( ); + // The duplicate is rejected at registration time, so only two steps count. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + await user.click( screen.getByRole( 'button', { name: /remove original/i } ) ); + // The duplicate instance is still mounted, so the value must stay registered. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + expect( screen.getByText( 'Step 1 of 2' ) ).toBeInTheDocument(); + spy.mockRestore(); + } ); } ); describe( 'Stepper.Panel dev warnings', () => { diff --git a/packages/ui/src/stepper/test/use-step-registration.ts b/packages/ui/src/stepper/test/use-step-registration.ts new file mode 100644 index 000000000000..2e22bd688d6f --- /dev/null +++ b/packages/ui/src/stepper/test/use-step-registration.ts @@ -0,0 +1,180 @@ +import { renderHook, act } from '@testing-library/react'; +import { useStepRegistration } from '../use-step-registration'; +import type { StepMeta } from '../types'; + +describe( 'useStepRegistration duplicate values', () => { + let warnSpy: jest.SpyInstance; + + beforeEach( () => { + // Duplicate values are a dev error and emit a one-time console warning; + // suppress it here so test output stays clean. + warnSpy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + } ); + + afterEach( () => { + warnSpy.mockRestore(); + } ); + + it( 'reflects the surviving instance metadata when a duplicate unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterDuplicate: () => void = () => {}; + act( () => { + result.current.registerStep( 'a-1', { value: 'a', status: 'completed', disabled: false } ); + deregisterDuplicate = result.current.registerStep( 'a-2', { + value: 'a', + status: 'error', + disabled: true, + } ); + } ); + // In real usage each Step's effect syncs its own props; the duplicate's + // sync must not overwrite the shared-value entry (the value-keyed bug). + act( () => { + result.current.updateStep( 'a-2', { value: 'a', status: 'error', disabled: true } ); + } ); + // Even while both are mounted, the canonical entry stays 'a-1', not the + // later writer 'a-2'. + expect( result.current.steps ).toEqual( [ + { value: 'a', status: 'completed', disabled: false }, + ] ); + act( () => { + deregisterDuplicate(); + } ); + // ...and it remains correct after the duplicate unmounts. + expect( result.current.steps ).toEqual( [ + { value: 'a', status: 'completed', disabled: false }, + ] ); + } ); + + it( 'keeps a value in its original slot when an earlier duplicate unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterFirstA: () => void = () => {}; + act( () => { + deregisterFirstA = result.current.registerStep( 'a-1', { value: 'a', disabled: false } ); + result.current.registerStep( 'b-1', { value: 'b', disabled: false } ); + // Duplicate of 'a', registered after 'b'. + result.current.registerStep( 'a-2', { value: 'a', disabled: true } ); + } ); + act( () => { + deregisterFirstA(); + } ); + // 'a' keeps its first-appearance slot (index 0) rather than jumping to the + // end, and now reflects the surviving instance ('a-2'). + expect( result.current.steps ).toEqual( [ + { value: 'a', disabled: true }, + { value: 'b', disabled: false }, + ] ); + } ); + + it( 'keeps the value registered until the last instance unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterFirst: () => void = () => {}; + act( () => { + deregisterFirst = result.current.registerStep( 'a-1', { value: 'a', disabled: false } ); + result.current.registerStep( 'a-2', { value: 'a', disabled: false } ); + } ); + act( () => { + deregisterFirst(); + } ); + // One instance remains, so the value stays registered and reflects the + // surviving instance. + expect( result.current.steps ).toEqual( [ { value: 'a', disabled: false } ] ); + } ); +} ); + +describe( 'useStepRegistration duplicate warning', () => { + let warnSpy: jest.SpyInstance; + + beforeEach( () => { + warnSpy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + } ); + + afterEach( () => { + warnSpy.mockRestore(); + } ); + + it( 'warns when a second instance registers an already-registered value', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + result.current.registerStep( 'dup-1', { value: 'shared', disabled: false } ); + result.current.registerStep( 'dup-2', { value: 'shared', disabled: false } ); + } ); + // Distinct values per test keep the warning util's message-level dedupe + // from masking this assertion across the file. + expect( warnSpy ).toHaveBeenCalledWith( + expect.stringContaining( "Two steps share value 'shared'" ) + ); + } ); + + it( 'does not warn when every step value is unique', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + result.current.registerStep( 'u-1', { value: 'one', disabled: false } ); + result.current.registerStep( 'u-2', { value: 'two', disabled: false } ); + } ); + expect( warnSpy ).not.toHaveBeenCalled(); + } ); +} ); + +describe( 'useStepRegistration updateStep', () => { + function setup() { + const hook = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + hook.result.current.registerStep( 'a', { + value: 'a', + status: 'completed', + disabled: false, + } ); + hook.result.current.registerStep( 'b', { value: 'b', disabled: false } ); + } ); + return hook; + } + + it( 'keeps the same steps array reference when meta has not changed', () => { + const { result } = setup(); + const before = result.current.steps; + act( () => { + result.current.updateStep( 'a', { value: 'a', status: 'completed', disabled: false } ); + } ); + expect( result.current.steps ).toBe( before ); + } ); + + it( 'updates the entry when a field value changes', () => { + const { result } = setup(); + act( () => { + result.current.updateStep( 'a', { value: 'a', status: 'error', disabled: false } ); + } ); + expect( result.current.steps[ 0 ] ).toEqual( { + value: 'a', + status: 'error', + disabled: false, + } ); + } ); + + it( 'updates the entry when a field present in the existing meta is absent from the new meta', () => { + const { result } = setup(); + // `status` key intentionally omitted — the compare must not treat a + // removed field as "no change". + act( () => { + result.current.updateStep( 'a', { value: 'a', disabled: false } ); + } ); + expect( result.current.steps[ 0 ] ).toEqual( { value: 'a', disabled: false } ); + expect( result.current.steps[ 0 ] ).not.toHaveProperty( 'status' ); + } ); + + it( 'is a no-op for an id that was never registered', () => { + const { result } = setup(); + const before = result.current.steps; + act( () => { + result.current.updateStep( 'missing', { value: 'missing', disabled: false } ); + } ); + expect( result.current.steps ).toBe( before ); + } ); + + it( 'preserves registration order when updating a step', () => { + const { result } = setup(); + act( () => { + result.current.updateStep( 'a', { value: 'a', status: 'error', disabled: true } ); + } ); + expect( result.current.steps.map( ( s ) => s.value ) ).toEqual( [ 'a', 'b' ] ); + } ); +} ); diff --git a/packages/ui/src/stepper/types.ts b/packages/ui/src/stepper/types.ts index 6750ef6aebb7..b0d4f69e4e9d 100644 --- a/packages/ui/src/stepper/types.ts +++ b/packages/ui/src/stepper/types.ts @@ -47,8 +47,8 @@ export type StepperContextValue = { activationMode: 'auto' | 'manual'; steps: StepMeta[]; totalSteps: number; - registerStep: ( meta: StepMeta ) => () => void; - updateStep: ( meta: StepMeta ) => void; + registerStep: ( id: string, meta: StepMeta ) => () => void; + updateStep: ( id: string, meta: StepMeta ) => void; registerTriggerRef: ( value: string, el: HTMLElement | null ) => void; formatStepLabel: ( step: number, total: number, status?: StepStatus ) => string; indicatorVariant: IndicatorVariant; @@ -154,6 +154,4 @@ export type StepperPanelProps = ComponentProps< 'div' > & { value?: string; /** Keep panel mounted when inactive (works in both orientations) */ keepMounted?: boolean; - children: ReactNode; - className?: string; }; diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index 27466c68a3ac..6cc1e099f579 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -1,51 +1,106 @@ -import { useCallback, useState } from '@wordpress/element'; +import { useCallback, useMemo, useRef, useState } from '@wordpress/element'; +import { warning } from '../utils/warning'; // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- +type Instance< T > = { id: string; slot: number; meta: T }; + /** * Maintains an ordered list of registered steps for any record type that * includes a `value` string. Registration order is the sole source of truth * for index and counting. * + * Registrations are keyed by a unique per-mount `id` (not `value`), so each + * mounted instance owns its own metadata. The public `steps` list is derived + * with one entry per `value`, taken from the first still-mounted instance — + * this means a duplicate value never overrides or leaves stale metadata on the + * surviving step. A value holds the slot it was first registered in for as long + * as any instance of it stays mounted, so a surviving duplicate does not jump + * position when an earlier instance unmounts. Duplicate values are still a dev + * error and emit a warning. + * * Used by Stepper.Root (with StepMeta) to maintain step order and metadata. */ export function useStepRegistration< T extends { value: string } >() { - const [ steps, setSteps ] = useState< T[] >( [] ); + const [ instances, setInstances ] = useState< Instance< T >[] >( [] ); - const registerStep = useCallback( ( meta: T ) => { - setSteps( ( prev ) => { - // Avoid duplicate registration (e.g. from concurrent remounts or late renders) - if ( prev.some( ( s ) => s.value === meta.value ) ) { - return prev; - } - return [ ...prev, meta ]; - } ); + // Per-value mount count, used only to warn on duplicate values without + // reaching into the (pure) state updater. + const mountCounts = useRef( new Map< string, number >() ); + + // Stable slot per value, assigned when a value first mounts and held until + // its last instance unmounts. Duplicate instances of a value reuse its slot, + // so each carries its slot in state and `steps` order stays pinned to + // first-appearance even as duplicates come and go. Read only here (never + // during render) so the derivation stays pure. + const slotOrder = useRef( new Map< string, number >() ); + const nextSlot = useRef( 0 ); + + const registerStep = useCallback( ( id: string, meta: T ) => { + const counts = mountCounts.current; + const count = counts.get( meta.value ) ?? 0; + counts.set( meta.value, count + 1 ); + if ( count > 0 ) { + warning( + `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` + ); + } + let slot = slotOrder.current.get( meta.value ); + if ( slot === undefined ) { + slot = nextSlot.current++; + slotOrder.current.set( meta.value, slot ); + } + setInstances( ( prev ) => [ ...prev, { id, slot, meta } ] ); return () => { - setSteps( ( prev ) => prev.filter( ( s ) => s.value !== meta.value ) ); + const remaining = ( counts.get( meta.value ) ?? 1 ) - 1; + if ( remaining > 0 ) { + counts.set( meta.value, remaining ); + } else { + counts.delete( meta.value ); + slotOrder.current.delete( meta.value ); + } + setInstances( ( prev ) => prev.filter( ( inst ) => inst.id !== id ) ); }; }, [] ); - const updateStep = useCallback( ( meta: T ) => { - setSteps( ( prev ) => { - const idx = prev.findIndex( ( s ) => s.value === meta.value ); + const updateStep = useCallback( ( id: string, meta: T ) => { + setInstances( ( prev ) => { + const idx = prev.findIndex( ( inst ) => inst.id === id ); if ( idx === -1 ) { return prev; } - const existing = prev[ idx ]; - // Union keys from both objects so fields present in existing but - // absent from meta are not silently ignored. - const keys = [ - ...new Set( [ ...Object.keys( existing ), ...Object.keys( meta ) ] ), - ] as ( keyof T )[]; - if ( keys.every( ( k ) => existing[ k ] === meta[ k ] ) ) { + const existing = prev[ idx ].meta; + // Plain shallow compare. The key-count check catches fields present + // in existing but absent from meta (e.g. a removed `status`). + const metaKeys = Object.keys( meta ) as ( keyof T )[]; + const isUnchanged = + Object.keys( existing ).length === metaKeys.length && + metaKeys.every( ( k ) => existing[ k ] === meta[ k ] ); + if ( isUnchanged ) { return prev; } - return prev.map( ( s ) => ( s.value === meta.value ? meta : s ) ); + return prev.map( ( inst ) => ( inst.id === id ? { ...inst, meta } : inst ) ); } ); }, [] ); + // One step per value, taken from the first still-mounted instance and + // ordered by each value's stable first-appearance slot. Memoised so a no-op + // update (which leaves `instances` referentially stable) keeps the same + // `steps` reference. + const steps = useMemo( () => { + const canonical = new Map< string, { slot: number; meta: T } >(); + for ( const inst of instances ) { + if ( ! canonical.has( inst.meta.value ) ) { + canonical.set( inst.meta.value, { slot: inst.slot, meta: inst.meta } ); + } + } + return [ ...canonical.values() ] + .sort( ( a, b ) => a.slot - b.slot ) + .map( ( entry ) => entry.meta ); + }, [ instances ] ); + return { steps, registerStep, updateStep }; }