From b43f173995a8f33fb47a16d76550e348a66596d5 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Thu, 2 Apr 2020 16:08:56 -0700 Subject: [PATCH 1/9] Add state to track controlled inner blocks Allows us to tell if a certain block is controlling its own InnerBlocks. This is helpful for entities like template parts which persist their blocks outside of the root block-editor onChange method. Without this state, it would be hard to tell which blocks belong to which entity. With this state, we can easily traverse up the block heierarchy to find the closest parent block which is an inner block controller. Practically, we can use this to make sure that the dirty state between different entities is consistent. This commit also updates the tests which refer to the block state so that they all contain the new state slice in the before state. Otherwise, some of those tests were failing some some of the before state did not exist. --- .../developers/data/data-core-block-editor.md | 22 +++++++++++++++++++ packages/block-editor/src/store/actions.js | 17 ++++++++++++++ packages/block-editor/src/store/reducer.js | 13 +++++++++++ packages/block-editor/src/store/selectors.js | 12 ++++++++++ .../block-editor/src/store/test/reducer.js | 9 ++++++++ .../block-editor/src/store/test/selectors.js | 11 ++++++++++ 6 files changed, 84 insertions(+) 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 0632f870731fef..565f66258edfa2 100644 --- a/docs/designers-developers/developers/data/data-core-block-editor.md +++ b/docs/designers-developers/developers/data/data-core-block-editor.md @@ -6,6 +6,19 @@ Namespace: `core/block-editor`. +# **areInnerBlocksControlled** + +Checks if a given block has controlled inner blocks. + +_Parameters_ + +- _state_ `Object`: Global application state. +- _clientId_ `string`: The block to check. + +_Returns_ + +- `boolean`: True if the block has controlled inner blocks. + # **canInsertBlockType** Determines if the given block type is allowed to be inserted into the block list. @@ -1219,6 +1232,15 @@ _Parameters_ - _clientId_ `string`: Block client ID. +# **setHasControlledInnerBlocks** + +Returns an action object that sets whether the block has controlled innerblocks. + +_Parameters_ + +- _clientId_ `string`: The block's clientId. +- _hasControlledInnerBlocks_ `boolean`: True if the block's inner blocks are controlled. + # **setNavigationMode** Generators that triggers an action used to enable or disable the navigation mode. diff --git a/packages/block-editor/src/store/actions.js b/packages/block-editor/src/store/actions.js index a4991797255b02..bd1d275b673aa7 100644 --- a/packages/block-editor/src/store/actions.js +++ b/packages/block-editor/src/store/actions.js @@ -1010,3 +1010,20 @@ export function toggleBlockHighlight( clientId, isHighlighted ) { isHighlighted, }; } + +/** + * Returns an action object that sets whether the block has controlled innerblocks. + * + * @param {string} clientId The block's clientId. + * @param {boolean} hasControlledInnerBlocks True if the block's inner blocks are controlled. + */ +export function setHasControlledInnerBlocks( + clientId, + hasControlledInnerBlocks +) { + return { + type: 'SET_HAS_CONTROLLED_INNER_BLOCKS', + hasControlledInnerBlocks, + clientId, + }; +} diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index c4d51f58ee1911..5b25a5b1eadaec 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -971,6 +971,19 @@ export const blocks = flow( return state; }, + + controlledInnerBlocks( + state = {}, + { type, clientId, hasControlledInnerBlocks } + ) { + if ( type === 'SET_HAS_CONTROLLED_INNER_BLOCKS' ) { + return { + ...state, + [ clientId ]: hasControlledInnerBlocks, + }; + } + return state; + }, } ); /** diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index fc9dd38979294b..efa3ae86628fed 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -1633,3 +1633,15 @@ export function didAutomaticChange( state ) { export function isBlockHighlighted( state, clientId ) { return state.highlightedBlock === clientId; } + +/** + * Checks if a given block has controlled inner blocks. + * + * @param {Object} state Global application state. + * @param {string} clientId The block to check. + * + * @return {boolean} True if the block has controlled inner blocks. + */ +export function areInnerBlocksControlled( state, clientId ) { + return !! state.blocks.controlledInnerBlocks[ clientId ]; +} diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index 8610a1b8a3f31c..fadf7b3f34ba9a 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -239,6 +239,7 @@ describe( 'state', () => { chicken: {}, 'chicken-child': {}, }, + controlledInnerBlocks: {}, } ); const newChildBlock = createBlock( 'core/test-child-block', { @@ -291,6 +292,7 @@ describe( 'state', () => { chicken: {}, [ newChildBlockId ]: {}, }, + controlledInnerBlocks: {}, } ); expect( state.cache.chicken ).not.toBe( existingState.cache.chicken @@ -319,6 +321,7 @@ describe( 'state', () => { cache: { chicken: {}, }, + controlledInnerBlocks: {}, } ); const newChildBlock = createBlock( 'core/test-child-block', { @@ -371,6 +374,7 @@ describe( 'state', () => { chicken: {}, [ newChildBlockId ]: {}, }, + controlledInnerBlocks: {}, } ); expect( state.cache.chicken ).not.toBe( existingState.cache.chicken @@ -421,6 +425,7 @@ describe( 'state', () => { 'chicken-child': {}, 'chicken-child-2': {}, }, + controlledInnerBlocks: {}, } ); const newChildBlock1 = createBlock( 'core/test-child-block', { @@ -511,6 +516,7 @@ describe( 'state', () => { [ newChildBlockId2 ]: {}, [ newChildBlockId3 ]: {}, }, + controlledInnerBlocks: {}, } ); } ); @@ -554,6 +560,7 @@ describe( 'state', () => { 'chicken-child': {}, 'chicken-grand-child': {}, }, + controlledInnerBlocks: {}, } ); const newChildBlock = createBlock( 'core/test-block' ); @@ -600,6 +607,7 @@ describe( 'state', () => { chicken: {}, [ newChildBlockId ]: {}, }, + controlledInnerBlocks: {}, } ); // the cache key of the parent should be updated @@ -620,6 +628,7 @@ describe( 'state', () => { isPersistentChange: true, isIgnoredChange: false, cache: {}, + controlledInnerBlocks: {}, } ); } ); diff --git a/packages/block-editor/src/store/test/selectors.js b/packages/block-editor/src/store/test/selectors.js index 3cff15b7c922bc..e03b4ccd2a2596 100644 --- a/packages/block-editor/src/store/test/selectors.js +++ b/packages/block-editor/src/store/test/selectors.js @@ -220,6 +220,7 @@ describe( 'selectors', () => { cache: { 123: {}, }, + controlledInnerBlocks: {}, }, }; @@ -239,6 +240,7 @@ describe( 'selectors', () => { order: {}, parents: {}, cache: {}, + controlledInnerBlocks: {}, }, }; @@ -269,6 +271,7 @@ describe( 'selectors', () => { 123: {}, 456: {}, }, + controlledInnerBlocks: {}, }, }; @@ -311,6 +314,7 @@ describe( 'selectors', () => { 123: {}, 23: {}, }, + controlledInnerBlocks: {}, }, }; @@ -365,6 +369,7 @@ describe( 'selectors', () => { 'client-id-03': {}, 'client-id-04': {}, }, + controlledInnerBlocks: {}, }, }; @@ -418,6 +423,7 @@ describe( 'selectors', () => { 'client-id-04': {}, 'client-id-05': {}, }, + controlledInnerBlocks: {}, }, }; @@ -534,6 +540,7 @@ describe( 'selectors', () => { 'uuid-28': 'uuid-24', 'uuid-30': 'uuid-28', }, + controlledInnerBlocks: {}, }, }; expect( @@ -907,6 +914,7 @@ describe( 'selectors', () => { cache: { 23: {}, }, + controlledInnerBlocks: {}, }, selectionStart: { clientId: 23 }, selectionEnd: { clientId: 23 }, @@ -2335,6 +2343,7 @@ describe( 'selectors', () => { block3: {}, block4: {}, }, + controlledInnerBlocks: {}, }, settings: { __experimentalReusableBlocks: [ @@ -2418,6 +2427,7 @@ describe( 'selectors', () => { cache: { block1: {}, }, + controlledInnerBlocks: {}, }, preferences: { insertUsage: {}, @@ -2499,6 +2509,7 @@ describe( 'selectors', () => { cache: { block1: {}, }, + controlledInnerBlocks: {}, }, preferences: { insertUsage: {}, From 964a7965eaf0c64cddcc29af3e61bc4ecf333d74 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Mon, 4 May 2020 19:21:42 -0700 Subject: [PATCH 2/9] Add useBlockSync hook for BlockEditorProvider The useBlockSync hook syncs a component't block-edior changes with an onChange or onInput handler. The onChange handler is used for persistent changes, and onInput is used for other changes. All changes are scoped to the entity provided: e.g. w.r.t. a root clientId. No changes are included which are part of other entities. For example, a wp_template containing a wp_template_part will not be notified of changes within that template part, but only to changes in the template itself. In other words, it ignores changes which happen in nested inner block controllers. Previously, very similar logic was used for BlockEditorProvider, but it proved necessary to extract so that it could be integrated into InnerBlocks later on. At the same time, we have refactored BlockEditorProvider into a functional component with hooks to save space and to remove needless higher order components. --- .../developers/data/data-core-block-editor.md | 18 +- .../src/components/provider/index.js | 167 ++------------- .../src/components/provider/index.native.js | 167 ++------------- .../src/components/provider/use-block-sync.js | 199 ++++++++++++++++++ packages/block-editor/src/store/selectors.js | 31 ++- 5 files changed, 265 insertions(+), 317 deletions(-) create mode 100644 packages/block-editor/src/components/provider/use-block-sync.js 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 565f66258edfa2..82e01275c5387a 100644 --- a/docs/designers-developers/developers/data/data-core-block-editor.md +++ b/docs/designers-developers/developers/data/data-core-block-editor.md @@ -69,6 +69,16 @@ containing its `blockName`, `clientId`, and current `attributes` state. This is not the block's registration settings, which must be retrieved from the blocks module registration store. +getBlock recurses through its inner blocks until all its children blocks have +been retrieved. Note that getBlock will not return the child inner blocks of +an inner block controller. This is because an inner block controller syncs +itself with its own entity, and should therefore not be included with the +blocks of a different entity. For example, say you call `getBlocks( TP )` to +get the blocks of a template part. If another template part is a child of TP, +then the nested template part's child blocks will not be returned. This way, +the template block itself is considered part of the parent, but the children +are not. + _Parameters_ - _state_ `Object`: Editor state. @@ -251,10 +261,14 @@ _Returns_ # **getBlocks** Returns all block objects for the current post being edited as an array in -the order they appear in the post. +the order they appear in the post. Note that this will exclude child blocks +of nested inner block controllers. Note: It's important to memoize this selector to avoid return a new instance -on each call +on each call. We use the block cache state for each top-level block of the +given clientID. This way, the selector only refreshes on changes to blocks +associated with the given entity, and does not refresh when changes are made +to blocks which are part of different inner block controllers. _Parameters_ diff --git a/packages/block-editor/src/components/provider/index.js b/packages/block-editor/src/components/provider/index.js index 57413c693f1fa6..543a3ee487874f 100644 --- a/packages/block-editor/src/components/provider/index.js +++ b/packages/block-editor/src/components/provider/index.js @@ -1,170 +1,29 @@ -/** - * External dependencies - */ -import { last, noop } from 'lodash'; - /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; -import { withDispatch } from '@wordpress/data'; -import { compose } from '@wordpress/compose'; +import { useDispatch } from '@wordpress/data'; +import { useEffect } from '@wordpress/element'; /** * Internal dependencies */ import withRegistryProvider from './with-registry-provider'; +import useBlockSync from './use-block-sync'; /** @typedef {import('@wordpress/data').WPDataRegistry} WPDataRegistry */ -class BlockEditorProvider extends Component { - componentDidMount() { - this.props.updateSettings( this.props.settings ); - this.props.resetBlocks( this.props.value ); - this.attachChangeObserver( this.props.registry ); - this.isSyncingOutcomingValue = []; - } - - componentDidUpdate( prevProps ) { - const { - settings, - updateSettings, - value, - resetBlocks, - selectionStart, - selectionEnd, - resetSelection, - registry, - } = this.props; - - if ( settings !== prevProps.settings ) { - updateSettings( settings ); - } - - if ( registry !== prevProps.registry ) { - this.attachChangeObserver( registry ); - } - - if ( this.isSyncingOutcomingValue.includes( value ) ) { - // Skip block reset if the value matches expected outbound sync - // triggered by this component by a preceding change detection. - // Only skip if the value matches expectation, since a reset should - // still occur if the value is modified (not equal by reference), - // to allow that the consumer may apply modifications to reflect - // back on the editor. - if ( last( this.isSyncingOutcomingValue ) === value ) { - this.isSyncingOutcomingValue = []; - } - } else if ( value !== prevProps.value ) { - // Reset changing value in all other cases than the sync described - // above. Since this can be reached in an update following an out- - // bound sync, unset the outbound value to avoid considering it in - // subsequent renders. - this.isSyncingOutcomingValue = []; - this.isSyncingIncomingValue = value; - resetBlocks( value ); - - if ( selectionStart && selectionEnd ) { - resetSelection( selectionStart, selectionEnd ); - } - } - } - - componentWillUnmount() { - if ( this.unsubscribe ) { - this.unsubscribe(); - } - } - - /** - * Given a registry object, overrides the default dispatch behavior for the - * `core/block-editor` store to interpret a state change and decide whether - * we should call `onChange` or `onInput` depending on whether the change - * is persistent or not. - * - * This needs to be done synchronously after state changes (instead of using - * `componentDidUpdate`) in order to avoid batching these changes. - * - * @param {WPDataRegistry} registry Registry from which block editor - * dispatch is to be overridden. - */ - attachChangeObserver( registry ) { - if ( this.unsubscribe ) { - this.unsubscribe(); - } +function BlockEditorProvider( props ) { + const { children, settings } = props; - const { - getBlocks, - getSelectionStart, - getSelectionEnd, - isLastBlockChangePersistent, - __unstableIsLastBlockChangeIgnored, - } = registry.select( 'core/block-editor' ); + const { updateSettings } = useDispatch( 'core/block-editor' ); + useEffect( () => { + updateSettings( settings ); + }, [ settings ] ); - let blocks = getBlocks(); - let isPersistent = isLastBlockChangePersistent(); + // Syncs the entity provider with changes in the block-editor store. + useBlockSync( props ); - this.unsubscribe = registry.subscribe( () => { - const { onChange = noop, onInput = noop } = this.props; - - const newBlocks = getBlocks(); - const newIsPersistent = isLastBlockChangePersistent(); - - if ( - newBlocks !== blocks && - ( this.isSyncingIncomingValue || - __unstableIsLastBlockChangeIgnored() ) - ) { - this.isSyncingIncomingValue = null; - blocks = newBlocks; - isPersistent = newIsPersistent; - return; - } - - if ( - newBlocks !== blocks || - // This happens when a previous input is explicitely marked as persistent. - ( newIsPersistent && ! isPersistent ) - ) { - // When knowing the blocks value is changing, assign instance - // value to skip reset in subsequent `componentDidUpdate`. - if ( newBlocks !== blocks ) { - this.isSyncingOutcomingValue.push( newBlocks ); - } - - blocks = newBlocks; - isPersistent = newIsPersistent; - - const selectionStart = getSelectionStart(); - const selectionEnd = getSelectionEnd(); - - if ( isPersistent ) { - onChange( blocks, { selectionStart, selectionEnd } ); - } else { - onInput( blocks, { selectionStart, selectionEnd } ); - } - } - } ); - } - - render() { - const { children } = this.props; - - return children; - } + return children; } -export default compose( [ - withRegistryProvider, - withDispatch( ( dispatch ) => { - const { updateSettings, resetBlocks, resetSelection } = dispatch( - 'core/block-editor' - ); - - return { - updateSettings, - resetBlocks, - resetSelection, - }; - } ), -] )( BlockEditorProvider ); +export default withRegistryProvider( BlockEditorProvider ); diff --git a/packages/block-editor/src/components/provider/index.native.js b/packages/block-editor/src/components/provider/index.native.js index 57413c693f1fa6..543a3ee487874f 100644 --- a/packages/block-editor/src/components/provider/index.native.js +++ b/packages/block-editor/src/components/provider/index.native.js @@ -1,170 +1,29 @@ -/** - * External dependencies - */ -import { last, noop } from 'lodash'; - /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; -import { withDispatch } from '@wordpress/data'; -import { compose } from '@wordpress/compose'; +import { useDispatch } from '@wordpress/data'; +import { useEffect } from '@wordpress/element'; /** * Internal dependencies */ import withRegistryProvider from './with-registry-provider'; +import useBlockSync from './use-block-sync'; /** @typedef {import('@wordpress/data').WPDataRegistry} WPDataRegistry */ -class BlockEditorProvider extends Component { - componentDidMount() { - this.props.updateSettings( this.props.settings ); - this.props.resetBlocks( this.props.value ); - this.attachChangeObserver( this.props.registry ); - this.isSyncingOutcomingValue = []; - } - - componentDidUpdate( prevProps ) { - const { - settings, - updateSettings, - value, - resetBlocks, - selectionStart, - selectionEnd, - resetSelection, - registry, - } = this.props; - - if ( settings !== prevProps.settings ) { - updateSettings( settings ); - } - - if ( registry !== prevProps.registry ) { - this.attachChangeObserver( registry ); - } - - if ( this.isSyncingOutcomingValue.includes( value ) ) { - // Skip block reset if the value matches expected outbound sync - // triggered by this component by a preceding change detection. - // Only skip if the value matches expectation, since a reset should - // still occur if the value is modified (not equal by reference), - // to allow that the consumer may apply modifications to reflect - // back on the editor. - if ( last( this.isSyncingOutcomingValue ) === value ) { - this.isSyncingOutcomingValue = []; - } - } else if ( value !== prevProps.value ) { - // Reset changing value in all other cases than the sync described - // above. Since this can be reached in an update following an out- - // bound sync, unset the outbound value to avoid considering it in - // subsequent renders. - this.isSyncingOutcomingValue = []; - this.isSyncingIncomingValue = value; - resetBlocks( value ); - - if ( selectionStart && selectionEnd ) { - resetSelection( selectionStart, selectionEnd ); - } - } - } - - componentWillUnmount() { - if ( this.unsubscribe ) { - this.unsubscribe(); - } - } - - /** - * Given a registry object, overrides the default dispatch behavior for the - * `core/block-editor` store to interpret a state change and decide whether - * we should call `onChange` or `onInput` depending on whether the change - * is persistent or not. - * - * This needs to be done synchronously after state changes (instead of using - * `componentDidUpdate`) in order to avoid batching these changes. - * - * @param {WPDataRegistry} registry Registry from which block editor - * dispatch is to be overridden. - */ - attachChangeObserver( registry ) { - if ( this.unsubscribe ) { - this.unsubscribe(); - } +function BlockEditorProvider( props ) { + const { children, settings } = props; - const { - getBlocks, - getSelectionStart, - getSelectionEnd, - isLastBlockChangePersistent, - __unstableIsLastBlockChangeIgnored, - } = registry.select( 'core/block-editor' ); + const { updateSettings } = useDispatch( 'core/block-editor' ); + useEffect( () => { + updateSettings( settings ); + }, [ settings ] ); - let blocks = getBlocks(); - let isPersistent = isLastBlockChangePersistent(); + // Syncs the entity provider with changes in the block-editor store. + useBlockSync( props ); - this.unsubscribe = registry.subscribe( () => { - const { onChange = noop, onInput = noop } = this.props; - - const newBlocks = getBlocks(); - const newIsPersistent = isLastBlockChangePersistent(); - - if ( - newBlocks !== blocks && - ( this.isSyncingIncomingValue || - __unstableIsLastBlockChangeIgnored() ) - ) { - this.isSyncingIncomingValue = null; - blocks = newBlocks; - isPersistent = newIsPersistent; - return; - } - - if ( - newBlocks !== blocks || - // This happens when a previous input is explicitely marked as persistent. - ( newIsPersistent && ! isPersistent ) - ) { - // When knowing the blocks value is changing, assign instance - // value to skip reset in subsequent `componentDidUpdate`. - if ( newBlocks !== blocks ) { - this.isSyncingOutcomingValue.push( newBlocks ); - } - - blocks = newBlocks; - isPersistent = newIsPersistent; - - const selectionStart = getSelectionStart(); - const selectionEnd = getSelectionEnd(); - - if ( isPersistent ) { - onChange( blocks, { selectionStart, selectionEnd } ); - } else { - onInput( blocks, { selectionStart, selectionEnd } ); - } - } - } ); - } - - render() { - const { children } = this.props; - - return children; - } + return children; } -export default compose( [ - withRegistryProvider, - withDispatch( ( dispatch ) => { - const { updateSettings, resetBlocks, resetSelection } = dispatch( - 'core/block-editor' - ); - - return { - updateSettings, - resetBlocks, - resetSelection, - }; - } ), -] )( BlockEditorProvider ); +export default withRegistryProvider( BlockEditorProvider ); diff --git a/packages/block-editor/src/components/provider/use-block-sync.js b/packages/block-editor/src/components/provider/use-block-sync.js new file mode 100644 index 00000000000000..8ba4ebf14405fc --- /dev/null +++ b/packages/block-editor/src/components/provider/use-block-sync.js @@ -0,0 +1,199 @@ +/** + * External dependencies + */ +import { last, noop } from 'lodash'; + +/** + * WordPress dependencies + */ +import { useEffect, useRef } from '@wordpress/element'; +import { useRegistry } from '@wordpress/data'; + +/** + * A function to call when the block value has been updated in the block-editor + * store. + * + * @callback onBlockUpdate + * @param {Object[]} blocks The updated blocks. + * @param {Object} options The updated block options, such as selectionStart + * and selectionEnd. + */ + +/** + * useBlockSync is a side effect which handles bidirectional sync between the + * block-editor store and a controlling data source which provides blocks. This + * is most commonly used by the BlockEditorProvider to synchronize the contents + * of the block-editor store with the root entity, like a post. + * + * Another example would be the template part block, which provides blocks from + * a separate entity data source than a root entity. This hook syncs edits to + * the template part in the block editor back to the entity and vice-versa. + * + * Here are some of its basic functions: + * - Initalizes the block-editor store for the given clientID to the blocks + * given via props. + * - Adds incoming changes (like undo) to the block-editor store. + * - Adds outgoing changes (like editing content) to the controlling entity, + * determining if a change should be considered persistent or not. + * - Handles edge cases and race conditions which occur in those operations. + * - Ignores changes which happen to other entities (like nested inner block + * controllers. + * - Passes selection state from the block-editor store to the controlling entity. + * + * @param {Object} props Props for the block sync hook + * @param {string} props.clientId The client ID of the inner block controller. + * If none is passed, then it is assumed to be a + * root controller rather than an inner block + * controller. + * @param {Object[]} props.value The control value for the blocks. This value + * is used to initalize the block-editor store + * and for resetting the blocks to incoming + * changes like undo. + * @param {Object} props.selectionStart The selection start vlaue from the + * controlling component. + * @param {Object} props.selectionEnd The selection end vlaue from the + * controlling component. + * @param {onBlockUpdate} props.onChange Function to call when a persistent + * change has been made in the block-editor blocks + * for the given clientId. For example, after + * this function is called, an entity is marked + * dirty because it has changes to save. + * @param {onBlockUpdate} props.onInput Function to call when a non-persistent + * change has been made in the block-editor blocks + * for the given clientId. When this is called, + * controlling sources do not become dirty. + */ +export default function useBlockSync( { + clientId = null, + value: controlledBlocks, + selectionStart: controlledSelectionStart, + selectionEnd: controlledSelectionEnd, + onChange = noop, + onInput = noop, +} ) { + const registry = useRegistry(); + + const { + resetBlocks, + resetSelection, + replaceInnerBlocks, + setHasControlledInnerBlocks, + __unstableMarkNextChangeAsNotPersistent, + } = registry.dispatch( 'core/block-editor' ); + const { getBlocks } = registry.select( 'core/block-editor' ); + + const pendingChanges = useRef( { incoming: null, outgoing: [] } ); + + const setControlledBlocks = () => { + if ( ! controlledBlocks ) { + return; + } + + // We don't need to persist this change because we only replace + // controlled inner blocks when the change was caused by an entity, + // and so it would already be persisted. + __unstableMarkNextChangeAsNotPersistent(); + if ( clientId ) { + setHasControlledInnerBlocks( clientId, true ); + __unstableMarkNextChangeAsNotPersistent(); + replaceInnerBlocks( clientId, controlledBlocks, false ); + } else { + resetBlocks( controlledBlocks ); + } + }; + + // Add a subscription to the block-editor registry to detect when changes + // have been made. This lets us inform the data source of changes. This + // is an effect so that the subscriber can run synchronously without + // waiting for React renders for changes. + useEffect( () => { + const { + getSelectionStart, + getSelectionEnd, + isLastBlockChangePersistent, + __unstableIsLastBlockChangeIgnored, + } = registry.select( 'core/block-editor' ); + + let blocks; + let isPersistent = isLastBlockChangePersistent(); + let previousAreBlocksDifferent = false; + + const unsubscribe = registry.subscribe( () => { + const newIsPersistent = isLastBlockChangePersistent(); + + const newBlocks = getBlocks( clientId ); + const areBlocksDifferent = newBlocks !== blocks; + blocks = newBlocks; + + if ( + areBlocksDifferent && + ( pendingChanges.current.incoming || + __unstableIsLastBlockChangeIgnored() ) + ) { + pendingChanges.current.incoming = null; + isPersistent = newIsPersistent; + return; + } + + // Since we often dispatch an action to mark the previous action as + // persistent, we need to make sure that the blocks changed on the + // previous action before committing the change. + const didPersistenceChange = + previousAreBlocksDifferent && + ! areBlocksDifferent && + newIsPersistent && + ! isPersistent; + + if ( areBlocksDifferent || didPersistenceChange ) { + isPersistent = newIsPersistent; + // We know that onChange/onInput will update controlledBlocks. + // We need to be aware that it was caused by an outgoing change + // so that we do not treat it as an incoming change later on, + // which would cause a block reset. + pendingChanges.current.outgoing.push( blocks ); + + // Inform the controlling entity that changes have been made to + // the block-editor store they should be aware about. + const updateParent = isPersistent ? onChange : onInput; + updateParent( blocks, { + selectionStart: getSelectionStart(), + selectionEnd: getSelectionEnd(), + } ); + } + previousAreBlocksDifferent = areBlocksDifferent; + } ); + return () => unsubscribe(); + }, [ registry, onChange, onInput, clientId ] ); + + // Determine if blocks need to be reset when they change. + useEffect( () => { + if ( pendingChanges.current.outgoing.includes( controlledBlocks ) ) { + // Skip block reset if the value matches expected outbound sync + // triggered by this component by a preceding change detection. + // Only skip if the value matches expectation, since a reset should + // still occur if the value is modified (not equal by reference), + // to allow that the consumer may apply modifications to reflect + // back on the editor. + if ( + last( pendingChanges.current.outgoing ) === controlledBlocks + ) { + pendingChanges.current.outgoing = []; + } + } else if ( getBlocks( clientId ) !== controlledBlocks ) { + // Reset changing value in all other cases than the sync described + // above. Since this can be reached in an update following an out- + // bound sync, unset the outbound value to avoid considering it in + // subsequent renders. + pendingChanges.current.outgoing = []; + pendingChanges.current.incoming = controlledBlocks; + setControlledBlocks(); + + if ( controlledSelectionStart && controlledSelectionEnd ) { + resetSelection( + controlledSelectionStart, + controlledSelectionEnd + ); + } + } + }, [ controlledBlocks, clientId ] ); +} diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index efa3ae86628fed..096b3dd9b2b963 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -137,6 +137,16 @@ export function getBlockAttributes( state, clientId ) { * is not the block's registration settings, which must be retrieved from the * blocks module registration store. * + * getBlock recurses through its inner blocks until all its children blocks have + * been retrieved. Note that getBlock will not return the child inner blocks of + * an inner block controller. This is because an inner block controller syncs + * itself with its own entity, and should therefore not be included with the + * blocks of a different entity. For example, say you call `getBlocks( TP )` to + * get the blocks of a template part. If another template part is a child of TP, + * then the nested template part's child blocks will not be returned. This way, + * the template block itself is considered part of the parent, but the children + * are not. + * * @param {Object} state Editor state. * @param {string} clientId Block client ID. * @@ -152,7 +162,9 @@ export const getBlock = createSelector( return { ...block, attributes: getBlockAttributes( state, clientId ), - innerBlocks: getBlocks( state, clientId ), + innerBlocks: areInnerBlocksControlled( state, clientId ) + ? EMPTY_ARRAY + : getBlocks( state, clientId ), }; }, ( state, clientId ) => [ @@ -185,10 +197,14 @@ export const __unstableGetBlockWithoutInnerBlocks = createSelector( /** * Returns all block objects for the current post being edited as an array in - * the order they appear in the post. + * the order they appear in the post. Note that this will exclude child blocks + * of nested inner block controllers. * * Note: It's important to memoize this selector to avoid return a new instance - * on each call + * on each call. We use the block cache state for each top-level block of the + * given clientID. This way, the selector only refreshes on changes to blocks + * associated with the given entity, and does not refresh when changes are made + * to blocks which are part of different inner block controllers. * * @param {Object} state Editor state. * @param {?string} rootClientId Optional root client ID of block list. @@ -201,10 +217,11 @@ export const getBlocks = createSelector( getBlock( state, clientId ) ); }, - ( state ) => [ - state.blocks.byClientId, - state.blocks.order, - state.blocks.attributes, + ( state, rootClientId ) => [ + ...map( + state.blocks.order[ rootClientId || '' ], + ( id ) => state.blocks.cache[ id ] + ), ] ); From fb4c3bec1d1ddfd904131f9356ecfb4eab4430dd Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Thu, 2 Apr 2020 16:14:14 -0700 Subject: [PATCH 3/9] Refactor InnerBlocks into FC with blockSync hook This changes InnerBlocks into a functional component. Practically, this should work exactly the same as before. This allows us to more easily use the useBlockSync hook in order to set up inner block controllers (i.e. with template parts). We now specify a ControlledInnerBlocks component which handles block sync instead of having that logic within InnerBlocks itself. It also allows us to reuse the same logic as the root BlockEditorProvider, so that we don't miss any of the logic and race condition fixes that already exist there. I did try adding similar logic to the old InnerBlocks component with what existed, but it proved very prone to breakage and was hard to make compatible with the root entity controller. It is much more straightforward to keep the logic in the same place! We also refactored the react native file for inner blocks, and added forwardedRef and block context. --- .../inner-blocks/get-block-context.js | 39 ++ .../src/components/inner-blocks/index.js | 379 ++++++------------ .../components/inner-blocks/index.native.js | 302 ++++++-------- .../use-inner-block-template-sync.js | 73 ++++ .../use-nested-settings-update.js | 82 ++++ .../src/template-part/edit/inner-blocks.js | 2 +- 6 files changed, 430 insertions(+), 447 deletions(-) create mode 100644 packages/block-editor/src/components/inner-blocks/get-block-context.js create mode 100644 packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js create mode 100644 packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js diff --git a/packages/block-editor/src/components/inner-blocks/get-block-context.js b/packages/block-editor/src/components/inner-blocks/get-block-context.js new file mode 100644 index 00000000000000..295ef380d3808d --- /dev/null +++ b/packages/block-editor/src/components/inner-blocks/get-block-context.js @@ -0,0 +1,39 @@ +/** + * External dependencies + */ +import { mapValues } from 'lodash'; + +/** + * Block context cache, implemented as a WeakMap mapping block types to a + * WeakMap mapping attributes object to context value. + * + * @type {WeakMap>} + */ +const BLOCK_CONTEXT_CACHE = new WeakMap(); + +/** + * Returns a cached context object value for a given set of attributes for the + * block type. + * + * @param {Record} attributes Block attributes object. + * @param {WPBlockType} blockType Block type settings. + * + * @return {Record} Context value. + */ +export default function getBlockContext( attributes, blockType ) { + if ( ! BLOCK_CONTEXT_CACHE.has( blockType ) ) { + BLOCK_CONTEXT_CACHE.set( blockType, new WeakMap() ); + } + + const blockTypeCache = BLOCK_CONTEXT_CACHE.get( blockType ); + if ( ! blockTypeCache.has( attributes ) ) { + const context = mapValues( + blockType.providesContext, + ( attributeName ) => attributes[ attributeName ] + ); + + blockTypeCache.set( attributes, context ); + } + + return blockTypeCache.get( attributes ); +} diff --git a/packages/block-editor/src/components/inner-blocks/index.js b/packages/block-editor/src/components/inner-blocks/index.js index 64b958d7f8bf10..82449b48cc36cd 100644 --- a/packages/block-editor/src/components/inner-blocks/index.js +++ b/packages/block-editor/src/components/inner-blocks/index.js @@ -1,304 +1,159 @@ /** * External dependencies */ -import { mapValues, pick, isEqual } from 'lodash'; import classnames from 'classnames'; /** * WordPress dependencies */ -import { withViewportMatch } from '@wordpress/viewport'; -import { Component, forwardRef, useRef } from '@wordpress/element'; -import { withSelect, withDispatch } from '@wordpress/data'; -import { - getBlockType, - synchronizeBlocksWithTemplate, - withBlockContentContext, -} from '@wordpress/blocks'; -import isShallowEqual from '@wordpress/is-shallow-equal'; -import { compose } from '@wordpress/compose'; +import { useViewportMatch } from '@wordpress/compose'; +import { forwardRef, useRef } from '@wordpress/element'; +import { useSelect } from '@wordpress/data'; +import { getBlockType, withBlockContentContext } from '@wordpress/blocks'; /** * Internal dependencies */ import ButtonBlockAppender from './button-block-appender'; import DefaultBlockAppender from './default-block-appender'; +import useNestedSettingsUpdate from './use-nested-settings-update'; +import useInnerBlockTemplateSync from './use-inner-block-template-sync'; +import getBlockContext from './get-block-context'; /** * Internal dependencies */ import BlockList from '../block-list'; import { BlockContextProvider } from '../block-context'; -import { withBlockEditContext } from '../block-edit/context'; +import { useBlockEditContext } from '../block-edit/context'; +import useBlockSync from '../provider/use-block-sync'; /** - * Block context cache, implemented as a WeakMap mapping block types to a - * WeakMap mapping attributes object to context value. + * InnerBlocks is a component which allows a single block to have multiple blocks + * as children. The UncontrolledInnerBlocks component is used whenever the inner + * blocks are not controlled by another entity. In other words, it is normally + * used for inner blocks in the post editor * - * @type {WeakMap>} + * @param {Object} props The component props. */ -const BLOCK_CONTEXT_CACHE = new WeakMap(); - -/** - * Returns a cached context object value for a given set of attributes for the - * block type. - * - * @param {Record} attributes Block attributes object. - * @param {WPBlockType} blockType Block type settings. - * - * @return {Record} Context value. - */ -function getBlockContext( attributes, blockType ) { - if ( ! BLOCK_CONTEXT_CACHE.has( blockType ) ) { - BLOCK_CONTEXT_CACHE.set( blockType, new WeakMap() ); - } - - const blockTypeCache = BLOCK_CONTEXT_CACHE.get( blockType ); - if ( ! blockTypeCache.has( attributes ) ) { - const context = mapValues( - blockType.providesContext, - ( attributeName ) => attributes[ attributeName ] - ); - - blockTypeCache.set( attributes, context ); - } - - return blockTypeCache.get( attributes ); -} - -class InnerBlocks extends Component { - constructor() { - super( ...arguments ); - this.state = { - templateInProcess: !! this.props.template, - }; - this.updateNestedSettings(); - } - - componentDidMount() { - const { - block, - templateLock, - __experimentalBlocks, - replaceInnerBlocks, - __unstableMarkNextChangeAsNotPersistent, - } = this.props; - const { innerBlocks } = block; - // Only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists directly on the block. - if ( innerBlocks.length === 0 || templateLock === 'all' ) { - this.synchronizeBlocksWithTemplate(); - } - - if ( this.state.templateInProcess ) { - this.setState( { - templateInProcess: false, - } ); - } - - // Set controlled blocks value from parent, if any. - if ( __experimentalBlocks ) { - __unstableMarkNextChangeAsNotPersistent(); - replaceInnerBlocks( __experimentalBlocks, false ); - } - } - - componentDidUpdate( prevProps ) { - const { - block, - templateLock, - template, - isLastBlockChangePersistent, - onInput, - onChange, - } = this.props; - const { innerBlocks } = block; - - this.updateNestedSettings(); - // Only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists directly on the block. - if ( innerBlocks.length === 0 || templateLock === 'all' ) { - const hasTemplateChanged = ! isEqual( - template, - prevProps.template - ); - if ( hasTemplateChanged ) { - this.synchronizeBlocksWithTemplate(); - } - } - - // Sync with controlled blocks value from parent, if possible. - if ( prevProps.block.innerBlocks !== innerBlocks ) { - const resetFunc = isLastBlockChangePersistent ? onChange : onInput; - if ( resetFunc ) { - resetFunc( innerBlocks ); - } - } - } - - /** - * Called on mount or when a mismatch exists between the templates and - * inner blocks, synchronizes inner blocks with the template, replacing - * current blocks. - */ - synchronizeBlocksWithTemplate() { - const { template, block, replaceInnerBlocks } = this.props; - const { innerBlocks } = block; - - // Synchronize with templates. If the next set differs, replace. - const nextBlocks = synchronizeBlocksWithTemplate( - innerBlocks, - template - ); - if ( ! isEqual( nextBlocks, innerBlocks ) ) { - replaceInnerBlocks( nextBlocks ); - } - } - - updateNestedSettings() { - const { - blockListSettings, - allowedBlocks, - updateNestedSettings, - templateLock, - parentLock, - __experimentalCaptureToolbars, - __experimentalMoverDirection, - } = this.props; - - const newSettings = { - allowedBlocks, - templateLock: - templateLock === undefined ? parentLock : templateLock, - __experimentalCaptureToolbars: - __experimentalCaptureToolbars || false, - __experimentalMoverDirection, - }; - - if ( ! isShallowEqual( blockListSettings, newSettings ) ) { - updateNestedSettings( newSettings ); - } - } - - render() { - const { - enableClickThrough, - clientId, - hasOverlay, - __experimentalCaptureToolbars: captureToolbars, - forwardedRef, - block, - ...props - } = this.props; - const { templateInProcess } = this.state; - - if ( templateInProcess ) { - return null; - } - - const classes = classnames( { - 'has-overlay': enableClickThrough && hasOverlay, - 'is-capturing-toolbar': captureToolbars, - } ); - - let blockList = ( - - ); - - // Wrap context provider if (and only if) block has context to provide. - const blockType = getBlockType( block.name ); - if ( blockType && blockType.providesContext ) { - const context = getBlockContext( block.attributes, blockType ); - - blockList = ( - - { blockList } - - ); - } - - if ( props.__experimentalTagName ) { - return blockList; - } - - return ( -
- { blockList } -
- ); - } -} - -const ComposedInnerBlocks = compose( [ - withViewportMatch( { isSmallScreen: '< medium' } ), - withBlockEditContext( ( context ) => pick( context, [ 'clientId' ] ) ), - withSelect( ( select, ownProps ) => { +function UncontrolledInnerBlocks( props ) { + const { + clientId, + allowedBlocks, + template, + templateLock, + forwardedRef, + templateInsertUpdatesSelection, + __experimentalCaptureToolbars: captureToolbars, + __experimentalMoverDirection, + } = props; + + const isSmallScreen = useViewportMatch( 'medium', '<' ); + + const { hasOverlay, block, enableClickThrough } = useSelect( ( select ) => { const { + getBlock, isBlockSelected, hasSelectedInnerBlock, - getBlock, - getBlockListSettings, - getBlockRootClientId, - getTemplateLock, isNavigationMode, - isLastBlockChangePersistent, } = select( 'core/block-editor' ); - const { clientId, isSmallScreen } = ownProps; - const block = getBlock( clientId ); - const rootClientId = getBlockRootClientId( clientId ); - + const theBlock = getBlock( clientId ); return { - block, - blockListSettings: getBlockListSettings( clientId ), + block: theBlock, hasOverlay: - block.name !== 'core/template' && + theBlock.name !== 'core/template' && ! isBlockSelected( clientId ) && ! hasSelectedInnerBlock( clientId, true ), - parentLock: getTemplateLock( rootClientId ), enableClickThrough: isNavigationMode() || isSmallScreen, - isLastBlockChangePersistent: isLastBlockChangePersistent(), }; - } ), - withDispatch( ( dispatch, ownProps ) => { - const { - replaceInnerBlocks, - __unstableMarkNextChangeAsNotPersistent, - updateBlockListSettings, - } = dispatch( 'core/block-editor' ); - const { - block, - clientId, - templateInsertUpdatesSelection = true, - } = ownProps; + } ); + + useNestedSettingsUpdate( + clientId, + allowedBlocks, + templateLock, + captureToolbars, + __experimentalMoverDirection + ); - return { - replaceInnerBlocks( blocks, forceUpdateSelection ) { - replaceInnerBlocks( - clientId, - blocks, - forceUpdateSelection !== undefined - ? forceUpdateSelection - : block.innerBlocks.length === 0 && - templateInsertUpdatesSelection && - blocks.length !== 0 - ); - }, - __unstableMarkNextChangeAsNotPersistent, - updateNestedSettings( settings ) { - dispatch( updateBlockListSettings( clientId, settings ) ); - }, - }; - } ), -] )( InnerBlocks ); + useInnerBlockTemplateSync( + clientId, + template, + templateLock, + templateInsertUpdatesSelection + ); + + const classes = classnames( { + 'has-overlay': enableClickThrough && hasOverlay, + 'is-capturing-toolbar': captureToolbars, + } ); + + let blockList = ( + + ); + + // Wrap context provider if (and only if) block has context to provide. + const blockType = getBlockType( block.name ); + if ( blockType && blockType.providesContext ) { + const context = getBlockContext( block.attributes, blockType ); + + blockList = ( + + { blockList } + + ); + } + + if ( props.__experimentalTagName ) { + return blockList; + } -const ForwardedInnerBlocks = forwardRef( ( props, ref ) => { - const fallbackRef = useRef(); return ( - +
+ { blockList } +
); +} + +/** + * The controlled inner blocks component wraps the uncontrolled inner blocks + * component with the blockSync hook. This keeps the innerBlocks of the block in + * the block-editor store in sync with the blocks of the controlling entity. An + * example of an inner block controller is a template part block, which provides + * its own blocks from the template part entity data source. + * + * @param {Object} props The component props. + */ +function ControlledInnerBlocks( props ) { + useBlockSync( props ); + return ; +} + +/** + * Wrapped InnerBlocks component which detects whether to use the controlled or + * uncontrolled variations of the InnerBlocks component. This is the component + * which should be used throughout the application. + */ +const ForwardedInnerBlocks = forwardRef( ( props, ref ) => { + const { clientId } = useBlockEditContext(); + const fallbackRef = useRef(); + + const allProps = { + clientId, + forwardedRef: ref || fallbackRef, + ...props, + }; + + // Detects if the InnerBlocks should be controlled by an incoming value. + if ( props.value && props.onChange ) { + return ; + } + return ; } ); // Expose default appender placeholders as components. diff --git a/packages/block-editor/src/components/inner-blocks/index.native.js b/packages/block-editor/src/components/inner-blocks/index.native.js index 3f078de90dff97..db06f0369abaa3 100644 --- a/packages/block-editor/src/components/inner-blocks/index.native.js +++ b/packages/block-editor/src/components/inner-blocks/index.native.js @@ -1,204 +1,138 @@ -/** - * External dependencies - */ -import { pick, isEqual } from 'lodash'; - /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; -import { withSelect, withDispatch } from '@wordpress/data'; -import { - synchronizeBlocksWithTemplate, - withBlockContentContext, -} from '@wordpress/blocks'; -import isShallowEqual from '@wordpress/is-shallow-equal'; -import { compose } from '@wordpress/compose'; +import { useSelect } from '@wordpress/data'; +import { getBlockType, withBlockContentContext } from '@wordpress/blocks'; /** * Internal dependencies */ import ButtonBlockAppender from './button-block-appender'; import DefaultBlockAppender from './default-block-appender'; -import BlockList from '../block-list'; -import { withBlockEditContext } from '../block-edit/context'; - -class InnerBlocks extends Component { - constructor() { - super( ...arguments ); - this.state = { - templateInProcess: !! this.props.template, - }; - this.updateNestedSettings(); - } - - getTemplateLock() { - const { templateLock, parentLock } = this.props; - return templateLock === undefined ? parentLock : templateLock; - } +import useNestedSettingsUpdate from './use-nested-settings-update'; +import useInnerBlockTemplateSync from './use-inner-block-template-sync'; +import getBlockContext from './get-block-context'; - componentDidMount() { - const { innerBlocks } = this.props.block; - // only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists - if ( innerBlocks.length === 0 || this.getTemplateLock() === 'all' ) { - this.synchronizeBlocksWithTemplate(); - } - - if ( this.state.templateInProcess ) { - this.setState( { - templateInProcess: false, - } ); - } - } - - componentDidUpdate( prevProps ) { - const { template, block } = this.props; - const { innerBlocks } = block; - - this.updateNestedSettings(); - // only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists - if ( innerBlocks.length === 0 || this.getTemplateLock() === 'all' ) { - const hasTemplateChanged = ! isEqual( - template, - prevProps.template - ); - if ( hasTemplateChanged ) { - this.synchronizeBlocksWithTemplate(); - } - } - } +/** + * Internal dependencies + */ +import BlockList from '../block-list'; +import { useBlockEditContext } from '../block-edit/context'; +import useBlockSync from '../provider/use-block-sync'; +import { BlockContextProvider } from '../block-context'; - /** - * Called on mount or when a mismatch exists between the templates and - * inner blocks, synchronizes inner blocks with the template, replacing - * current blocks. - */ - synchronizeBlocksWithTemplate() { - const { template, block, replaceInnerBlocks } = this.props; - const { innerBlocks } = block; - - // Synchronize with templates. If the next set differs, replace. - const nextBlocks = synchronizeBlocksWithTemplate( - innerBlocks, - template +/** + * InnerBlocks is a component which allows a single block to have multiple blocks + * as children. The UncontrolledInnerBlocks component is used whenever the inner + * blocks are not controlled by another entity. In other words, it is normally + * used for inner blocks in the post editor + * + * @param {Object} props The component props. + */ +function UncontrolledInnerBlocks( props ) { + const { + clientId, + allowedBlocks, + template, + templateLock, + templateInsertUpdatesSelection, + __experimentalMoverDirection, + renderAppender, + renderFooterAppender, + parentWidth, + horizontal, + contentResizeMode, + contentStyle, + onAddBlock, + onDeleteBlock, + marginVertical, + marginHorizontal, + horizontalAlignment, + } = props; + + const block = useSelect( ( select ) => + select( 'core/block-editor' ).getBlock( clientId ) + ); + + useNestedSettingsUpdate( clientId, allowedBlocks, templateLock ); + + useInnerBlockTemplateSync( + clientId, + template, + templateLock, + templateInsertUpdatesSelection + ); + + let blockList = ( + + ); + + // Wrap context provider if (and only if) block has context to provide. + const blockType = getBlockType( block.name ); + if ( blockType && blockType.providesContext ) { + const context = getBlockContext( block.attributes, blockType ); + + blockList = ( + + { blockList } + ); - if ( ! isEqual( nextBlocks, innerBlocks ) ) { - replaceInnerBlocks( nextBlocks ); - } } - updateNestedSettings() { - const { - blockListSettings, - allowedBlocks, - updateNestedSettings, - } = this.props; - - const newSettings = { - allowedBlocks, - templateLock: this.getTemplateLock(), - }; - - if ( ! isShallowEqual( blockListSettings, newSettings ) ) { - updateNestedSettings( newSettings ); - } - } + return blockList; +} - render() { - const { - clientId, - renderAppender, - renderFooterAppender, - __experimentalMoverDirection, - parentWidth, - horizontal, - contentResizeMode, - contentStyle, - onAddBlock, - onDeleteBlock, - marginVertical, - marginHorizontal, - horizontalAlignment, - } = this.props; - const { templateInProcess } = this.state; - - return ( - <> - { ! templateInProcess && ( - - ) } - - ); - } +/** + * The controlled inner blocks component wraps the uncontrolled inner blocks + * component with the blockSync hook. This keeps the innerBlocks of the block in + * the block-editor store in sync with the blocks of the controlling entity. An + * example of an inner block controller is a template part block, which provides + * its own blocks from the template part entity data source. + * + * @param {Object} props The component props. + */ +function ControlledInnerBlocks( props ) { + useBlockSync( props ); + return ; } -InnerBlocks = compose( [ - withBlockEditContext( ( context ) => pick( context, [ 'clientId' ] ) ), - withSelect( ( select, ownProps ) => { - const { - isBlockSelected, - hasSelectedInnerBlock, - getBlock, - getBlockListSettings, - getBlockRootClientId, - getTemplateLock, - } = select( 'core/block-editor' ); - const { clientId } = ownProps; - const block = getBlock( clientId ); - const rootClientId = getBlockRootClientId( clientId ); - - return { - block, - blockListSettings: getBlockListSettings( clientId ), - hasOverlay: - block.name !== 'core/template' && - ! isBlockSelected( clientId ) && - ! hasSelectedInnerBlock( clientId, true ), - parentLock: getTemplateLock( rootClientId ), - }; - } ), - withDispatch( ( dispatch, ownProps ) => { - const { replaceInnerBlocks, updateBlockListSettings } = dispatch( - 'core/block-editor' - ); - const { - block, - clientId, - templateInsertUpdatesSelection = true, - } = ownProps; - - return { - replaceInnerBlocks( blocks ) { - replaceInnerBlocks( - clientId, - blocks, - block.innerBlocks.length === 0 && - templateInsertUpdatesSelection - ); - }, - updateNestedSettings( settings ) { - dispatch( updateBlockListSettings( clientId, settings ) ); - }, - }; - } ), -] )( InnerBlocks ); +/** + * Wrapped InnerBlocks component which detects whether to use the controlled or + * uncontrolled variations of the InnerBlocks component. This is the component + * which should be used throughout the application. + * + * @param {Object} props The component props. + */ +const InnerBlocks = ( props ) => { + const { clientId } = useBlockEditContext(); + + const allProps = { + clientId, + ...props, + }; + + // Detects if the InnerBlocks should be controlled by an incoming value. + return props.value && props.onChange ? ( + + ) : ( + + ); +}; // Expose default appender placeholders as components. InnerBlocks.DefaultBlockAppender = DefaultBlockAppender; diff --git a/packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js b/packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js new file mode 100644 index 00000000000000..f663aa9f4554f7 --- /dev/null +++ b/packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js @@ -0,0 +1,73 @@ +/** + * External dependencies + */ +import { isEqual } from 'lodash'; + +/** + * WordPress dependencies + */ +import { useRef, useEffect } from '@wordpress/element'; +import { useSelect, useDispatch } from '@wordpress/data'; +import { synchronizeBlocksWithTemplate } from '@wordpress/blocks'; + +/** + * This hook makes sure that a block's inner blocks stay in sync with the given + * block "template". The template is a block hierarchy to which inner blocks must + * conform. If the blocks get "out of sync" with the template and the template + * is meant to be locked (e.g. templateLock = "all"), then we replace the inner + * blocks with the correct value after synchronizing it with the template. + * + * @param {string} clientId The block client ID. + * @param {Object} template The template to match. + * @param {string} templateLock The template lock state for the inner blocks. For + * example, if the template lock is set to "all", + * then the inner blocks will stay in sync with the + * template. If not defined or set to false, then + * the inner blocks will not be synchronized with + * the given template. + * @param {boolean} templateInsertUpdatesSelection Whether or not to update the + * block-editor selection state when inner blocks + * are replaced after template synchronization. + */ +export default function useInnerBlockTemplateSync( + clientId, + template, + templateLock, + templateInsertUpdatesSelection +) { + const { replaceInnerBlocks } = useDispatch( 'core/block-editor' ); + + const innerBlocks = useSelect( + ( select ) => select( 'core/block-editor' ).getBlocks( clientId ), + [ clientId ] + ); + + // Maintain a reference to the previous value so we can do a deep equality check. + const existingTemplate = useRef( null ); + useEffect( () => { + // Only synchronize innerBlocks with template if innerBlocks are empty or + // a locking all exists directly on the block. + if ( innerBlocks.length === 0 || templateLock === 'all' ) { + const hasTemplateChanged = ! isEqual( + template, + existingTemplate.current + ); + if ( hasTemplateChanged ) { + existingTemplate.current = template; + const nextBlocks = synchronizeBlocksWithTemplate( + innerBlocks, + template + ); + if ( ! isEqual( nextBlocks, innerBlocks ) ) { + replaceInnerBlocks( + clientId, + nextBlocks, + innerBlocks.length === 0 && + templateInsertUpdatesSelection && + nextBlocks.length !== 0 + ); + } + } + } + }, [ innerBlocks, templateLock, clientId ] ); +} diff --git a/packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js b/packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js new file mode 100644 index 00000000000000..d894d46f6234c2 --- /dev/null +++ b/packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js @@ -0,0 +1,82 @@ +/** + * WordPress dependencies + */ +import { useEffect } from '@wordpress/element'; +import { useSelect, useDispatch } from '@wordpress/data'; +import isShallowEqual from '@wordpress/is-shallow-equal'; + +/** + * This hook is a side efect which updates the block-editor store when changes + * happen to inner block settings. The given props are transformed into a + * settings object, and if that is different from the current settings object in + * the block-ediotr store, then the store is updated with the new settings which + * came from props. + * + * @param {string} clientId The client ID of the block to update. + * @param {string[]} allowedBlocks An array of block names which are permitted + * in inner blocks. + * @param {string} [templateLock] The template lock specified for the inner + * blocks component. (e.g. "all") + * @param {boolean} captureToolbars Whether or children toolbars should be shown + * in the inner blocks component rather than on + * the child block. + * @param {string} __experimentalMoverDirection The direction in which the block + * should face. + */ +export default function useNestedSettingsUpdate( + clientId, + allowedBlocks, + templateLock, + captureToolbars, + __experimentalMoverDirection +) { + const { updateBlockListSettings } = useDispatch( 'core/block-editor' ); + + const { blockListSettings, parentLock } = useSelect( + ( select ) => { + const rootClientId = select( + 'core/block-editor' + ).getBlockRootClientId( clientId ); + return { + blockListSettings: select( + 'core/block-editor' + ).getBlockListSettings( clientId ), + parentLock: select( 'core/block-editor' ).getTemplateLock( + rootClientId + ), + }; + }, + [ clientId ] + ); + + useEffect( () => { + const newSettings = { + allowedBlocks, + templateLock: + templateLock === undefined ? parentLock : templateLock, + }; + + // These values are not defined for RN, so only include them if they + // are defined. + if ( captureToolbars !== undefined ) { + newSettings.__experimentalCaptureToolbars = captureToolbars; + } + + if ( __experimentalMoverDirection !== undefined ) { + newSettings.__experimentalMoverDirection = __experimentalMoverDirection; + } + + if ( ! isShallowEqual( blockListSettings, newSettings ) ) { + updateBlockListSettings( clientId, newSettings ); + } + }, [ + clientId, + blockListSettings, + allowedBlocks, + templateLock, + parentLock, + captureToolbars, + __experimentalMoverDirection, + updateBlockListSettings, + ] ); +} diff --git a/packages/block-library/src/template-part/edit/inner-blocks.js b/packages/block-library/src/template-part/edit/inner-blocks.js index f6eb61341ca692..cb2292583e39a4 100644 --- a/packages/block-library/src/template-part/edit/inner-blocks.js +++ b/packages/block-library/src/template-part/edit/inner-blocks.js @@ -14,7 +14,7 @@ export default function TemplatePartInnerBlocks() { ); return ( From 0fb12c0bfc8219b506ad77e5b32dce8665497405 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Mon, 27 Apr 2020 16:48:43 -0700 Subject: [PATCH 4/9] Integrate reducers with controlled inner blocks - Correctly include and exclude clientIDs from block reset depending on whether they are part of an inner block controller or not. - Also handle inner block controllers themselves, not updating their cache or order when the action occurs - Stop invalidating grandparent controller cache - Fix undefined index access in selector state - Handle replaceInnerBlocks action similar to the reset blocks action so that we do not inadvertantly delete a template part from the editor. There were a lot of bugs around RESET_BLOCKS and REPLACE_INNER_BLOCKS actions as relating to controlled inner blocks. This is because those actions tend to delet all blocks from the editor without providing back all of the blocks to reinsert. This is because the entity which dispatches the action does not have access to the inner blocks of other entities, so they are not dispatched in the action. These changes update the reducers to make sure that nested inner block controllers are not deleted when they should stay in the editor. Additionally, there are some fixes for the cache state so that it does not invalidate when the changes affect a different entity. --- packages/block-editor/src/store/reducer.js | 145 ++++++++++++++++++--- 1 file changed, 127 insertions(+), 18 deletions(-) diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 5b25a5b1eadaec..223fe269508841 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -16,6 +16,7 @@ import { identity, difference, omitBy, + pickBy, } from 'lodash'; /** @@ -129,22 +130,38 @@ function getFlattenedBlockAttributes( blocks ) { * * Calling this with `rootClientId` set to `''` results in a list of client IDs * that are in the post. That is, it excludes blocks like fetched reusable - * blocks which are stored into state but not visible. + * blocks which are stored into state but not visible. It also excludes + * InnerBlocks controllers, like template parts. + * + * It is important to exclude the full inner block controller and not just the + * inner blocks because in many cases, we need to persist the previous value of + * an inner block controller. To do so, it must be excluded from the list of + * client IDs which are considered to be part of the top-level entity. * * @param {Object} blocksOrder Object that maps block client IDs to a list of * nested block client IDs. * @param {?string} rootClientId The root client ID to search. Defaults to ''. + * @param {?Object} controlledInnerBlocks The InnerBlocks controller state. * * @return {Array} List of descendant client IDs. */ -function getNestedBlockClientIds( blocksOrder, rootClientId = '' ) { +function getNestedBlockClientIds( + blocksOrder, + rootClientId = '', + controlledInnerBlocks = {} +) { return reduce( blocksOrder[ rootClientId ], - ( result, clientId ) => [ - ...result, - clientId, - ...getNestedBlockClientIds( blocksOrder, clientId ), - ], + ( result, clientId ) => { + if ( !! controlledInnerBlocks[ clientId ] ) { + return result; + } + return [ + ...result, + clientId, + ...getNestedBlockClientIds( blocksOrder, clientId ), + ]; + }, [] ); } @@ -246,7 +263,7 @@ const withBlockCache = ( reducer ) => ( state = {}, action ) => { do { result.push( current ); current = state.parents[ current ]; - } while ( current ); + } while ( current && ! state.controlledInnerBlocks[ current ] ); return result; }, [] ); }; @@ -261,7 +278,10 @@ const withBlockCache = ( reducer ) => ( state = {}, action ) => { case 'RECEIVE_BLOCKS': case 'INSERT_BLOCKS': { const updatedBlockUids = keys( flattenBlocks( action.blocks ) ); - if ( action.rootClientId ) { + if ( + action.rootClientId && + ! state.controlledInnerBlocks[ action.rootClientId ] + ) { updatedBlockUids.push( action.rootClientId ); } newState.cache = { @@ -455,10 +475,15 @@ function withIgnoredBlockChange( reducer ) { * @return {Function} Enhanced reducer function. */ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { + // Gets all children which need to be removed. const getAllChildren = ( clientIds ) => { let result = clientIds; for ( let i = 0; i < result.length; i++ ) { - if ( ! state.order[ result[ i ] ] ) { + if ( + ! state.order[ result[ i ] ] || + ( action.keepControlledInnerBlocks && + action.keepControlledInnerBlocks[ result[ i ] ] ) + ) { continue; } @@ -497,7 +522,7 @@ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { * Higher-order reducer which targets the combined blocks reducer and handles * the `RESET_BLOCKS` action. When dispatched, this action will replace all * blocks that exist in the post, leaving blocks that exist only in state (e.g. - * reusable blocks) alone. + * reusable blocks and blocks controlled by inner blocks controllers) alone. * * @param {Function} reducer Original reducer function. * @@ -505,7 +530,43 @@ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { */ const withBlockReset = ( reducer ) => ( state, action ) => { if ( state && action.type === 'RESET_BLOCKS' ) { - const visibleClientIds = getNestedBlockClientIds( state.order ); + /** + * A list of client IDs associated with the top level entity (like a + * post or template). It excludes the client IDs of blocks associated + * with other entities, like inner block controllers or reusable blocks. + */ + const visibleClientIds = getNestedBlockClientIds( + state.order, + '', + state.controlledInnerBlocks + ); + + // pickBy returns only the truthy values from controlledInnerBlocks + const controlledInnerBlocks = Object.keys( + pickBy( state.controlledInnerBlocks ) + ); + + /** + * Each update operation consists of a few parts: + * 1. First, the client IDs associated with the top level entity are + * removed from the existing state key, leaving in place controlled + * blocks (like reusable blocks and inner block controllers). + * 2. Second, the blocks from the reset action are used to calculate the + * individual state keys. This will re-populate the clientIDs which + * were removed in step 1. + * 3. In some cases, we remove the recalculated inner block controllers, + * letting their old values persist. We need to do this because the + * reset block action from a top-level entity is not aware of any + * inner blocks inside InnerBlock controllers. So if the new values + * were used, it would not take into account the existing InnerBlocks + * which already exist in the state for inner block controllers. For + * example, `attributes` uses the newly computed value for controllers + * since attributes are stored in the top-level entity. But `order` + * uses the previous value for the controllers since the new value + * does not include the order of controlled inner blocks. So if the + * new value was used, template parts would disappear from the editor + * whenever you try to undo a change in the top level entity. + */ return { ...state, byClientId: { @@ -518,7 +579,10 @@ const withBlockReset = ( reducer ) => ( state, action ) => { }, order: { ...omit( state.order, visibleClientIds ), - ...mapBlockOrder( action.blocks ), + ...omit( + mapBlockOrder( action.blocks ), + controlledInnerBlocks + ), }, parents: { ...omit( state.parents, visibleClientIds ), @@ -526,7 +590,10 @@ const withBlockReset = ( reducer ) => ( state, action ) => { }, cache: { ...omit( state.cache, visibleClientIds ), - ...mapValues( flattenBlocks( action.blocks ), () => ( {} ) ), + ...omit( + mapValues( flattenBlocks( action.blocks ), () => ( {} ) ), + controlledInnerBlocks + ), }, }; } @@ -536,9 +603,10 @@ const withBlockReset = ( reducer ) => ( state, action ) => { /** * Higher-order reducer which targets the combined blocks reducer and handles - * the `REPLACE_INNER_BLOCKS` action. When dispatched, this action the state should become equivalent - * to the execution of a `REMOVE_BLOCKS` action containing all the child's of the root block followed by - * the execution of `INSERT_BLOCKS` with the new blocks. + * the `REPLACE_INNER_BLOCKS` action. When dispatched, this action the state + * should become equivalent to the execution of a `REMOVE_BLOCKS` action + * containing all the child's of the root block followed by the execution of + * `INSERT_BLOCKS` with the new blocks. * * @param {Function} reducer Original reducer function. * @@ -548,10 +616,33 @@ const withReplaceInnerBlocks = ( reducer ) => ( state, action ) => { if ( action.type !== 'REPLACE_INNER_BLOCKS' ) { return reducer( state, action ); } + + // Finds every nested inner block controller. We must check the action blocks + // and not just the block parent state because some inner block controllers + // should be deleted if specified, whereas others should not be deleted. If + // a controlled should not be deleted, then we need to avoid deleting its + // inner blocks from the block state because its inner blocks will not be + // attached to the block in the action. + const nestedControllers = {}; + if ( Object.keys( state.controlledInnerBlocks ).length ) { + const stack = [ ...action.blocks ]; + while ( stack.length ) { + const { innerBlocks, ...block } = stack.shift(); + stack.push( ...innerBlocks ); + if ( !! state.controlledInnerBlocks[ block.clientId ] ) { + nestedControllers[ block.clientId ] = true; + } + } + } + + // The `keepControlledInnerBlocks` prop will keep the inner blocks of the + // marked block in the block state so that they can be reattached to the + // marked block when we re-insert everything a few lines below. let stateAfterBlocksRemoval = state; if ( state.order[ action.rootClientId ] ) { stateAfterBlocksRemoval = reducer( stateAfterBlocksRemoval, { type: 'REMOVE_BLOCKS', + keepControlledInnerBlocks: nestedControllers, clientIds: state.order[ action.rootClientId ], } ); } @@ -562,6 +653,23 @@ const withReplaceInnerBlocks = ( reducer ) => ( state, action ) => { type: 'INSERT_BLOCKS', index: 0, } ); + + // We need to re-attach the block order of the controlled inner blocks. + // Otherwise, an inner block controller's blocks will be deleted entirely + // from its entity.. + stateAfterInsert.order = { + ...stateAfterInsert.order, + ...reduce( + nestedControllers, + ( result, value, key ) => { + if ( state.order[ key ] ) { + result[ key ] = state.order[ key ]; + } + return result; + }, + {} + ), + }; } return stateAfterInsert; }; @@ -1072,7 +1180,8 @@ function selection( state = {}, action ) { return { clientId: action.clientId }; case 'REPLACE_INNER_BLOCKS': // REPLACE_INNER_BLOCKS and INSERT_BLOCKS should follow the same logic. case 'INSERT_BLOCKS': { - if ( ! action.updateSelection ) { + // REPLACE_INNER_BLOCKS can be called with an empty array. + if ( ! action.updateSelection || ! action.blocks.length ) { return state; } From 5344bc22982c85c920ce9d864a3006c55d54f64b Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Thu, 7 May 2020 11:48:07 -0700 Subject: [PATCH 5/9] Wait for block to display before continuing Some tests began failing since the block did not exist immediately when the tests tried to interact with the inserted blocks. --- packages/e2e-test-utils/README.md | 2 +- packages/e2e-test-utils/src/insert-block.js | 24 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/e2e-test-utils/README.md b/packages/e2e-test-utils/README.md index 1e84e815f74dc3..d2a08ea8068805 100644 --- a/packages/e2e-test-utils/README.md +++ b/packages/e2e-test-utils/README.md @@ -282,7 +282,7 @@ _Returns_ # **insertBlock** Opens the inserter, searches for the given term, then selects the first -result that appears. +result that appears. It then waits briefly for the block list to update. _Parameters_ diff --git a/packages/e2e-test-utils/src/insert-block.js b/packages/e2e-test-utils/src/insert-block.js index a3a31994165969..2f097521254f60 100644 --- a/packages/e2e-test-utils/src/insert-block.js +++ b/packages/e2e-test-utils/src/insert-block.js @@ -2,17 +2,39 @@ * Internal dependencies */ import { searchForBlock } from './search-for-block'; +import { getAllBlocks } from './get-all-blocks'; /** * Opens the inserter, searches for the given term, then selects the first - * result that appears. + * result that appears. It then waits briefly for the block list to update. * * @param {string} searchTerm The text to search the inserter for. */ export async function insertBlock( searchTerm ) { + const oldBlocks = getAllBlocks(); + await searchForBlock( searchTerm ); const insertButton = ( await page.$x( `//button//span[contains(text(), '${ searchTerm }')]` ) )[ 0 ]; await insertButton.click(); + + const waitForBlocksToChange = ( delay = 20 ) => + new Promise( ( resolve, reject ) => { + let elapsedTime = 0; + const pendingBlockList = setInterval( () => { + const blocks = getAllBlocks(); + // Reference will change when the selector updates. + if ( blocks !== oldBlocks ) { + clearInterval( pendingBlockList ); + resolve(); + } + elapsedTime += delay; + if ( elapsedTime > 600 ) { + clearInterval( pendingBlockList ); + reject( `Block ${ searchTerm } was never inserted.` ); + } + }, delay ); + } ); + await waitForBlocksToChange(); } From 8bb23553809706fe479e0a950355f3a8df3c8c5f Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Thu, 7 May 2020 21:58:23 -0700 Subject: [PATCH 6/9] Add e2e test for multi-entity dirty state --- .../experiments/multi-entity-editing.test.js | 278 ++++++++++++++++++ .../src/components/add-template/index.js | 2 +- 2 files changed, 279 insertions(+), 1 deletion(-) create mode 100644 packages/e2e-tests/specs/experiments/multi-entity-editing.test.js diff --git a/packages/e2e-tests/specs/experiments/multi-entity-editing.test.js b/packages/e2e-tests/specs/experiments/multi-entity-editing.test.js new file mode 100644 index 00000000000000..4d6e6fe1ea5623 --- /dev/null +++ b/packages/e2e-tests/specs/experiments/multi-entity-editing.test.js @@ -0,0 +1,278 @@ +/** + * External dependencies + */ +import { kebabCase } from 'lodash'; + +/** + * WordPress dependencies + */ +import { insertBlock, visitAdminPage } from '@wordpress/e2e-test-utils'; +import { addQueryArgs } from '@wordpress/url'; + +/** + * Internal dependencies + */ +import { + enableExperimentalFeatures, + disableExperimentalFeatures, +} from '../../experimental-features'; +import { trashExistingPosts } from '../../config/setup-test-framework'; + +const visitSiteEditor = async () => { + const query = addQueryArgs( '', { + page: 'gutenberg-edit-site', + } ).slice( 1 ); + await visitAdminPage( 'admin.php', query ); + // Waits for the template part to load... + await page.waitForSelector( + '.wp-block[data-type="core/template-part"] .block-editor-inner-blocks' + ); +}; + +const openTemplateDropdown = async () => { + // Open the dropdown menu. + const templateDropdown = + 'button.components-dropdown-menu__toggle[aria-label="Switch Template"]'; + await page.click( templateDropdown ); + await page.waitForSelector( '.edit-site-template-switcher__popover' ); +}; + +const getTemplateDropdownElement = async ( itemName ) => { + await openTemplateDropdown(); + const [ item ] = await page.$x( + `//div[contains(@class, "edit-site-template-switcher__popover")]//button[contains(., "${ itemName }")]` + ); + return item; +}; + +const createTemplate = async ( templateName = 'Test Template' ) => { + // Click the "new template" button. + const createNewTemplateButton = await getTemplateDropdownElement( 'New' ); + await createNewTemplateButton.click(); + await page.waitForSelector( '.components-modal__frame' ); + + // Create a new template with the given name. + await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Tab' ); + await page.keyboard.type( templateName ); + const [ addTemplateButton ] = await page.$x( + '//div[contains(@class, "components-modal__frame")]//button[contains(., "Add")]' + ); + await addTemplateButton.click(); + + // Wait for the site editor to load the new template. + await page.waitForXPath( + `//button[contains(@class, "components-dropdown-menu__toggle")][contains(text(), "${ kebabCase( + templateName + ) }")]`, + { timeout: 3000 } + ); +}; + +const createTemplatePart = async ( + templatePartName = 'test-template-part', + themeName = 'test-theme', + isNested = false +) => { + // Create new template part. + await insertBlock( 'Template Part' ); + await page.keyboard.type( templatePartName ); + await page.keyboard.press( 'Tab' ); + await page.keyboard.type( themeName ); + await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Enter' ); + await page.waitForSelector( + isNested + ? '.wp-block[data-type="core/template-part"] .wp-block[data-type="core/template-part"] .block-editor-inner-blocks' + : '.wp-block[data-type="core/template-part"] .block-editor-inner-blocks' + ); +}; + +const editTemplatePart = async ( textToAdd, isNested = false ) => { + await page.click( + isNested + ? '.wp-block[data-type="core/template-part"] .wp-block[data-type="core/template-part"]' + : '.wp-block[data-type="core/template-part"]' + ); + for ( const text of textToAdd ) { + await page.keyboard.type( text ); + await page.keyboard.press( 'Enter' ); + } +}; + +const saveAllEntities = async () => { + if ( await openEntitySavePanel() ) { + await page.click( 'button.editor-entities-saved-states__save-button' ); + } +}; + +const openEntitySavePanel = async () => { + // Open the entity save panel if it is not already open. + try { + await page.waitForSelector( '.entities-saved-states__panel', { + timeout: 100, + } ); + } catch { + try { + await page.click( + '.edit-site-save-button__button[aria-disabled=false]', + { timeout: 100 } + ); + } catch { + return false; // Not dirty because the button is disabled. + } + await page.waitForSelector( '.entities-saved-states__panel' ); + } + // If we made it this far, the panel is opened. + return true; +}; + +const isEntityDirty = async ( name ) => { + const isOpen = await openEntitySavePanel(); + if ( ! isOpen ) { + return false; + } + try { + await page.waitForXPath( + `//label[@class="components-checkbox-control__label"]//strong[contains(text(),"${ name }")]`, + { timeout: 500 } + ); + return true; + } catch {} + return false; +}; + +const removeErrorMocks = () => { + // TODO: Add back console mocks when + // https://github.com/WordPress/gutenberg/issues/17355 is fixed. + /* eslint-disable no-console */ + console.warn.mockReset(); + console.error.mockReset(); + console.info.mockReset(); + /* eslint-enable no-console */ +}; + +describe( 'Multi-entity editor states', () => { + // Setup & Teardown. + const requiredExperiments = [ + '#gutenberg-full-site-editing', + '#gutenberg-full-site-editing-demo', + ]; + + const templatePartName = 'Test Template Part Name Edit'; + const templateName = 'Test Template Name Edit'; + const nestedTPName = 'Test Nested Template Part Name Edit'; + + beforeAll( async () => { + await enableExperimentalFeatures( requiredExperiments ); + await trashExistingPosts( 'wp_template' ); + await trashExistingPosts( 'wp_template_part' ); + } ); + + afterAll( async () => { + await disableExperimentalFeatures( requiredExperiments ); + } ); + + it( 'should not display any dirty entities when loading the site editor', async () => { + await visitSiteEditor(); + expect( await openEntitySavePanel() ).toBe( true ); + + await saveAllEntities(); + await visitSiteEditor(); + + // Unable to open the save panel implies that no entities are dirty. + expect( await openEntitySavePanel() ).toBe( false ); + } ); + + it( 'should not dirty an entity by switching to it in the template dropdown', async () => { + const templatePartButton = await getTemplateDropdownElement( 'header' ); + await templatePartButton.click(); + + // Wait for blocks to load. + await page.waitForSelector( '.wp-block' ); + expect( await isEntityDirty( 'header' ) ).toBe( false ); + expect( await isEntityDirty( 'front-page' ) ).toBe( false ); + + // Switch back and make sure it is still clean. + const templateButton = await getTemplateDropdownElement( 'front-page' ); + await templateButton.click(); + await page.waitForSelector( '.wp-block' ); + expect( await isEntityDirty( 'header' ) ).toBe( false ); + expect( await isEntityDirty( 'front-page' ) ).toBe( false ); + + removeErrorMocks(); + } ); + + describe( 'Multi-entity edit', () => { + beforeAll( async () => { + await visitSiteEditor(); + await createTemplate( templateName ); + await createTemplatePart( templatePartName ); + await editTemplatePart( [ + 'Default template part test text.', + 'Second paragraph test.', + ] ); + await createTemplatePart( nestedTPName, 'test-theme', true ); + await editTemplatePart( + [ 'Nested Template Part Text.', 'Second Nested test.' ], + true + ); + await saveAllEntities(); + removeErrorMocks(); + } ); + + afterEach( async () => { + await saveAllEntities(); + removeErrorMocks(); + } ); + + it( 'should only dirty the parent entity when editing the parent', async () => { + await page.click( '.block-editor-button-block-appender' ); + await page.waitForSelector( '.block-editor-inserter__menu' ); + await page.click( 'button.editor-block-list-item-paragraph' ); + + // Add changes to the main parent entity. + await page.keyboard.type( 'Test.' ); + + expect( await isEntityDirty( templateName ) ).toBe( true ); + expect( await isEntityDirty( templatePartName ) ).toBe( false ); + expect( await isEntityDirty( nestedTPName ) ).toBe( false ); + } ); + + it( 'should only dirty the child when editing the child', async () => { + await page.click( + '.wp-block[data-type="core/template-part"] .wp-block[data-type="core/paragraph"]' + ); + await page.keyboard.type( 'Some more test words!' ); + + expect( await isEntityDirty( templateName ) ).toBe( false ); + expect( await isEntityDirty( templatePartName ) ).toBe( true ); + expect( await isEntityDirty( nestedTPName ) ).toBe( false ); + } ); + + it( 'should only dirty the nested entity when editing the nested entity', async () => { + await page.click( + '.wp-block[data-type="core/template-part"] .wp-block[data-type="core/template-part"] .wp-block[data-type="core/paragraph"]' + ); + await page.keyboard.type( 'Nested test words!' ); + + expect( await isEntityDirty( templateName ) ).toBe( false ); + expect( await isEntityDirty( templatePartName ) ).toBe( false ); + expect( await isEntityDirty( nestedTPName ) ).toBe( true ); + } ); + + it( 'should not dirty any entities when hovering over template preview', async () => { + const mainTemplateButton = await getTemplateDropdownElement( + kebabCase( templateName ) + ); + // Hover and wait for template/template part to load. + await mainTemplateButton.hover(); + await page.waitForSelector( + '.edit-site-template-switcher__template-preview .wp-block[data-type="core/template-part"]' + ); + expect( await isEntityDirty( templateName ) ).toBe( false ); + expect( await isEntityDirty( templatePartName ) ).toBe( false ); + expect( await isEntityDirty( nestedTPName ) ).toBe( false ); + } ); + } ); +} ); diff --git a/packages/edit-site/src/components/add-template/index.js b/packages/edit-site/src/components/add-template/index.js index 9cdf5352001231..1337a5f45afe1d 100644 --- a/packages/edit-site/src/components/add-template/index.js +++ b/packages/edit-site/src/components/add-template/index.js @@ -30,7 +30,7 @@ export default function AddTemplate( { ); const { saveEntityRecord } = useDispatch( 'core' ); - const [ slug, _setSlug ] = useState(); + const [ slug, _setSlug ] = useState( '' ); const [ help, setHelp ] = useState(); const setSlug = useCallback( ( nextSlug ) => { From 7c31ce3267ac944faeec53332ec0832ee88d7e41 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Mon, 11 May 2020 19:02:25 -0700 Subject: [PATCH 7/9] Add unit tests for useBlockSync hook --- .../provider/test/use-block-sync.js | 368 ++++++++++++++++++ 1 file changed, 368 insertions(+) create mode 100644 packages/block-editor/src/components/provider/test/use-block-sync.js 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 new file mode 100644 index 00000000000000..29b6b15ed3f1d0 --- /dev/null +++ b/packages/block-editor/src/components/provider/test/use-block-sync.js @@ -0,0 +1,368 @@ +/** + * External dependencies + */ +import { create, act } from 'react-test-renderer'; + +/** + * Internal dependencies + */ +import useBlockSync from '../use-block-sync'; +import withRegistryProvider from '../with-registry-provider'; +import * as blockEditorActions from '../../../store/actions'; + +const TestWrapper = withRegistryProvider( ( props ) => { + if ( props.setRegistry ) { + props.setRegistry( props.registry ); + } + useBlockSync( props ); + return

Test.

; +} ); + +describe( 'useBlockSync hook', () => { + afterEach( () => { + jest.clearAllMocks(); + } ); + + it( 'resets the block-editor blocks when the controll value changes', async () => { + const fakeBlocks = []; + const resetBlocks = jest.spyOn( blockEditorActions, 'resetBlocks' ); + const replaceInnerBlocks = jest.spyOn( + blockEditorActions, + 'replaceInnerBlocks' + ); + const onChange = jest.fn(); + const onInput = jest.fn(); + + let root; + await act( async () => { + root = create( + + ); + } ); + + // Reset blocks should be called on mount. + expect( onChange ).not.toHaveBeenCalled(); + expect( onInput ).not.toHaveBeenCalled(); + expect( replaceInnerBlocks ).not.toHaveBeenCalled(); + expect( resetBlocks ).toHaveBeenCalledWith( fakeBlocks ); + + const testBlocks = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + await act( async () => { + root.update( + + ); + } ); + + // Reset blocks should be called when the incoming value changes. + expect( onChange ).not.toHaveBeenCalled(); + expect( onInput ).not.toHaveBeenCalled(); + expect( replaceInnerBlocks ).not.toHaveBeenCalled(); + expect( resetBlocks ).toHaveBeenCalledWith( testBlocks ); + } ); + + it( 'replaces the inner blocks of a block when the control value changes if a clientId is passed', async () => { + const fakeBlocks = []; + const replaceInnerBlocks = jest.spyOn( + blockEditorActions, + 'replaceInnerBlocks' + ); + const resetBlocks = jest.spyOn( blockEditorActions, 'resetBlocks' ); + const onChange = jest.fn(); + const onInput = jest.fn(); + + let root; + await act( async () => { + root = create( + + ); + } ); + + expect( resetBlocks ).not.toHaveBeenCalled(); + expect( onChange ).not.toHaveBeenCalled(); + expect( onInput ).not.toHaveBeenCalled(); + expect( replaceInnerBlocks ).toHaveBeenCalledWith( + 'test', // It should use the given client ID. + fakeBlocks, // It should use the controlled blocks value. + false // It shoudl not update the selection state. + ); + + const testBlocks = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + await act( async () => { + root.update( + + ); + } ); + + // Reset blocks should be called when the incoming value changes. + expect( onChange ).not.toHaveBeenCalled(); + expect( onInput ).not.toHaveBeenCalled(); + expect( resetBlocks ).not.toHaveBeenCalled(); + expect( replaceInnerBlocks ).toHaveBeenCalledWith( + 'test', + testBlocks, + false + ); + } ); + + it( 'does not add the controlled blocks to the block-editor store if the store already contains them', async () => { + const replaceInnerBlocks = jest.spyOn( + blockEditorActions, + 'replaceInnerBlocks' + ); + const onChange = jest.fn(); + const onInput = jest.fn(); + + const value1 = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + let root; + let registry; + const setRegistry = ( reg ) => { + registry = reg; + }; + await act( async () => { + root = create( + + ); + } ); + + registry + .dispatch( 'core/block-editor' ) + .updateBlockAttributes( 'a', { foo: 2 } ); + + const newBlockValue = registry + .select( 'core/block-editor' ) + .getBlocks( 'test' ); + replaceInnerBlocks.mockClear(); + + // Assert that the reference has changed so that the side effect will be + // triggered once more. + expect( newBlockValue ).not.toBe( value1 ); + + await act( async () => { + root.update( + + ); + } ); + + // replaceInnerBlocks should not be called when the controlling + // block value is the same as what already exists in the store. + expect( replaceInnerBlocks ).not.toHaveBeenCalled(); + } ); + + it( 'sets a block as an inner block controller if a clientId is provided', async () => { + const setAsController = jest.spyOn( + blockEditorActions, + 'setHasControlledInnerBlocks' + ); + + await act( async () => { + create( + + ); + } ); + expect( setAsController ).toHaveBeenCalledWith( 'test', true ); + } ); + + it( 'calls onInput when a non-persistent block change occurs', async () => { + const onChange = jest.fn(); + const onInput = jest.fn(); + + const value1 = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + let registry; + const setRegistry = ( reg ) => { + registry = reg; + }; + await act( async () => { + create( + + ); + } ); + onChange.mockClear(); + onInput.mockClear(); + + // Create a non-persistent change. + registry + .dispatch( 'core/block-editor' ) + .__unstableMarkNextChangeAsNotPersistent(); + registry + .dispatch( 'core/block-editor' ) + .updateBlockAttributes( 'a', { foo: 2 } ); + + expect( onInput ).toHaveBeenCalledWith( + [ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ], + { selectionEnd: {}, selectionStart: {} } + ); + expect( onChange ).not.toHaveBeenCalled(); + } ); + + it( 'calls onChange if a persistent change occurs', async () => { + const onChange = jest.fn(); + const onInput = jest.fn(); + + const value1 = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + let registry; + const setRegistry = ( reg ) => { + registry = reg; + }; + await act( async () => { + create( + + ); + } ); + onChange.mockClear(); + onInput.mockClear(); + + // Create a persistent change. + registry + .dispatch( 'core/block-editor' ) + .updateBlockAttributes( 'a', { foo: 2 } ); + + expect( onChange ).toHaveBeenCalledWith( + [ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ], + { selectionEnd: {}, selectionStart: {} } + ); + expect( onInput ).not.toHaveBeenCalled(); + } ); + + it( 'avoids updating the parent if there is a pending incoming change', async () => { + const replaceInnerBlocks = jest.spyOn( + blockEditorActions, + 'replaceInnerBlocks' + ); + + const onChange = jest.fn(); + const onInput = jest.fn(); + + const value1 = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + + await act( async () => { + create( + + ); + } ); + onChange.mockClear(); + onInput.mockClear(); + replaceInnerBlocks.mockClear(); + + await act( async () => { + create( + + ); + } ); + + expect( replaceInnerBlocks ).toHaveBeenCalledWith( 'test', [], false ); + expect( onChange ).not.toHaveBeenCalled(); + expect( onInput ).not.toHaveBeenCalled(); + } ); + + it( 'avoids updating the block-editor store if there is a pending outgoint change', async () => { + const replaceInnerBlocks = jest.spyOn( + blockEditorActions, + 'replaceInnerBlocks' + ); + + const onChange = jest.fn(); + const onInput = jest.fn(); + + const value1 = [ + { clientId: 'a', innerBlocks: [], attributes: { foo: 1 } }, + ]; + + let registry; + const setRegistry = ( reg ) => { + registry = reg; + }; + await act( async () => { + create( + + ); + } ); + onChange.mockClear(); + onInput.mockClear(); + replaceInnerBlocks.mockClear(); + + registry + .dispatch( 'core/block-editor' ) + .updateBlockAttributes( 'a', { foo: 2 } ); + + expect( replaceInnerBlocks ).not.toHaveBeenCalled(); + expect( onChange ).toHaveBeenCalledWith( + [ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ], + { selectionEnd: {}, selectionStart: {} } + ); + expect( onInput ).not.toHaveBeenCalled(); + } ); +} ); From 906e95b8e80916bac71aee6d2b245dd1802e69c6 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Tue, 12 May 2020 12:11:08 -0700 Subject: [PATCH 8/9] Update block-editor changelog --- packages/block-editor/CHANGELOG.md | 42 ++++++++++++++++-------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index a7002b25cb7144..61b17c2b875581 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -2,63 +2,67 @@ ## Unreleased +### Breaking Changes + +- The block controll value for `InnerBlocks` has been changed from `__experimentalBlocks` to `value` and is now considered a stable API. + ## 3.7.0 (2020-02-10) ### New Features -- Add new `__experimentalEditorSkeleton` component. This has been moved over from the `@wordpress/edit-post` package, where it was an internal component called `EditorRegions`. Its class names have thus been renamed from `edit-post-editor-regions` to `block-editor-editor-skeleton`. +- Add new `__experimentalEditorSkeleton` component. This has been moved over from the `@wordpress/edit-post` package, where it was an internal component called `EditorRegions`. Its class names have thus been renamed from `edit-post-editor-regions` to `block-editor-editor-skeleton`. ## 3.3.0 (2019-11-14) ### New Features -- Added a `label` prop to `URLInput`. This allows the label to be set without needing to wrap the `URLInput` in a `BaseControl`. +- Added a `label` prop to `URLInput`. This allows the label to be set without needing to wrap the `URLInput` in a `BaseControl`. ### Deprecation -- `dropZoneUIOnly` prop in `MediaPlaceholder` component has been deprecated in favor of `disableMediaButtons` prop. +- `dropZoneUIOnly` prop in `MediaPlaceholder` component has been deprecated in favor of `disableMediaButtons` prop. ## 3.0.0 (2019-08-05) ### New Features -- Added a new `allowedFormats` prop to `RichText` to fine tune allowed formats. Deprecated the `formattingControls` prop in favour of this. Also added a `withoutInteractiveFormatting` to specifically disable format types that would insert interactive elements, which can not be nested. +- Added a new `allowedFormats` prop to `RichText` to fine tune allowed formats. Deprecated the `formattingControls` prop in favour of this. Also added a `withoutInteractiveFormatting` to specifically disable format types that would insert interactive elements, which can not be nested. ### Breaking Changes -- `BlockEditorProvider` no longer renders a wrapping `SlotFillProvider` or `DropZoneProvider` (from `@wordpress/components`). For custom block editors, you should render your own as wrapping the `BlockEditorProvider`. A future release will include a new `BlockEditor` component for simple, standard usage. `BlockEditorProvider` will serve the simple purpose of establishing its own context for block editors. +- `BlockEditorProvider` no longer renders a wrapping `SlotFillProvider` or `DropZoneProvider` (from `@wordpress/components`). For custom block editors, you should render your own as wrapping the `BlockEditorProvider`. A future release will include a new `BlockEditor` component for simple, standard usage. `BlockEditorProvider` will serve the simple purpose of establishing its own context for block editors. ## 2.2.0 (2019-06-12) ### Internal -- Refactored `BlockSettingsMenu` to use `DropdownMenu` from `@wordpress/components`. +- Refactored `BlockSettingsMenu` to use `DropdownMenu` from `@wordpress/components`. ## 2.0.0 (2019-04-16) ### New Features -- Added the `addToGallery` property to the `MediaUpload` interface. The property allows users to open the media modal in the `gallery-library`instead of `gallery-edit` state. -- Added the `addToGallery` property to the `MediaPlaceholder` component. The component passes the property to the `MediaUpload` component used inside the placeholder. -- Added the `isAppender` property to the `MediaPlaceholder` component. The property changes the look of the placeholder to be adequate to scenarios where new files are added to an already existing set of files, e.g., adding files to a gallery. -- Added the `dropZoneUIOnly` property to the `MediaPlaceholder` component. The property makes the `MediaPlaceholder` only render a dropzone without any other additional UI. -- Added a cancel link to the list of buttons in the `MediaPlaceholder` component which appears if an `onCancel` handler exists. -- Added the usage of `mediaPreview` for the `Placeholder` component to the `MediaPlaceholder` component. -- Added a an `onDoubleClick` event handler to the `MediaPlaceholder` component. -- Added a way to pass special `ref` property to the `PlainText` component. -- The `URLPopover` component now passes through all unhandled props to the underlying Popover component. +- Added the `addToGallery` property to the `MediaUpload` interface. The property allows users to open the media modal in the `gallery-library`instead of `gallery-edit` state. +- Added the `addToGallery` property to the `MediaPlaceholder` component. The component passes the property to the `MediaUpload` component used inside the placeholder. +- Added the `isAppender` property to the `MediaPlaceholder` component. The property changes the look of the placeholder to be adequate to scenarios where new files are added to an already existing set of files, e.g., adding files to a gallery. +- Added the `dropZoneUIOnly` property to the `MediaPlaceholder` component. The property makes the `MediaPlaceholder` only render a dropzone without any other additional UI. +- Added a cancel link to the list of buttons in the `MediaPlaceholder` component which appears if an `onCancel` handler exists. +- Added the usage of `mediaPreview` for the `Placeholder` component to the `MediaPlaceholder` component. +- Added a an `onDoubleClick` event handler to the `MediaPlaceholder` component. +- Added a way to pass special `ref` property to the `PlainText` component. +- The `URLPopover` component now passes through all unhandled props to the underlying Popover component. ### Breaking Changes -- `CopyHandler` will now only catch cut/copy events coming from its `props.children`, instead of from anywhere in the `document`. +- `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). +- 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 -- Initial version. +- Initial version. From 8253f490abf83044cb7a16e77f395b18beba3848 Mon Sep 17 00:00:00 2001 From: Noah Allen Date: Tue, 12 May 2020 13:58:37 -0700 Subject: [PATCH 9/9] Fix typo in changelog Co-authored-by: Chris Van Patten --- packages/block-editor/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index 61b17c2b875581..27311c857ea90b 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -4,7 +4,7 @@ ### Breaking Changes -- The block controll value for `InnerBlocks` has been changed from `__experimentalBlocks` to `value` and is now considered a stable API. +- The block control value for `InnerBlocks` has been changed from `__experimentalBlocks` to `value` and is now considered a stable API. ## 3.7.0 (2020-02-10)