From 49a3c44aa7057343938004905655f800fd9619be Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Tue, 10 Nov 2020 15:55:33 +1100 Subject: [PATCH] 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 * 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 --- .../developers/data/data-core-block-editor.md | 30 +++-- .../inserter/hooks/use-insertion-point.js | 112 +++++++++--------- .../src/components/inserter/quick-inserter.js | 37 +++--- packages/block-editor/src/store/actions.js | 34 +++++- packages/block-editor/src/store/reducer.js | 53 ++++++++- packages/block-editor/src/store/selectors.js | 21 ++-- .../block-editor/src/store/test/actions.js | 21 ++++ .../block-editor/src/store/test/reducer.js | 80 ++++++++++--- .../block-editor/src/store/test/selectors.js | 11 +- .../__snapshots__/adding-blocks.test.js.snap | 12 ++ .../editor/various/adding-blocks.test.js | 15 +++ 11 files changed, 294 insertions(+), 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 index b4e3f2cd31a4ad..dd1c3d896137d8 100644 --- 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_ # **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 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`. # **getBlockListSettings** @@ -812,7 +818,8 @@ _Returns_ # **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_ @@ -820,7 +827,7 @@ _Parameters_ _Returns_ -- `?boolean`: Whether the insertion point is visible or not. +- `?boolean`: Whether the insertion point should be shown. # **isBlockMultiSelected** @@ -1048,7 +1055,7 @@ _Parameters_ # **hideInsertionPoint** -Returns an action object hiding the insertion point. +Hides the insertion point for users. _Returns_ @@ -1373,13 +1380,14 @@ _Returns_ # **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 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 index bb240e67859d43..db67c49f80354f 100644 --- 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,48 +29,66 @@ 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, @@ -76,28 +96,7 @@ function useInsertionPoint( { 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 index ac2c3f80b945fe..94930a0d0b043a 100644 --- 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 index 0bedccade322db..7aaca6c9fd1ec8 100644 --- 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 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 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. */ diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 85898175da7dac..20a57146bb42c4 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -1368,9 +1368,31 @@ export function blocksMode( state = {}, action ) { } /** - * Reducer returning the block insertion point visibility, either null if there - * is not an explicit insertion point assigned, or an object of its `index` and - * `rootClientId`. + * A helper for resetting the insertion point state. + * + * @param {Object} state Current state. + * @param {Object} action Dispatched action. + * @param {*} defaultValue The default value for the reducer. + * + * @return {*} Either the default value if a reset is required, or the state. + */ +function resetInsertionPoint( state, action, defaultValue ) { + switch ( action.type ) { + case 'CLEAR_SELECTED_BLOCK': + case 'SELECT_BLOCK': + case 'REPLACE_INNER_BLOCKS': + case 'INSERT_BLOCKS': + case 'REMOVE_BLOCKS': + case 'REPLACE_BLOCKS': + return defaultValue; + } + + return state; +} + +/** + * Reducer returning the insertion point position, consisting of the + * rootClientId and an index. * * @param {Object} state Current state. * @param {Object} action Dispatched action. @@ -1379,15 +1401,33 @@ export function blocksMode( state = {}, action ) { */ export function insertionPoint( state = null, action ) { switch ( action.type ) { - case 'SHOW_INSERTION_POINT': + case 'SET_INSERTION_POINT': + case 'SHOW_INSERTION_POINT': { const { rootClientId, index } = action; return { rootClientId, index }; + } + } + + return resetInsertionPoint( state, action, null ); +} +/** + * Reducer returning the visibility of the insertion point. + * + * @param {Object} state Current state. + * @param {Object} action Dispatched action. + * + * @return {Object} Updated state. + */ +export function insertionPointVisibility( state = false, action ) { + switch ( action.type ) { + case 'SHOW_INSERTION_POINT': + return true; case 'HIDE_INSERTION_POINT': - return null; + return false; } - return state; + return resetInsertionPoint( state, action, false ); } /** @@ -1658,6 +1698,7 @@ export default combineReducers( { blocksMode, blockListSettings, insertionPoint, + insertionPointVisibility, template, settings, preferences, diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index 2647cbac74f13f..c2153ee33cb4b1 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -1138,12 +1138,18 @@ export function isCaretWithinFormattedText( state ) { } /** - * 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: * - * @param {Object} state Editor state. + * 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 will default to inserting blocks at this point. + * + * @param {Object} state Global application state. * - * @return {Object} Insertion point object with `rootClientId`, `index`. + * @return {Object} Insertion point object with `rootClientId` and `index`. */ export function getBlockInsertionPoint( state ) { let rootClientId, index; @@ -1166,14 +1172,15 @@ export function getBlockInsertionPoint( state ) { } /** - * 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(). * * @param {Object} state Global application state. * - * @return {?boolean} Whether the insertion point is visible or not. + * @return {?boolean} Whether the insertion point should be shown. */ export function isBlockInsertionPointVisible( state ) { - return state.insertionPoint !== null; + return state.insertionPointVisibility; } /** diff --git a/packages/block-editor/src/store/test/actions.js b/packages/block-editor/src/store/test/actions.js index dc7ad96e3261a7..94379114f31bfd 100644 --- a/packages/block-editor/src/store/test/actions.js +++ b/packages/block-editor/src/store/test/actions.js @@ -24,6 +24,7 @@ import { resetBlocks, selectBlock, selectPreviousBlock, + __unstableSetInsertionPoint, showInsertionPoint, startMultiSelect, startTyping, @@ -699,11 +700,31 @@ describe( 'actions', () => { } ); } ); + describe( '__unstableSetInsertionPoint', () => { + it( 'should return the SET_INSERTION_POINT action', () => { + expect( __unstableSetInsertionPoint() ).toEqual( { + type: 'SET_INSERTION_POINT', + } ); + expect( __unstableSetInsertionPoint( 'rootClientId', 2 ) ).toEqual( + { + type: 'SET_INSERTION_POINT', + rootClientId: 'rootClientId', + index: 2, + } + ); + } ); + } ); + describe( 'showInsertionPoint', () => { it( 'should return the SHOW_INSERTION_POINT action', () => { expect( showInsertionPoint() ).toEqual( { type: 'SHOW_INSERTION_POINT', } ); + expect( showInsertionPoint( 'rootClientId', 2 ) ).toEqual( { + type: 'SHOW_INSERTION_POINT', + rootClientId: 'rootClientId', + index: 2, + } ); } ); } ); diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index c1cf2cf43e9b7e..3906c5c4c4c1f0 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -30,6 +30,7 @@ import { preferences, blocksMode, insertionPoint, + insertionPointVisibility, template, blockListSettings, lastBlockAttributesChange, @@ -2015,35 +2016,82 @@ describe( 'state', () => { } ); describe( 'insertionPoint', () => { - it( 'should default to null', () => { + it( 'defaults to `null`', () => { const state = insertionPoint( undefined, {} ); - expect( state ).toBe( null ); + expect( state ).toEqual( null ); } ); - it( 'should set insertion point', () => { - const state = insertionPoint( null, { - type: 'SHOW_INSERTION_POINT', - rootClientId: 'clientId1', - index: 0, - } ); + it.each( [ 'SET_INSERTION_POINT', 'SHOW_INSERTION_POINT' ] )( + 'sets the insertion point on %s', + ( type ) => { + const original = deepFreeze( { + rootClientId: 'clientId1', + index: 0, + } ); - expect( state ).toEqual( { - rootClientId: 'clientId1', - index: 0, - } ); - } ); + const expectedNewState = { + rootClientId: 'clientId2', + index: 1, + }; - it( 'should clear the insertion point', () => { + const state = insertionPoint( original, { + type, + ...expectedNewState, + } ); + + expect( state ).toEqual( expectedNewState ); + } + ); + + it.each( [ + 'CLEAR_SELECTED_BLOCK', + 'SELECT_BLOCK', + 'REPLACE_INNER_BLOCKS', + 'INSERT_BLOCKS', + 'REMOVE_BLOCKS', + 'REPLACE_BLOCKS', + ] )( 'resets the insertion point to `null` on %s', ( type ) => { const original = deepFreeze( { rootClientId: 'clientId1', index: 0, } ); const state = insertionPoint( original, { - type: 'HIDE_INSERTION_POINT', + type, } ); - expect( state ).toBe( null ); + expect( state ).toEqual( null ); + } ); + } ); + + describe( 'insertionPointVisibility', () => { + it( 'defaults to `false`', () => { + const state = insertionPointVisibility( undefined, {} ); + expect( state ).toBe( false ); + } ); + + it( 'shows the insertion point', () => { + const state = insertionPointVisibility( false, { + type: 'SHOW_INSERTION_POINT', + } ); + + expect( state ).toBe( true ); + } ); + + it.each( [ + 'HIDE_INSERTION_POINT', + 'CLEAR_SELECTED_BLOCK', + 'SELECT_BLOCK', + 'REPLACE_INNER_BLOCKS', + 'INSERT_BLOCKS', + 'REMOVE_BLOCKS', + 'REPLACE_BLOCKS', + ] )( 'sets the insertion point on %s to `false`', ( type ) => { + const state = insertionPointVisibility( true, { + type, + } ); + + expect( state ).toBe( false ); } ); } ); diff --git a/packages/block-editor/src/store/test/selectors.js b/packages/block-editor/src/store/test/selectors.js index 8fd1b3df71cc01..8656f069347cfd 100644 --- a/packages/block-editor/src/store/test/selectors.js +++ b/packages/block-editor/src/store/test/selectors.js @@ -2155,20 +2155,17 @@ describe( 'selectors', () => { } ); describe( 'isBlockInsertionPointVisible', () => { - it( 'should return false if no assigned insertion point', () => { + it( 'should return false if insertion point is set to not show', () => { const state = { - insertionPoint: null, + insertionPointVisibility: false, }; expect( isBlockInsertionPointVisible( state ) ).toBe( false ); } ); - it( 'should return true if assigned insertion point', () => { + it( 'should return true if insertion point is set to show', () => { const state = { - insertionPoint: { - rootClientId: undefined, - index: 5, - }, + insertionPointVisibility: true, }; expect( isBlockInsertionPointVisible( state ) ).toBe( true ); diff --git a/packages/e2e-tests/specs/editor/various/__snapshots__/adding-blocks.test.js.snap b/packages/e2e-tests/specs/editor/various/__snapshots__/adding-blocks.test.js.snap index 16a83b6266b96b..9870b5be306fa7 100644 --- a/packages/e2e-tests/specs/editor/various/__snapshots__/adding-blocks.test.js.snap +++ b/packages/e2e-tests/specs/editor/various/__snapshots__/adding-blocks.test.js.snap @@ -53,6 +53,18 @@ lines preserved[/myshortcode] " `; +exports[`adding blocks inserts a block in proper place after having clicked \`Browse All\` from block appender 1`] = ` +" +
+

