Inserter: Fix 'Browse All' in Quick Inserter (#26443)

* Inserter: Fix 'Browse All' in Quick Inserter Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'. * Inserter: Fix typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Call getBlockInsertionPoint once * Add useSelect dependencies * Make setInsertionPoint unstable * Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered * Make index required and clarify rootClientId usage * Split insertionPoint into two reducers * Fix getInsertionPoint selector that expects default state of reducer to be null * Avoid resetting insertionPoint state on HIDE_INSERTION_POINT Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
WordPress · Nov 10, 2020 · 7f1fac9 · 7f1fac9
1 parent b112748
commit 7f1fac9
Show file tree

Hide file tree

Showing 11 changed files with 294 additions and 132 deletions.
diff --git a/docs/designers-developers/developers/data/data-core-block-editor.md b/docs/designers-developers/developers/data/data-core-block-editor.md
@@ -160,16 +160,22 @@ _Returns_
 
 <a name="getBlockInsertionPoint" href="#getBlockInsertionPoint">#</a> **getBlockInsertionPoint**
 
-Returns the insertion point, the index at which the new inserted block would
-be placed. Defaults to the last index.
+Returns the insertion point. This will be:
+
+1) The insertion point manually set using setInsertionPoint() or
+   showInsertionPoint(); or
+2) The point after the current block selection, if there is a selection; or
+3) The point at the end of the block list.
+
+Components like <Inserter> will default to inserting blocks at this point.
 
 _Parameters_
 
--   _state_ `Object`: Editor state.
+-   _state_ `Object`: Global application state.
 
 _Returns_
 
--   `Object`: Insertion point object with `rootClientId`, `index`.
+-   `Object`: Insertion point object with `rootClientId` and `index`.
 
 <a name="getBlockListSettings" href="#getBlockListSettings">#</a> **getBlockListSettings**
 
@@ -812,15 +818,16 @@ _Returns_
 
 <a name="isBlockInsertionPointVisible" href="#isBlockInsertionPointVisible">#</a> **isBlockInsertionPointVisible**
 
-Returns true if we should show the block insertion point.
+Whether or not the insertion point should be shown to users. This is set
+using showInsertionPoint() or hideInsertionPoint().
 
 _Parameters_
 
 -   _state_ `Object`: Global application state.
 
 _Returns_
 
--   `?boolean`: Whether the insertion point is visible or not.
+-   `?boolean`: Whether the insertion point should be shown.
 
 <a name="isBlockMultiSelected" href="#isBlockMultiSelected">#</a> **isBlockMultiSelected**
 
@@ -1048,7 +1055,7 @@ _Parameters_
 
 <a name="hideInsertionPoint" href="#hideInsertionPoint">#</a> **hideInsertionPoint**
 
-Returns an action object hiding the insertion point.
+Hides the insertion point for users.
 
 _Returns_
 
@@ -1373,13 +1380,14 @@ _Returns_
 
 <a name="showInsertionPoint" href="#showInsertionPoint">#</a> **showInsertionPoint**
 
-Returns an action object used in signalling that the insertion point should
-be shown.
+Sets the insertion point and shows it to users.
+
+Components like <Inserter> will default to inserting blocks at this point.
 
 _Parameters_
 
--   _rootClientId_ `?string`: Optional root client ID of block list on which to insert.
--   _index_ `?number`: Index at which block should be inserted.
+-   _rootClientId_ `?string`: Root client ID of block list in which to insert. Use `undefined` for the root block list.
+-   _index_ `number`: Index at which block should be inserted.
 
 _Returns_
 

diff --git a/packages/block-editor/src/components/inserter/hooks/use-insertion-point.js b/packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
@@ -1,8 +1,3 @@
-/**
- * External dependencies
- */
-import { pick } from 'lodash';
-
 /**
  * WordPress dependencies
  */
