From 949bef382507874c543e386eca4223d64a9e9562 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 11 Apr 2019 11:30:32 -0400 Subject: [PATCH] Block Editor: Consider received blocks state change as ignored (#14916) * Block Editor: Avoid creating new state reference on unchanging isPersistentChange * Block Editor: Consider received blocks state change as ignored * fixup a35fa8269 * Block Editor: Regenerate documentation --- .../developers/data/data-core-block-editor.md | 14 +++++ packages/block-editor/CHANGELOG.md | 5 ++ .../src/components/provider/index.js | 8 ++- packages/block-editor/src/store/reducer.js | 57 ++++++++++++------- packages/block-editor/src/store/selectors.js | 18 ++++++ .../block-editor/src/store/test/reducer.js | 32 +++++++++-- .../e2e-tests/specs/change-detection.test.js | 17 ++++++ 7 files changed, 124 insertions(+), 27 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 fb3f337dd59cd..dcdff0b67aa14 100644 --- a/docs/designers-developers/developers/data/data-core-block-editor.md +++ b/docs/designers-developers/developers/data/data-core-block-editor.md @@ -763,6 +763,20 @@ via its `onChange` callback, in addition to `onInput`. Whether the most recent block change was persistent. +### __unstableIsLastBlockChangeIgnored + +Returns true if the most recent block change is be considered ignored, or +false otherwise. An ignored change is one not to be committed by +BlockEditorProvider, neither via `onChange` nor `onInput`. + +*Parameters* + + * state: Block editor state. + +*Returns* + +Whether the most recent block change was ignored. + ## Actions ### resetBlocks diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index 4306b855955f5..e93c6b24e4354 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -4,6 +4,11 @@ - `CopyHandler` will now only catch cut/copy events coming from its `props.children`, instead of from anywhere in the `document`. +### Internal + +- Improved handling of blocks state references for unchanging states. +- Updated handling of blocks state to effectively ignored programmatically-received blocks data (e.g. reusable blocks received from editor). + ## 1.0.0 (2019-03-06) ### New Features diff --git a/packages/block-editor/src/components/provider/index.js b/packages/block-editor/src/components/provider/index.js index fd788477a0d6d..6366cef566012 100644 --- a/packages/block-editor/src/components/provider/index.js +++ b/packages/block-editor/src/components/provider/index.js @@ -64,6 +64,7 @@ class BlockEditorProvider extends Component { const { getBlocks, isLastBlockChangePersistent, + __unstableIsLastBlockChangeIgnored, } = registry.select( 'core/block-editor' ); let blocks = getBlocks(); @@ -76,7 +77,12 @@ class BlockEditorProvider extends Component { } = this.props; const newBlocks = getBlocks(); const newIsPersistent = isLastBlockChangePersistent(); - if ( newBlocks !== blocks && this.isSyncingIncomingValue ) { + if ( + newBlocks !== blocks && ( + this.isSyncingIncomingValue || + __unstableIsLastBlockChangeIgnored() + ) + ) { this.isSyncingIncomingValue = false; blocks = newBlocks; isPersistent = newIsPersistent; diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 5e91c85f524bc..0c7448d1a2a74 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -188,16 +188,6 @@ export function isUpdatingSameBlockAttribute( action, lastAction ) { function withPersistentBlockChange( reducer ) { let lastAction; - /** - * Set of action types for which a blocks state change should be considered - * non-persistent. - * - * @type {Set} - */ - const IGNORED_ACTION_TYPES = new Set( [ - 'RECEIVE_BLOCKS', - ] ); - return ( state, action ) => { let nextState = reducer( state, action ); @@ -206,19 +196,14 @@ function withPersistentBlockChange( reducer ) { // Defer to previous state value (or default) unless changing or // explicitly marking as persistent. if ( state === nextState && ! isExplicitPersistentChange ) { - return { - ...nextState, - isPersistentChange: get( state, [ 'isPersistentChange' ], true ), - }; - } + const nextIsPersistentChange = get( state, [ 'isPersistentChange' ], true ); + if ( state.isPersistentChange === nextIsPersistentChange ) { + return state; + } - // Some state changes should not be considered persistent, namely those - // which are not a direct result of user interaction. - const isIgnoredActionType = IGNORED_ACTION_TYPES.has( action.type ); - if ( isIgnoredActionType ) { return { ...nextState, - isPersistentChange: false, + isPersistentChange: nextIsPersistentChange, }; } @@ -239,6 +224,37 @@ function withPersistentBlockChange( reducer ) { }; } +/** + * Higher-order reducer intended to augment the blocks reducer, assigning an + * `isIgnoredChange` property value corresponding to whether a change in state + * can be considered as ignored. A change is considered ignored when the result + * of an action not incurred by direct user interaction. + * + * @param {Function} reducer Original reducer function. + * + * @return {Function} Enhanced reducer function. + */ +function withIgnoredBlockChange( reducer ) { + /** + * Set of action types for which a blocks state change should be ignored. + * + * @type {Set} + */ + const IGNORED_ACTION_TYPES = new Set( [ + 'RECEIVE_BLOCKS', + ] ); + + return ( state, action ) => { + const nextState = reducer( state, action ); + + if ( nextState !== state ) { + nextState.isIgnoredChange = IGNORED_ACTION_TYPES.has( action.type ); + } + + return nextState; + }; +} + /** * Higher-order reducer targeting the combined blocks reducer, augmenting * block client IDs in remove action to include cascade of inner blocks. @@ -380,6 +396,7 @@ export const blocks = flow( withBlockReset, withSaveReusableBlock, withPersistentBlockChange, + withIgnoredBlockChange, )( { byClientId( state = {}, action ) { switch ( action.type ) { diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index d1b142530cff6..657ac6f4116ab 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -1367,6 +1367,24 @@ export function isLastBlockChangePersistent( state ) { return state.blocks.isPersistentChange; } +/** + * Returns true if the most recent block change is be considered ignored, or + * false otherwise. An ignored change is one not to be committed by + * BlockEditorProvider, neither via `onChange` nor `onInput`. + * + * @param {Object} state Block editor state. + * + * @return {boolean} Whether the most recent block change was ignored. + */ +export function __unstableIsLastBlockChangeIgnored( state ) { + // TODO: Removal Plan: Changes incurred by RECEIVE_BLOCKS should not be + // ignored if in-fact they result in a change in blocks state. The current + // need to ignore changes not a result of user interaction should be + // accounted for in the refactoring of reusable blocks as occurring within + // their own separate block editor / state (#7119). + return state.blocks.isIgnoredChange; +} + /** * Returns the value of a post meta from the editor settings. * diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index 62adf23f298fd..fe63fc34cd65b 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -233,7 +233,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -295,7 +296,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -386,7 +388,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -478,7 +481,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -512,6 +516,7 @@ describe( 'state', () => { attributes: {}, order: {}, isPersistentChange: true, + isIgnoredChange: false, } ); } ); @@ -1500,7 +1505,22 @@ describe( 'state', () => { expect( state.isPersistentChange ).toBe( true ); } ); - it( 'should not consider received blocks as persistent change', () => { + it( 'should retain reference for same state, same persistence', () => { + const original = deepFreeze( blocks( undefined, { + type: 'RESET_BLOCKS', + blocks: [], + } ) ); + + const state = blocks( original, { + type: '__INERT__', + } ); + + expect( state ).toBe( original ); + } ); + } ); + + describe( 'isIgnoredChange', () => { + it( 'should consider received blocks as ignored change', () => { const state = blocks( undefined, { type: 'RECEIVE_BLOCKS', blocks: [ { @@ -1510,7 +1530,7 @@ describe( 'state', () => { } ], } ); - expect( state.isPersistentChange ).toBe( false ); + expect( state.isIgnoredChange ).toBe( true ); } ); } ); } ); diff --git a/packages/e2e-tests/specs/change-detection.test.js b/packages/e2e-tests/specs/change-detection.test.js index 0e529a95ca512..7c9dab21d4d79 100644 --- a/packages/e2e-tests/specs/change-detection.test.js +++ b/packages/e2e-tests/specs/change-detection.test.js @@ -288,4 +288,21 @@ describe( 'Change detection', () => { await assertIsDirty( true ); } ); + + it( 'should not prompt when receiving reusable blocks', async () => { + // Regression Test: Verify that non-modifying behaviors does not incur + // dirtiness. Previously, this could occur as a result of either (a) + // selecting a block, (b) opening the inserter, or (c) editing a post + // which contained a reusable block. The root issue was changes in + // block editor state as a result of reusable blocks data having been + // received, reflected here in this test. + // + // TODO: This should be considered a temporary test, existing only so + // long as the experimental reusable blocks fetching data flow exists. + // + // See: https://github.com/WordPress/gutenberg/issues/14766 + await page.evaluate( () => window.wp.data.dispatch( 'core/editor' ).__experimentalReceiveReusableBlocks( [] ) ); + + await assertIsDirty( false ); + } ); } );