diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js index 0888b440faebd..61d8cf4c2d559 100644 --- a/packages/editor/src/store/selectors.js +++ b/packages/editor/src/store/selectors.js @@ -374,24 +374,30 @@ export function isEditedPostSaveable( state ) { export function isEditedPostEmpty( state ) { const blocks = getBlocksForSerialization( state ); - // While the condition of truthy content string would be sufficient for - // determining emptiness, testing saveable blocks length is a trivial - // operation by comparison. Since this function can be called frequently, - // optimize for the fast case where saveable blocks are non-empty. - if ( ! blocks.length ) { - return true; - } - - const freeFormBlockName = getFreeformContentHandlerName(); + // While the condition of truthy content string is sufficient to determine + // emptiness, testing saveable blocks length is a trivial operation. Since + // this function can be called frequently, optimize for the fast case as a + // condition of the mere existence of blocks. Note that the value of edited + // content is used in place of blocks, thus allowed to fall through. + if ( blocks.length && ! ( 'content' in getPostEdits( state ) ) ) { + // Pierce the abstraction of the serializer in knowing that blocks are + // joined with with newlines such that even if every individual block + // produces an empty save result, the serialized content is non-empty. + if ( blocks.length > 1 ) { + return false; + } - if ( ! some( blocks, { name: freeFormBlockName } ) ) { - return false; + // Freeform and unregistered blocks omit comment delimiters in their + // output. The freeform block specifically may produce an empty string + // to save. In the case of a single freeform block, fall through to the + // full serialize. Otherwise, the single block is assumed non-empty by + // virtue of its comment delimiters. + if ( blocks[ 0 ].name !== getFreeformContentHandlerName() ) { + return false; + } } - // Content serialization is considered an expensive operation, and should - // only be considered after more trivial operations of assuming presence of - // non-freeform blocks as implying that serializable content exists. - return ! getEditedPostAttribute( state, 'content' ); + return ! getEditedPostContent( state ); } /** diff --git a/packages/editor/src/store/test/selectors.js b/packages/editor/src/store/test/selectors.js index 7892e3f484226..0daa48afa97fe 100644 --- a/packages/editor/src/store/test/selectors.js +++ b/packages/editor/src/store/test/selectors.js @@ -13,7 +13,6 @@ import { getBlockTypes, getDefaultBlockName, setDefaultBlockName, - getFreeformContentHandlerName, setFreeformContentHandlerName, } from '@wordpress/blocks'; import { moment } from '@wordpress/date'; @@ -159,6 +158,20 @@ describe( 'selectors', () => { parent: [ 'core/test-block-b' ], } ); + registerBlockType( 'core/test-freeform', { + save: ( props ) => { props.attributes.content }, + category: 'common', + title: 'Test Freeform Content Handler', + icon: 'test', + attributes: { + content: { + type: 'string', + }, + }, + } ); + + setFreeformContentHandlerName( 'core/test-freeform' ); + cachedSelectors.forEach( ( { clear } ) => clear() ); } ); @@ -167,6 +180,9 @@ describe( 'selectors', () => { unregisterBlockType( 'core/test-block-a' ); unregisterBlockType( 'core/test-block-b' ); unregisterBlockType( 'core/test-block-c' ); + unregisterBlockType( 'core/test-freeform' ); + + setFreeformContentHandlerName( undefined ); } ); describe( 'hasEditorUndo', () => { @@ -1003,7 +1019,8 @@ describe( 'selectors', () => { byClientId: { 123: { clientId: 123, - name: 'core/test-block', + name: 'core/test-block-a', + isValid: true, attributes: { text: '', }, @@ -1027,15 +1044,19 @@ describe( 'selectors', () => { const state = { editor: { present: { - blocksByClientId: { - 123: { - clientId: 123, - name: 'core/freeform', - attributes: { }, + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + attributes: { + content: '', + }, + }, + }, + order: { + '': [ 123 ], }, - }, - blockOrder: { - '': [ 123 ], }, edits: {}, }, @@ -1051,15 +1072,20 @@ describe( 'selectors', () => { const state = { editor: { present: { - blocksByClientId: { - 123: { - clientId: 123, - name: 'core/freeform', - attributes: { }, + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: '', + }, + }, + }, + order: { + '': [ 123 ], }, - }, - blockOrder: { - '': [ 123 ], }, edits: {}, }, @@ -1252,7 +1278,8 @@ describe( 'selectors', () => { byClientId: { 123: { clientId: 123, - name: 'core/test-block', + name: 'core/test-block-a', + isValid: true, attributes: { text: '', }, @@ -1271,6 +1298,38 @@ describe( 'selectors', () => { expect( isEditedPostEmpty( state ) ).toBe( false ); } ); + it( 'should return true if blocks, but empty content edit', () => { + const state = { + editor: { + present: { + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-block-a', + isValid: true, + attributes: { + text: '', + }, + }, + }, + order: { + '': [ 123 ], + }, + }, + edits: { + content: '', + }, + }, + }, + currentPost: { + content: '', + }, + }; + + expect( isEditedPostEmpty( state ) ).toBe( true ); + } ); + it( 'should return true if the post has an empty content property', () => { const state = { editor: { @@ -1309,19 +1368,24 @@ describe( 'selectors', () => { expect( isEditedPostEmpty( state ) ).toBe( false ); } ); - it( 'should return true if empty attributes classic block', () => { + it( 'should return true if empty classic block', () => { const state = { editor: { present: { - blocksByClientId: { - 123: { - clientId: 123, - name: 'core/freeform', - attributes: { }, + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: '', + }, + }, + }, + order: { + '': [ 123 ], }, - }, - blockOrder: { - '': [ 123 ], }, edits: {}, }, @@ -1332,21 +1396,24 @@ describe( 'selectors', () => { expect( isEditedPostEmpty( state ) ).toBe( true ); } ); - it( 'should return true if empty content classic block', () => { + it( 'should return true if empty content freeform block', () => { const state = { editor: { present: { - blocksByClientId: { - 123: { - clientId: 123, - name: 'core/freeform', - attributes: { - content: '', + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: '', + }, }, }, - }, - blockOrder: { - '': [ 123 ], + order: { + '': [ 123 ], + }, }, edits: {}, }, @@ -1359,21 +1426,24 @@ describe( 'selectors', () => { expect( isEditedPostEmpty( state ) ).toBe( true ); } ); - it( 'should return false if non empty content classic block', () => { + it( 'should return false if non-empty content freeform block', () => { const state = { editor: { present: { - blocksByClientId: { - 123: { - clientId: 123, - name: 'core/freeform', - attributes: { - content: 'Test Data', + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: 'Test Data', + }, }, }, - }, - blockOrder: { - '': [ 123 ], + order: { + '': [ 123 ], + }, }, edits: {}, }, @@ -1385,6 +1455,44 @@ describe( 'selectors', () => { expect( isEditedPostEmpty( state ) ).toBe( false ); } ); + + it( 'should return false for multiple empty freeform blocks', () => { + const state = { + editor: { + present: { + blocks: { + byClientId: { + 123: { + clientId: 123, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: '', + }, + }, + 456: { + clientId: 456, + name: 'core/test-freeform', + isValid: true, + attributes: { + content: '', + }, + }, + }, + order: { + '': [ 123, 456 ], + }, + }, + edits: {}, + }, + }, + currentPost: { + content: '\n\n', + }, + }; + + expect( isEditedPostEmpty( state ) ).toBe( false ); + } ); } ); describe( 'isEditedPostBeingScheduled', () => { @@ -3410,11 +3518,10 @@ describe( 'selectors', () => { } ); describe( 'getEditedPostContent', () => { - let originalDefaultBlockName, originalFreeformContentHandlerName; + let originalDefaultBlockName; beforeAll( () => { originalDefaultBlockName = getDefaultBlockName(); - originalFreeformContentHandlerName = getFreeformContentHandlerName(); registerBlockType( 'core/default', { category: 'common', @@ -3427,23 +3534,11 @@ describe( 'selectors', () => { }, save: () => null, } ); - registerBlockType( 'core/unknown', { - category: 'common', - title: 'unknown', - attributes: { - html: { - type: 'string', - }, - }, - save: ( { attributes } ) => { attributes.html }, - } ); setDefaultBlockName( 'core/default' ); - setFreeformContentHandlerName( 'core/unknown' ); } ); afterAll( () => { setDefaultBlockName( originalDefaultBlockName ); - setFreeformContentHandlerName( originalFreeformContentHandlerName ); getBlockTypes().forEach( ( block ) => { unregisterBlockType( block.name ); } ); @@ -3502,8 +3597,8 @@ describe( 'selectors', () => { } ); it( 'returns removep\'d serialization of blocks for single unknown', () => { - const unknownBlock = createBlock( getFreeformContentHandlerName(), { - html: '

foo

', + const unknownBlock = createBlock( 'core/test-freeform', { + content: '

foo

', } ); const state = { editor: { @@ -3528,11 +3623,11 @@ describe( 'selectors', () => { } ); it( 'returns non-removep\'d serialization of blocks for multiple unknown', () => { - const firstUnknown = createBlock( getFreeformContentHandlerName(), { - html: '

foo

', + const firstUnknown = createBlock( 'core/test-freeform', { + content: '

foo

', } ); - const secondUnknown = createBlock( getFreeformContentHandlerName(), { - html: '

bar

', + const secondUnknown = createBlock( 'core/test-freeform', { + content: '

bar

', } ); const state = { editor: { @@ -3887,6 +3982,7 @@ describe( 'selectors', () => { 'core/block/2', 'core/block/1', 'core/test-block-b', + 'core/test-freeform', 'core/test-block-a', ] ); } ); @@ -3933,6 +4029,7 @@ describe( 'selectors', () => { expect( firstBlockFirstCall ).toBe( firstBlockSecondCall ); expect( firstBlockFirstCall.map( ( item ) => item.id ) ).toEqual( [ 'core/test-block-b', + 'core/test-freeform', 'core/test-block-a', 'core/block/1', 'core/block/2', @@ -3942,6 +4039,7 @@ describe( 'selectors', () => { const secondBlockSecondCall = getInserterItems( stateSecondBlockRestricted, 'block2' ); expect( secondBlockFirstCall.map( ( item ) => item.id ) ).toEqual( [ 'core/test-block-b', + 'core/test-freeform', 'core/test-block-a', 'core/block/1', 'core/block/2',