Paragraph inside group

+
+ + + +

Paragraph after group

+" +`; + exports[`adding blocks inserts a block in proper place after having clicked \`Browse All\` from inline inserter 1`] = ` "

First paragraph

diff --git a/packages/e2e-tests/specs/editor/various/adding-blocks.test.js b/packages/e2e-tests/specs/editor/various/adding-blocks.test.js index efc2a6cf6a70b4..6c2700e0a28f07 100644 --- a/packages/e2e-tests/specs/editor/various/adding-blocks.test.js +++ b/packages/e2e-tests/specs/editor/various/adding-blocks.test.js @@ -297,4 +297,19 @@ describe( 'adding blocks', () => { expect( indicatorRect.x ).toBe( paragraphRect.x ); expect( indicatorRect.y > paragraphRect.y ).toBe( true ); } ); + + // Check for regression of https://github.com/WordPress/gutenberg/issues/24403 + it( 'inserts a block in proper place after having clicked `Browse All` from block appender', async () => { + await insertBlock( 'Group' ); + await insertBlock( 'Paragraph' ); + await page.keyboard.type( 'Paragraph after group' ); + await page.click( '[data-type="core/group"] [aria-label="Add block"]' ); + const browseAll = await page.waitForXPath( + '//button[text()="Browse all"]' + ); + await browseAll.click(); + await insertBlock( 'Paragraph' ); + await page.keyboard.type( 'Paragraph inside group' ); + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); } );