SlotFill: fix a bug with storing stale fillProps (#67000)

* SlotFill: updateSlot should check the ref * Slot: store fillProps to a ref to fix effect deps * SlotFillProvider: tidy up API code * Add changelog entry Co-authored-by: jsnajdr <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: Mamaduka <[email protected]>
WordPress · Nov 15, 2024 · c3a9c1a · c3a9c1a
1 parent 2b19b58
commit c3a9c1a
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 27 deletions.
diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
@@ -10,6 +10,7 @@
 
 -   `Popover`: Fix missing label of the headerTitle Close button ([#66813](https://github.com/WordPress/gutenberg/pull/66813)).
 -   `ToggleGroupControl`: Fix active background for `0` value ([#66855](https://github.com/WordPress/gutenberg/pull/66855)).
+-   `SlotFill`: Fix a bug where a stale value of `fillProps` could be used ([#67000](https://github.com/WordPress/gutenberg/pull/67000)).
 
 ### Enhancements
 

diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx b/packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx
@@ -34,22 +34,34 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {
 
 	const unregisterSlot: SlotFillBubblesVirtuallyContext[ 'unregisterSlot' ] =
 		( name, ref ) => {
+			const slot = slots.get( name );
+			if ( ! slot ) {
+				return;
+			}
+
 			// Make sure we're not unregistering a slot registered by another element
 			// See https://github.com/WordPress/gutenberg/pull/19242#issuecomment-590295412
-			if ( slots.get( name )?.ref === ref ) {
-				slots.delete( name );
+			if ( slot.ref !== ref ) {
+				return;
 			}
+
+			slots.delete( name );
 		};
 
 	const updateSlot: SlotFillBubblesVirtuallyContext[ 'updateSlot' ] = (
 		name,
+		ref,
 		fillProps
 	) => {
 		const slot = slots.get( name );
 		if ( ! slot ) {
 			return;
 		}
 
+		if ( slot.ref !== ref ) {
+			return;
+		}
+
 		if ( isShallowEqual( slot.fillProps, fillProps ) ) {
 			return;
 		}
@@ -69,20 +81,18 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {
 		fills.set( name, [ ...( fills.get( name ) || [] ), ref ] );
 	};
 
-	const unregisterFill: SlotFillBubblesVirtuallyContext[ 'registerFill' ] = (
-		name,
-		ref
-	) => {
-		const fillsForName = fills.get( name );
-		if ( ! fillsForName ) {
-			return;
-		}
+	const unregisterFill: SlotFillBubblesVirtuallyContext[ 'unregisterFill' ] =
+		( name, ref ) => {
+			const fillsForName = fills.get( name );
+			if ( ! fillsForName ) {
+				return;
+			}
 
-		fills.set(
-			name,
-			fillsForName.filter( ( fillRef ) => fillRef !== ref )
-		);
-	};
+			fills.set(
+				name,
+				fillsForName.filter( ( fillRef ) => fillRef !== ref )
+			);
+		};
 
 	return {
 		slots,

diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot.tsx b/packages/components/src/slot-fill/bubbles-virtually/slot.tsx
@@ -35,29 +35,33 @@ function Slot(
 		as,
 		// `children` is not allowed. However, if it is passed,
 		// it will be displayed as is, so remove `children`.
-		// @ts-ignore
 		children,
 		...restProps
 	} = props;
 
 	const { registerSlot, unregisterSlot, ...registry } =
 		useContext( SlotFillContext );
+
 	const ref = useRef< HTMLElement >( null );
 
+	// We don't want to unregister and register the slot whenever
+	// `fillProps` change, which would cause the fill to be re-mounted. Instead,
+	// we can just update the slot (see hook below).
+	// For more context, see https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973
+	const fillPropsRef = useRef( fillProps );
+	useLayoutEffect( () => {
+		fillPropsRef.current = fillProps;
+	}, [ fillProps ] );
+
 	useLayoutEffect( () => {
-		registerSlot( name, ref, fillProps );
+		registerSlot( name, ref, fillPropsRef.current );
 		return () => {
 			unregisterSlot( name, ref );
 		};
-		// We don't want to unregister and register the slot whenever
-		// `fillProps` change, which would cause the fill to be re-mounted. Instead,
-		// we can just update the slot (see hook below).
-		// For more context, see https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973
 	}, [ registerSlot, unregisterSlot, name ] );
-	// fillProps may be an update that interacts with the layout, so we
-	// useLayoutEffect.
+
 	useLayoutEffect( () => {
-		registry.updateSlot( name, fillProps );
+		registry.updateSlot( name, ref, fillPropsRef.current );
 	} );
 
 	return (

diff --git a/packages/components/src/slot-fill/bubbles-virtually/use-slot.ts b/packages/components/src/slot-fill/bubbles-virtually/use-slot.ts
@@ -21,8 +21,10 @@ export default function useSlot( name: SlotKey ) {
 
 	const api = useMemo(
 		() => ( {
-			updateSlot: ( fillProps: FillProps ) =>
-				registry.updateSlot( name, fillProps ),
+			updateSlot: (
+				ref: SlotFillBubblesVirtuallySlotRef,
+				fillProps: FillProps
+			) => registry.updateSlot( name, ref, fillProps ),
 			unregisterSlot: ( ref: SlotFillBubblesVirtuallySlotRef ) =>
 				registry.unregisterSlot( name, ref ),
 			registerFill: ( ref: SlotFillBubblesVirtuallyFillRef ) =>

diff --git a/packages/components/src/slot-fill/types.ts b/packages/components/src/slot-fill/types.ts
@@ -131,7 +131,11 @@ export type SlotFillBubblesVirtuallyContext = {
 		name: SlotKey,
 		ref: SlotFillBubblesVirtuallySlotRef
 	) => void;
-	updateSlot: ( name: SlotKey, fillProps: FillProps ) => void;
+	updateSlot: (
+		name: SlotKey,
+		ref: SlotFillBubblesVirtuallySlotRef,
+		fillProps: FillProps
+	) => void;
 	registerFill: (
 		name: SlotKey,
 		ref: SlotFillBubblesVirtuallyFillRef