Avoid undo issues when reset parent blocks for controlled blocks

packages/block-editor/src/components/provider/use-block-sync.js

@@ -7,7 +7,7 @@ import { last, noop } from 'lodash';
  * WordPress dependencies
  */
 import { useEffect, useRef } from '@wordpress/element';
-import { useRegistry } from '@wordpress/data';
+import { useRegistry, useSelect } from '@wordpress/data';
 import { cloneBlock } from '@wordpress/blocks';
 
 /**
@@ -83,6 +83,15 @@ export default function useBlockSync( {
 		__unstableMarkNextChangeAsNotPersistent,
 	} = registry.dispatch( blockEditorStore );
 	const { getBlockName, getBlocks } = registry.select( blockEditorStore );
+	const isControlled = useSelect(
+		( select ) => {
+			return (
+				! clientId ||
+				select( blockEditorStore ).areInnerBlocksControlled( clientId )
+			);
+		},
+		[ clientId ]
+	);
 
 	const pendingChanges = useRef( { incoming: null, outgoing: [] } );
 	const subscribed = useRef( false );
@@ -97,15 +106,21 @@ export default function useBlockSync( {
 		// and so it would already be persisted.
 		__unstableMarkNextChangeAsNotPersistent();
 		if ( clientId ) {
-			setHasControlledInnerBlocks( clientId, true );
-			__unstableMarkNextChangeAsNotPersistent();
-			const storeBlocks = controlledBlocks.map( ( block ) =>
-				cloneBlock( block )
-			);
-			if ( subscribed.current ) {
-				pendingChanges.current.incoming = storeBlocks;
-			}
-			replaceInnerBlocks( clientId, storeBlocks );
+			// It is important to batch here because otherwise,
+			// as soon as `setHasControlledInnerBlocks` is called
+			// the effect to restore might be triggered
+			// before the actual blocks get set properly in state.
+			registry.batch( () => {
+				setHasControlledInnerBlocks( clientId, true );
+				const storeBlocks = controlledBlocks.map( ( block ) =>
+					cloneBlock( block )
+				);
+				if ( subscribed.current ) {
+					pendingChanges.current.incoming = storeBlocks;
+				}
+				__unstableMarkNextChangeAsNotPersistent();
+				replaceInnerBlocks( clientId, storeBlocks );
+			} );
 		} else {
 			if ( subscribed.current ) {
 				pendingChanges.current.incoming = controlledBlocks;
@@ -157,13 +172,23 @@ export default function useBlockSync( {
 		}
 	}, [ controlledBlocks, clientId ] );
 
+	useEffect( () => {
+		// When the block becomes uncontrolled, it means its inner state has been reset
+		// we need to take the blocks again from the external value property.
+		if ( ! isControlled ) {
+			pendingChanges.current.outgoing = [];
+			setControlledBlocks();
+		}
+	}, [ isControlled ] );
+
 	useEffect( () => {
 		const {
 			getSelectionStart,
 			getSelectionEnd,
 			getSelectedBlocksInitialCaretPosition,
 			isLastBlockChangePersistent,
 			__unstableIsLastBlockChangeIgnored,
+			areInnerBlocksControlled,
 		} = registry.select( blockEditorStore );
 
 		let blocks = getBlocks( clientId );
@@ -182,8 +207,17 @@ export default function useBlockSync( {
 			if ( clientId !== null && getBlockName( clientId ) === null )
 				return;
 
-			const newIsPersistent = isLastBlockChangePersistent();
+			// When RESET_BLOCKS on parent blocks get called, the controlled blocks
+			// can reset to uncontrolled, in these situations, it means we need to populate
+			// the blocks again from the external blocks (the value property here)
+			// and we should stop triggering onChange
+			const isStillControlled =
+				! clientId || areInnerBlocksControlled( clientId );
+			if ( ! isStillControlled ) {
+				return;
+			}
 
+			const newIsPersistent = isLastBlockChangePersistent();
 			const newBlocks = getBlocks( clientId );
 			const areBlocksDifferent = newBlocks !== blocks;
 			blocks = newBlocks;

packages/block-editor/src/store/reducer.js

@@ -737,6 +737,27 @@ const withSaveReusableBlock = ( reducer ) => ( state, action ) => {
 
 	return reducer( state, action );
 };
+/**
+ * Higher-order reducer which removes blocks from state when switching parent block controlled state.
+ *
+ * @param {Function} reducer Original reducer function.
+ *
+ * @return {Function} Enhanced reducer function.
+ */
+const withResetControlledBlocks = ( reducer ) => ( state, action ) => {
+	if ( action.type === 'SET_HAS_CONTROLLED_INNER_BLOCKS' ) {
+		// when switching a block from controlled to uncontrolled or inverse,
+		// we need to remove its content first.
+		const tempState = reducer( state, {
+			type: 'REPLACE_INNER_BLOCKS',
+			rootClientId: action.clientId,
+			blocks: [],
+		} );
+		return reducer( tempState, action );
+	}
+
+	return reducer( state, action );
+};
 
 /**
  * Reducer returning the blocks state.
@@ -754,7 +775,8 @@ export const blocks = flow(
 	withReplaceInnerBlocks, // needs to be after withInnerBlocksRemoveCascade
 	withBlockReset,
 	withPersistentBlockChange,
-	withIgnoredBlockChange
+	withIgnoredBlockChange,
+	withResetControlledBlocks
 )( {
 	byClientId( state = {}, action ) {
 		switch ( action.type ) {

packages/block-editor/src/store/test/reducer.js

@@ -2050,6 +2050,41 @@ describe( 'state', () => {
 					expect( state.isIgnoredChange ).toBe( true );
 				} );
 			} );
+
+			describe( 'controlledInnerBlocks', () => {
+				it( 'should remove the content of the block if it switches from controlled to uncontrolled or opposite', () => {
+					const original = blocks( undefined, {
+						type: 'RESET_BLOCKS',
+						blocks: [
+							{
+								clientId: 'chicken',
+								name: 'core/test-block',
+								attributes: {},
+								innerBlocks: [
+									{
+										clientId: 'child',
+										name: 'core/test-block',
+										attributes: {},
+										innerBlocks: [],
+									},
+								],
+							},
+						],
+					} );
+
+					const state = blocks( original, {
+						type: 'SET_HAS_CONTROLLED_INNER_BLOCKS',
+						clientId: 'chicken',
+						hasControlledInnerBlocks: true,
+					} );
+
+					expect( state.controlledInnerBlocks.chicken ).toBe( true );
+					// The previous content of the block should be removed
+					expect( state.byClientId.child ).toBeUndefined();
+					expect( state.tree.child ).toBeUndefined();
+					expect( state.tree.chicken.innerBlocks ).toEqual( [] );
+				} );
+			} );
 		} );
 	} );
 

packages/block-library/src/navigation/edit/index.js

@@ -28,7 +28,7 @@ import {
 } from '@wordpress/block-editor';
 import { EntityProvider, useEntityProp } from '@wordpress/core-data';
 
-import { useDispatch, useSelect } from '@wordpress/data';
+import { useDispatch, useSelect, useRegistry } from '@wordpress/data';
 import {
 	PanelBody,
 	ToggleControl,
@@ -132,6 +132,7 @@ function Navigation( {
 
 	const ref = navigationArea ? navigationAreaMenu : attributes.ref;
 
+	const registry = useRegistry();
 	const setRef = useCallback(
 		( postId ) => {
 			setAttributes( { ref: postId } );
@@ -149,7 +150,6 @@ function Navigation( {
 	const {
 		hasUncontrolledInnerBlocks,
 		uncontrolledInnerBlocks,
-		controlledInnerBlocks,
 		isInnerBlockSelected,
 		hasSubmenus,
 	} = useSelect(
@@ -178,7 +178,6 @@ function Navigation( {
 				),
 				hasUncontrolledInnerBlocks: _hasUncontrolledInnerBlocks,
 				uncontrolledInnerBlocks: _uncontrolledInnerBlocks,
-				controlledInnerBlocks: _controlledInnerBlocks,
 				isInnerBlockSelected: hasSelectedInnerBlock( clientId, true ),
 			};
 		},
@@ -322,18 +321,6 @@ function Navigation( {
 		setIsPlaceholderShown( ! isEntityAvailable );
 	}, [ isEntityAvailable ] );
 
-	// If the ref no longer exists the reset the inner blocks to provide a
-	// clean slate.
-	useEffect( () => {
-		if (
-			! hasUncontrolledInnerBlocks &&
-			controlledInnerBlocks?.length > 0 &&
-			ref === undefined
-		) {
-			replaceInnerBlocks( clientId, [] );
-		}
-	}, [ clientId, ref, hasUncontrolledInnerBlocks, controlledInnerBlocks ] );
-
 	const [ showCantEditNotice, hideCantEditNotice ] = useNavigationNotice( {
 		name: 'block-library/core/navigation/permissions/update',
 		message: __(
@@ -383,15 +370,19 @@ function Navigation( {
 	] );
 
 	const startWithEmptyMenu = useCallback( () => {
-		if ( navigationArea ) {
-			setAreaMenu( 0 );
-		}
-		setAttributes( {
-			ref: undefined,
+		registry.batch( () => {
+			if ( navigationArea ) {
+				setAreaMenu( 0 );
+			}
+			setAttributes( {
+				ref: undefined,
+			} );
+			if ( ! ref ) {
+				replaceInnerBlocks( clientId, [] );
+			}
+			setIsPlaceholderShown( true );
 		} );
-
-		setIsPlaceholderShown( true );
-	}, [ clientId ] );
+	}, [ clientId, ref ] );
 
 	// If the block has inner blocks, but no menu id, this was an older
 	// navigation block added before the block used a wp_navigation entity.
@@ -423,11 +414,6 @@ function Navigation( {
 						onSave={ ( post ) => {
 							// Set some state used as a guard to prevent the creation of multiple posts.
 							setHasSavedUnsavedInnerBlocks( true );
-							// replaceInnerBlocks is required to ensure the block editor store is sync'd
-							// to be aware that there are now no inner blocks (as blocks moved to entity).
-							// This should probably happen automatically with useBlockSync
-							// but there appears to be a bug.
-							replaceInnerBlocks( clientId, [] );
 							// Switch to using the wp_navigation entity.
 							setRef( post.id );
 						} }
@@ -613,15 +599,7 @@ function Navigation( {
 						{ hasResolvedCanUserDeleteNavigationEntity &&
 							canUserDeleteNavigationEntity && (
 								<NavigationMenuDeleteControl
-									onDelete={ () => {
-										if ( navigationArea ) {
-											setAreaMenu( 0 );
-										}
-										setAttributes( {
-											ref: undefined,
-										} );
-										setIsPlaceholderShown( true );
-									} }
+									onDelete={ startWithEmptyMenu }
 								/>
 							) }
 					</InspectorControls>

packages/block-library/src/navigation/edit/unsaved-inner-blocks.js

@@ -18,7 +18,6 @@ import { useContext, useEffect, useRef } from '@wordpress/element';
 import useNavigationMenu from '../use-navigation-menu';
 import useCreateNavigationMenu from './use-create-navigation-menu';
 
-const NOOP = () => {};
 const EMPTY_OBJECT = {};
 const DRAFT_MENU_PARAMS = [
 	'postType',
@@ -41,13 +40,6 @@ export default function UnsavedInnerBlocks( {
 
 	const innerBlocksProps = useInnerBlocksProps( blockProps, {
 		renderAppender: hasSelection ? undefined : false,
-
-		// Make the inner blocks 'controlled'. This allows the block to always
-		// work with controlled inner blocks, smoothing out the switch to using
-		// an entity.
-		value: blocks,
-		onChange: NOOP,
-		onInput: NOOP,
 	} );
 
 	const {