From 03be3e74218b4c1b418e22bb2f7bfe51cd2a7dc3 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 15 Dec 2023 10:25:29 +0800 Subject: [PATCH 1/4] Fix broken undo history stack for Pattern Overrides --- packages/block-editor/src/private-apis.js | 2 + .../block-editor/src/store/private-actions.js | 35 ++++++++++++-- packages/block-editor/src/store/reducer.js | 14 ++++-- .../block-editor/src/store/undo-ignore.js | 4 ++ packages/block-library/src/block/edit.js | 47 ++++++++++--------- packages/core-data/src/actions.js | 9 +++- 6 files changed, 80 insertions(+), 31 deletions(-) create mode 100644 packages/block-editor/src/store/undo-ignore.js diff --git a/packages/block-editor/src/private-apis.js b/packages/block-editor/src/private-apis.js index 74bf4af421dfb..f8d858b158a49 100644 --- a/packages/block-editor/src/private-apis.js +++ b/packages/block-editor/src/private-apis.js @@ -24,6 +24,7 @@ import { import { usesContextKey } from './components/rich-text/format-edit'; import { ExperimentalBlockCanvas } from './components/block-canvas'; import { getDuotoneFilter } from './components/duotone/utils'; +import { undoIgnoreBlocks } from './store/undo-ignore'; /** * Private @wordpress/block-editor APIs. @@ -52,4 +53,5 @@ lock( privateApis, { ReusableBlocksRenameHint, useReusableBlocksRenameHint, usesContextKey, + undoIgnoreBlocks, } ); diff --git a/packages/block-editor/src/store/private-actions.js b/packages/block-editor/src/store/private-actions.js index 1c29948d81416..48c5d15d469be 100644 --- a/packages/block-editor/src/store/private-actions.js +++ b/packages/block-editor/src/store/private-actions.js @@ -3,6 +3,11 @@ */ import { Platform } from '@wordpress/element'; +/** + * Internal dependencies + */ +import { undoIgnoreBlocks } from './undo-ignore'; + const castArray = ( maybeArray ) => Array.isArray( maybeArray ) ? maybeArray : [ maybeArray ]; @@ -291,10 +296,30 @@ export function deleteStyleOverride( id ) { }; } -export function syncDerivedBlockAttributes( clientId, attributes ) { - return { - type: 'SYNC_DERIVED_BLOCK_ATTRIBUTES', - clientIds: [ clientId ], - attributes, +/** + * A higher-order action that mark every change inside a callback as "non-persistent" + * and ignore pushing to the undo history stack. It's primarily used for synchronized + * derived updates from the block editor without affecting the undo history. + * + * @param {() => void} callback The synchronous callback to derive updates. + */ +export function syncDerivedUpdates( callback ) { + return ( { dispatch, select, registry } ) => { + registry.batch( () => { + // Mark every change in the `callback` as non-persistent. + dispatch( { + type: 'SET_EXPLICIT_PERSISTENT', + isPersistentChange: false, + } ); + callback(); + dispatch( { + type: 'SET_EXPLICIT_PERSISTENT', + isPersistentChange: undefined, + } ); + + // Ignore pushing undo stack for the updated blocks. + const updatedBlocks = select.getBlocks(); + undoIgnoreBlocks.add( updatedBlocks ); + } ); }; } diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 5319a3b255365..b5d76c58ff3f2 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -453,13 +453,21 @@ const withBlockTree = function withPersistentBlockChange( reducer ) { let lastAction; let markNextChangeAsNotPersistent = false; + let explicitPersistent; return ( state, action ) => { let nextState = reducer( state, action ); - if ( action.type === 'SYNC_DERIVED_BLOCK_ATTRIBUTES' ) { - return nextState.isPersistentChange - ? { ...nextState, isPersistentChange: false } + if ( action.type === 'SET_EXPLICIT_PERSISTENT' ) { + explicitPersistent = action.isPersistentChange; + } + + if ( explicitPersistent !== undefined ) { + return explicitPersistent !== nextState.isPersistentChange + ? { + ...nextState, + isPersistentChange: explicitPersistent, + } : nextState; } diff --git a/packages/block-editor/src/store/undo-ignore.js b/packages/block-editor/src/store/undo-ignore.js new file mode 100644 index 0000000000000..dbe19ac0a5499 --- /dev/null +++ b/packages/block-editor/src/store/undo-ignore.js @@ -0,0 +1,4 @@ +// Keep track of the blocks that should not be pushing an additional +// undo stack when editing the entity. +// See the implementation of `syncDerivedUpdates` and `editEntityRecord`. +export const undoIgnoreBlocks = new WeakSet(); diff --git a/packages/block-library/src/block/edit.js b/packages/block-library/src/block/edit.js index fbfef0b4cf177..67b2680f6840e 100644 --- a/packages/block-library/src/block/edit.js +++ b/packages/block-library/src/block/edit.js @@ -137,6 +137,7 @@ export default function ReusableBlockEdit( { attributes: { ref, overrides }, __unstableParentLayout: parentLayout, clientId: patternClientId, + setAttributes, } ) { const registry = useRegistry(); const hasAlreadyRendered = useHasRecursion( ref ); @@ -154,7 +155,9 @@ export default function ReusableBlockEdit( { setBlockEditingMode, } = useDispatch( blockEditorStore ); const { getBlockEditingMode } = useSelect( blockEditorStore ); + const { syncDerivedUpdates } = unlock( useDispatch( blockEditorStore ) ); + // Apply the initial overrides from the pattern block to the inner blocks. useEffect( () => { const initialBlocks = editedRecord.blocks ?? @@ -164,17 +167,19 @@ export default function ReusableBlockEdit( { defaultValuesRef.current = {}; const editingMode = getBlockEditingMode( patternClientId ); + // Replace the contents of the blocks with the overrides. registry.batch( () => { setBlockEditingMode( patternClientId, 'default' ); - __unstableMarkNextChangeAsNotPersistent(); - replaceInnerBlocks( - patternClientId, - applyInitialOverrides( - initialBlocks, - initialOverrides.current, - defaultValuesRef.current - ) - ); + syncDerivedUpdates( () => { + replaceInnerBlocks( + patternClientId, + applyInitialOverrides( + initialBlocks, + initialOverrides.current, + defaultValuesRef.current + ) + ); + } ); setBlockEditingMode( patternClientId, editingMode ); } ); }, [ @@ -185,6 +190,7 @@ export default function ReusableBlockEdit( { registry, getBlockEditingMode, setBlockEditingMode, + syncDerivedUpdates, ] ); const innerBlocks = useSelect( @@ -220,29 +226,26 @@ export default function ReusableBlockEdit( { : InnerBlocks.ButtonBlockAppender, } ); - // Sync the `overrides` attribute from the updated blocks. - // `syncDerivedBlockAttributes` is an action that just like `updateBlockAttributes` - // but won't create an undo level. - // This can be abstracted into a `useSyncDerivedAttributes` hook if needed. + // Sync the `overrides` attribute from the updated blocks to the pattern block. + // `syncDerivedUpdates` is used here to avoid creating an additional undo level. useEffect( () => { const { getBlocks } = registry.select( blockEditorStore ); - const { syncDerivedBlockAttributes } = unlock( - registry.dispatch( blockEditorStore ) - ); let prevBlocks = getBlocks( patternClientId ); return registry.subscribe( () => { const blocks = getBlocks( patternClientId ); if ( blocks !== prevBlocks ) { prevBlocks = blocks; - syncDerivedBlockAttributes( patternClientId, { - overrides: getOverridesFromBlocks( - blocks, - defaultValuesRef.current - ), + syncDerivedUpdates( () => { + setAttributes( { + overrides: getOverridesFromBlocks( + blocks, + defaultValuesRef.current + ), + } ); } ); } }, blockEditorStore ); - }, [ patternClientId, registry ] ); + }, [ syncDerivedUpdates, patternClientId, registry, setAttributes ] ); let children = null; diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 49776d0562984..4fd76c1a520c7 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -10,6 +10,7 @@ import { v4 as uuid } from 'uuid'; import apiFetch from '@wordpress/api-fetch'; import { addQueryArgs } from '@wordpress/url'; import deprecated from '@wordpress/deprecated'; +import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor'; /** * Internal dependencies @@ -20,6 +21,9 @@ import { getOrLoadEntitiesConfig, DEFAULT_ENTITY_KEY } from './entities'; import { createBatch } from './batch'; import { STORE_NAME } from './name'; import { getSyncProvider } from './sync'; +import { unlock } from './private-apis'; + +const { undoIgnoreBlocks } = unlock( blockEditorPrivateApis ); /** * Returns an action object used in signalling that authors have been received. @@ -405,7 +409,10 @@ export const editEntityRecord = ); } } else { - if ( ! options.undoIgnore ) { + if ( + ! options.undoIgnore && + ! undoIgnoreBlocks.has( edit.edits.blocks ) + ) { select.getUndoManager().addRecord( [ { From 9a914ce9c66667c431b5181f7ec23d7ca14eae26 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 15 Dec 2023 14:06:58 +0800 Subject: [PATCH 2/4] Fix unit test --- .../src/components/provider/use-block-sync.js | 2 ++ packages/block-editor/src/private-apis.js | 2 -- packages/block-editor/src/store/reducer.js | 15 +++++++++------ packages/block-editor/src/store/undo-ignore.js | 2 +- packages/core-data/src/actions.js | 9 +-------- packages/core-data/src/entity-provider.js | 14 ++++++++++---- .../src/components/document-outline/test/index.js | 4 ---- 7 files changed, 23 insertions(+), 25 deletions(-) diff --git a/packages/block-editor/src/components/provider/use-block-sync.js b/packages/block-editor/src/components/provider/use-block-sync.js index 4f2300f380892..a9f7df2eebcba 100644 --- a/packages/block-editor/src/components/provider/use-block-sync.js +++ b/packages/block-editor/src/components/provider/use-block-sync.js @@ -9,6 +9,7 @@ import { cloneBlock } from '@wordpress/blocks'; * Internal dependencies */ import { store as blockEditorStore } from '../../store'; +import { undoIgnoreBlocks } from '../../store/undo-ignore'; const noop = () => {}; @@ -271,6 +272,7 @@ export default function useBlockSync( { initialPosition: getSelectedBlocksInitialCaretPosition(), }, + undoIgnore: undoIgnoreBlocks.has( blocks ), } ); } previousAreBlocksDifferent = areBlocksDifferent; diff --git a/packages/block-editor/src/private-apis.js b/packages/block-editor/src/private-apis.js index f8d858b158a49..74bf4af421dfb 100644 --- a/packages/block-editor/src/private-apis.js +++ b/packages/block-editor/src/private-apis.js @@ -24,7 +24,6 @@ import { import { usesContextKey } from './components/rich-text/format-edit'; import { ExperimentalBlockCanvas } from './components/block-canvas'; import { getDuotoneFilter } from './components/duotone/utils'; -import { undoIgnoreBlocks } from './store/undo-ignore'; /** * Private @wordpress/block-editor APIs. @@ -53,5 +52,4 @@ lock( privateApis, { ReusableBlocksRenameHint, useReusableBlocksRenameHint, usesContextKey, - undoIgnoreBlocks, } ); diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index b5d76c58ff3f2..11811afd83f6f 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -458,17 +458,20 @@ function withPersistentBlockChange( reducer ) { return ( state, action ) => { let nextState = reducer( state, action ); + let nextIsPersistentChange; if ( action.type === 'SET_EXPLICIT_PERSISTENT' ) { explicitPersistent = action.isPersistentChange; + nextIsPersistentChange = state.isPersistentChange ?? true; } if ( explicitPersistent !== undefined ) { - return explicitPersistent !== nextState.isPersistentChange - ? { + nextIsPersistentChange = explicitPersistent; + return nextIsPersistentChange === nextState.isPersistentChange + ? nextState + : { ...nextState, - isPersistentChange: explicitPersistent, - } - : nextState; + isPersistentChange: nextIsPersistentChange, + }; } const isExplicitPersistentChange = @@ -481,7 +484,7 @@ function withPersistentBlockChange( reducer ) { markNextChangeAsNotPersistent = action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT'; - const nextIsPersistentChange = state?.isPersistentChange ?? true; + nextIsPersistentChange = state?.isPersistentChange ?? true; if ( state.isPersistentChange === nextIsPersistentChange ) { return state; } diff --git a/packages/block-editor/src/store/undo-ignore.js b/packages/block-editor/src/store/undo-ignore.js index dbe19ac0a5499..f0a64428ea7c2 100644 --- a/packages/block-editor/src/store/undo-ignore.js +++ b/packages/block-editor/src/store/undo-ignore.js @@ -1,4 +1,4 @@ // Keep track of the blocks that should not be pushing an additional // undo stack when editing the entity. -// See the implementation of `syncDerivedUpdates` and `editEntityRecord`. +// See the implementation of `syncDerivedUpdates` and `useBlockSync`. export const undoIgnoreBlocks = new WeakSet(); diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 4fd76c1a520c7..49776d0562984 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -10,7 +10,6 @@ import { v4 as uuid } from 'uuid'; import apiFetch from '@wordpress/api-fetch'; import { addQueryArgs } from '@wordpress/url'; import deprecated from '@wordpress/deprecated'; -import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor'; /** * Internal dependencies @@ -21,9 +20,6 @@ import { getOrLoadEntitiesConfig, DEFAULT_ENTITY_KEY } from './entities'; import { createBatch } from './batch'; import { STORE_NAME } from './name'; import { getSyncProvider } from './sync'; -import { unlock } from './private-apis'; - -const { undoIgnoreBlocks } = unlock( blockEditorPrivateApis ); /** * Returns an action object used in signalling that authors have been received. @@ -409,10 +405,7 @@ export const editEntityRecord = ); } } else { - if ( - ! options.undoIgnore && - ! undoIgnoreBlocks.has( edit.edits.blocks ) - ) { + if ( ! options.undoIgnore ) { select.getUndoManager().addRecord( [ { diff --git a/packages/core-data/src/entity-provider.js b/packages/core-data/src/entity-provider.js index 4b82b62e318bc..5dc19f5225c76 100644 --- a/packages/core-data/src/entity-provider.js +++ b/packages/core-data/src/entity-provider.js @@ -196,7 +196,7 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) { if ( noChange ) { return __unstableCreateUndoLevel( kind, name, id ); } - const { selection } = options; + const { selection, ...rest } = options; // We create a new function here on every persistent edit // to make sure the edit makes the post dirty and creates @@ -208,7 +208,10 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) { ...updateFootnotes( newBlocks ), }; - editEntityRecord( kind, name, id, edits, { isCached: false } ); + editEntityRecord( kind, name, id, edits, { + isCached: false, + ...rest, + } ); }, [ kind, @@ -223,11 +226,14 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) { const onInput = useCallback( ( newBlocks, options ) => { - const { selection } = options; + const { selection, ...rest } = options; const footnotesChanges = updateFootnotes( newBlocks ); const edits = { selection, ...footnotesChanges }; - editEntityRecord( kind, name, id, edits, { isCached: true } ); + editEntityRecord( kind, name, id, edits, { + isCached: true, + ...rest, + } ); }, [ kind, name, id, updateFootnotes, editEntityRecord ] ); diff --git a/packages/editor/src/components/document-outline/test/index.js b/packages/editor/src/components/document-outline/test/index.js index 21c258c9a65df..bf9954fe9b1f7 100644 --- a/packages/editor/src/components/document-outline/test/index.js +++ b/packages/editor/src/components/document-outline/test/index.js @@ -17,10 +17,6 @@ import { */ import { DocumentOutline } from '../'; -jest.mock( '@wordpress/block-editor', () => ( { - BlockTitle: () => 'Block Title', -} ) ); - describe( 'DocumentOutline', () => { let paragraph, headingH1, headingH2, headingH3, nestedHeading; beforeAll( () => { From d1fbba32c5b0664c5c494206396e4e2e44ecc8ae Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 19 Dec 2023 14:57:15 +0800 Subject: [PATCH 3/4] Fix unit test --- .../provider/test/use-block-sync.js | 37 ++++++++++--------- .../src/components/provider/use-block-sync.js | 6 ++- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/block-editor/src/components/provider/test/use-block-sync.js b/packages/block-editor/src/components/provider/test/use-block-sync.js index 09529c197516a..9b16e966249fa 100644 --- a/packages/block-editor/src/components/provider/test/use-block-sync.js +++ b/packages/block-editor/src/components/provider/test/use-block-sync.js @@ -263,13 +263,13 @@ describe( 'useBlockSync hook', () => { expect( onInput ).toHaveBeenCalledWith( [ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ], - { + expect.objectContaining( { selection: { selectionEnd: {}, selectionStart: {}, initialPosition: null, }, - } + } ) ); expect( onChange ).not.toHaveBeenCalled(); } ); @@ -303,13 +303,13 @@ describe( 'useBlockSync hook', () => { expect( onChange ).toHaveBeenCalledWith( [ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ], - { + expect.objectContaining( { selection: { selectionEnd: {}, selectionStart: {}, initialPosition: null, }, - } + } ) ); expect( onInput ).not.toHaveBeenCalled(); } ); @@ -406,13 +406,13 @@ describe( 'useBlockSync hook', () => { attributes: { foo: 2 }, }, ], - { + expect.objectContaining( { selection: { selectionEnd: {}, selectionStart: {}, initialPosition: null, }, - } + } ) ); expect( onInput ).not.toHaveBeenCalled(); } ); @@ -447,13 +447,16 @@ describe( 'useBlockSync hook', () => { { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } }, ]; - expect( onChange1 ).toHaveBeenCalledWith( updatedBlocks1, { - selection: { - initialPosition: null, - selectionEnd: {}, - selectionStart: {}, - }, - } ); + expect( onChange1 ).toHaveBeenCalledWith( + updatedBlocks1, + expect.objectContaining( { + selection: { + initialPosition: null, + selectionEnd: {}, + selectionStart: {}, + }, + } ) + ); const newBlocks = [ { clientId: 'b', innerBlocks: [], attributes: { foo: 1 } }, @@ -485,13 +488,13 @@ describe( 'useBlockSync hook', () => { // The second callback should be called with the new change. expect( onChange2 ).toHaveBeenCalledWith( [ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ], - { + expect.objectContaining( { selection: { selectionEnd: {}, selectionStart: {}, initialPosition: null, }, - } + } ) ); } ); @@ -544,13 +547,13 @@ describe( 'useBlockSync hook', () => { // Only the new callback should be called. expect( onChange2 ).toHaveBeenCalledWith( [ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ], - { + expect.objectContaining( { selection: { selectionEnd: {}, selectionStart: {}, initialPosition: null, }, - } + } ) ); } ); } ); diff --git a/packages/block-editor/src/components/provider/use-block-sync.js b/packages/block-editor/src/components/provider/use-block-sync.js index a9f7df2eebcba..969c0f1e4d1c5 100644 --- a/packages/block-editor/src/components/provider/use-block-sync.js +++ b/packages/block-editor/src/components/provider/use-block-sync.js @@ -265,6 +265,10 @@ export default function useBlockSync( { const updateParent = isPersistent ? onChangeRef.current : onInputRef.current; + const undoIgnore = undoIgnoreBlocks.has( blocks ); + if ( undoIgnore ) { + undoIgnoreBlocks.delete( blocks ); + } updateParent( blocks, { selection: { selectionStart: getSelectionStart(), @@ -272,7 +276,7 @@ export default function useBlockSync( { initialPosition: getSelectedBlocksInitialCaretPosition(), }, - undoIgnore: undoIgnoreBlocks.has( blocks ), + undoIgnore, } ); } previousAreBlocksDifferent = areBlocksDifferent; From f5c1a788ae17f9b6bc9bbbef41b7c07fa5781c39 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 22 Dec 2023 09:50:05 +0800 Subject: [PATCH 4/4] Revert uneeded changes --- packages/editor/src/components/document-outline/test/index.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/editor/src/components/document-outline/test/index.js b/packages/editor/src/components/document-outline/test/index.js index bf9954fe9b1f7..21c258c9a65df 100644 --- a/packages/editor/src/components/document-outline/test/index.js +++ b/packages/editor/src/components/document-outline/test/index.js @@ -17,6 +17,10 @@ import { */ import { DocumentOutline } from '../'; +jest.mock( '@wordpress/block-editor', () => ( { + BlockTitle: () => 'Block Title', +} ) ); + describe( 'DocumentOutline', () => { let paragraph, headingH1, headingH2, headingH3, nestedHeading; beforeAll( () => {