@@ -14,10 +9,17 @@ import { speak } from '@wordpress/a11y';
 /**
  * @typedef WPInserterConfig
  *
- * @property {string=} rootClientId        Inserter Root Client ID.
- * @property {string=} clientId            Inserter Client ID.
- * @property {boolean} isAppender          Whether the inserter is an appender or not.
- * @property {boolean} selectBlockOnInsert Whether the block should be selected on insert.
+ * @property {string=}   rootClientId        If set, insertion will be into the
+ *                                           block with this ID.
+ * @property {number=}   insertionIndex      If set, insertion will be into this
+ *                                           explicit position.
+ * @property {string=}   clientId            If set, insertion will be after the
+ *                                           block with this ID.
+ * @property {boolean=}  isAppender          Whether the inserter is an appender
+ *                                           or not.
+ * @property {boolean=}  selectBlockOnInsert Whether the block should be
+ *                                           selected on insert.
+ * @property {Function=} onSelect            Called after insertion.
  */
 
 /**
@@ -27,77 +29,74 @@ import { speak } from '@wordpress/a11y';
  * @return {Array} Insertion Point State (rootClientID, onInsertBlocks and onToggle).
  */
 function useInsertionPoint( {
-	onSelect,
 	rootClientId,
+	insertionIndex,
 	clientId,
 	isAppender,
 	selectBlockOnInsert,
-	insertionIndex,
+	onSelect,
 } ) {
 	const {
+		selectedBlock,
 		destinationRootClientId,
-		getSelectedBlock,
-		getBlockIndex,
-		getBlockSelectionEnd,
-		getBlockOrder,
+		destinationIndex,
 	} = useSelect(
 		( select ) => {
 			const {
-				getSettings,
-				getBlockRootClientId,
-				getBlockSelectionEnd: _getBlockSelectionEnd,
+				getSelectedBlock,
+				getBlockIndex,
+				getBlockOrder,
+				getBlockInsertionPoint,
 			} = select( 'core/block-editor' );
 
-			let destRootClientId = rootClientId;
-			if ( ! destRootClientId && ! clientId && ! isAppender ) {
-				const end = _getBlockSelectionEnd();
-				if ( end ) {
-					destRootClientId = getBlockRootClientId( end );
+			let _destinationRootClientId, _destinationIndex;
+
+			if ( rootClientId || insertionIndex || clientId || isAppender ) {
+				// If any of these arguments are set, we're in "manual mode"
+				// meaning the insertion point is set by the caller.
+
+				_destinationRootClientId = rootClientId;
+
+				if ( insertionIndex ) {
+					// Insert into a specific index.
+					_destinationIndex = insertionIndex;
+				} else if ( clientId ) {
+					// Insert after a specific client ID.
+					_destinationIndex = getBlockIndex(
+						clientId,
+						_destinationRootClientId
+					);
+				} else {
+					// Insert at the end of the list.
+					_destinationIndex = getBlockOrder(
+						_destinationRootClientId
+					).length;
 				}
+			} else {
+				// Otherwise, we're in "auto mode" where the insertion point is
+				// decided by getBlockInsertionPoint().
+				const insertionPoint = getBlockInsertionPoint();
+				_destinationRootClientId = insertionPoint.rootClientId;
+				_destinationIndex = insertionPoint.index;
 			}
+
 			return {
-				hasPatterns: !! getSettings().__experimentalBlockPatterns
-					?.length,
-				destinationRootClientId: destRootClientId,
-				...pick( select( 'core/block-editor' ), [
-					'getSelectedBlock',
-					'getBlockIndex',
-					'getBlockSelectionEnd',
-					'getBlockOrder',
-				] ),
+				selectedBlock: getSelectedBlock(),
+				destinationRootClientId: _destinationRootClientId,
+				destinationIndex: _destinationIndex,
 			};
 		},
-		[ isAppender, clientId, rootClientId ]
+		[ rootClientId, insertionIndex, clientId, isAppender ]
 	);
+
 	const {
 		replaceBlocks,
 		insertBlocks,
 		showInsertionPoint,
 		hideInsertionPoint,
 	} = useDispatch( 'core/block-editor' );
 
-	function getInsertionIndex() {
-		if ( insertionIndex !== undefined ) {
-			return insertionIndex;
-		}
-
-		// If the clientId is defined, we insert at the position of the block.
-		if ( clientId ) {
-			return getBlockIndex( clientId, destinationRootClientId );
-		}
-
-		// If there's a selected block, and the selected block is not the destination root block, we insert after the selected block.
-		const end = getBlockSelectionEnd();
-		if ( ! isAppender && end ) {
-			return getBlockIndex( end, destinationRootClientId ) + 1;
-		}
-
-		// Otherwise, we insert at the end of the current rootClientId
-		return getBlockOrder( destinationRootClientId ).length;
-	}
-
 	const onInsertBlocks = ( blocks, meta ) => {
-		const selectedBlock = getSelectedBlock();
 		if (
 			! isAppender &&
 			selectedBlock &&
@@ -107,7 +106,7 @@ function useInsertionPoint( {
 		} else {
 			insertBlocks(
 				blocks,
-				getInsertionIndex(),
+				destinationIndex,
 				destinationRootClientId,
 				selectBlockOnInsert,
 				meta
@@ -131,8 +130,7 @@ function useInsertionPoint( {
 
 	const onToggleInsertionPoint = ( show ) => {
 		if ( show ) {
-			const index = getInsertionIndex();
-			showInsertionPoint( destinationRootClientId, index );
+			showInsertionPoint( destinationRootClientId, destinationIndex );
 		} else {
 			hideInsertionPoint();
 		}

diff --git a/packages/block-editor/src/components/inserter/quick-inserter.js b/packages/block-editor/src/components/inserter/quick-inserter.js
@@ -154,17 +154,18 @@ export default function QuickInserter( {
 		[ filterValue, patterns ]
 	);
 
-	const setInserterIsOpened = useSelect(
-		( select ) =>
-			select( 'core/block-editor' ).getSettings()
-				.__experimentalSetIsInserterOpened,
-		[]
-	);
-
-	const previousBlockClientId = useSelect(
-		( select ) =>
-			select( 'core/block-editor' ).getPreviousBlockClientId( clientId ),
-		[ clientId ]
+	const { setInserterIsOpened, blockIndex } = useSelect(
+		( select ) => {
+			const { getSettings, getBlockIndex } = select(
+				'core/block-editor'
+			);
+			return {
+				setInserterIsOpened: getSettings()
+					.__experimentalSetIsInserterOpened,
+				blockIndex: getBlockIndex( clientId, rootClientId ),
+			};
+		},
+		[ clientId, rootClientId ]
 	);
 
 	useEffect( () => {
@@ -173,7 +174,7 @@ export default function QuickInserter( {
 		}
 	}, [ setInserterIsOpened ] );
 
-	const { selectBlock } = useDispatch( 'core/block-editor' );
+	const { __unstableSetInsertionPoint } = useDispatch( 'core/block-editor' );
 
 	// Announce search results on change
 	useEffect( () => {
@@ -189,17 +190,9 @@ export default function QuickInserter( {
 		debouncedSpeak( resultsFoundMessage );
 	}, [ filterValue, debouncedSpeak ] );
 
-	// When clicking Browse All select the appropriate block so as
-	// the insertion point can work as expected
 	const onBrowseAll = () => {
-		// We have to select the previous block because the menu inserter
-		// inserts the new block after the selected one.
-		// Ideally, this selection shouldn't focus the block to avoid the setTimeout.
-		selectBlock( previousBlockClientId );
-		// eslint-disable-next-line @wordpress/react-no-unsafe-timeout
-		setTimeout( () => {
-			setInserterIsOpened( true );
-		} );
+		__unstableSetInsertionPoint( rootClientId, blockIndex );
+		setInserterIsOpened( true );
 	};
 
 	// Disable reason (no-autofocus): The inserter menu is a modal display, not one which

diff --git a/packages/block-editor/src/store/actions.js b/packages/block-editor/src/store/actions.js
@@ -523,12 +523,34 @@ export function* insertBlocks(
 }
 
 /**
- * Returns an action object used in signalling that the insertion point should
- * be shown.
+ * Sets the insertion point without showing it to users.
  *
- * @param {?string} rootClientId Optional root client ID of block list on
- *                               which to insert.
- * @param {?number} index        Index at which block should be inserted.
+ * Components like <Inserter> will default to inserting blocks at this point.
+ *
+ * @param {?string} rootClientId Root client ID of block list in which to
+ *                               insert. Use `undefined` for the root block
+ *                               list.
+ * @param {number} index         Index at which block should be inserted.
+ *
+ * @return {Object} Action object.
+ */
+export function __unstableSetInsertionPoint( rootClientId, index ) {
+	return {
+		type: 'SET_INSERTION_POINT',
+		rootClientId,
+		index,
+	};
+}
+
+/**
+ * Sets the insertion point and shows it to users.
+ *
+ * Components like <Inserter> will default to inserting blocks at this point.
+ *
+ * @param {?string} rootClientId Root client ID of block list in which to
+ *                               insert. Use `undefined` for the root block
+ *                               list.
+ * @param {number} index         Index at which block should be inserted.
  *
  * @return {Object} Action object.
  */
@@ -541,7 +563,7 @@ export function showInsertionPoint( rootClientId, index ) {
 }
 
 /**
- * Returns an action object hiding the insertion point.
+ * Hides the insertion point for users.
  *
  * @return {Object} Action object.
  */