Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block API: Add role for 'internal' attributes not copied on duplication #34750

Open
wants to merge 17 commits into
base: trunk
Choose a base branch
from
Open

Block API: Add role for 'internal' attributes not copied on duplication #34750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0d180f0
Add internal experimentalRole for attributes
stacimc Sep 10, 2021
17ab3e6
Add tests
stacimc Sep 10, 2021
563bbb9
Refactor removeAttributesByRole to take role param and return block
stacimc Sep 21, 2021
58e3490
Remove internal attrs when copying via keyboard command
stacimc Sep 21, 2021
d6666f2
Remove internal attrs when copying via CopyMenuItem
stacimc Sep 21, 2021
b311d38
Do not alter attributes of block being duplicated
stacimc Sep 22, 2021
0b0ad99
Use `__experimentalCloneSanitizedBlock` in copy handlers
stacimc Sep 22, 2021
677a0a0
Use internal attribute for widgetId
stacimc Sep 22, 2021
235ac7a
Update tests for __experimentalRemoveAttributesByRole
stacimc Sep 22, 2021
d15a118
Update packages/widgets/package.json
ntsekouras Sep 23, 2021
4088716
Move attribute registration to function called in widget initialization
stacimc Sep 23, 2021
3b0e6ef
Corrected docstring
stacimc Sep 23, 2021
0c4673b
Check for existence of widget before moving sidebars
stacimc Oct 11, 2021
cbe7951
Do not separately sanitize attributes in cloneSanitizedBlock
stacimc Oct 11, 2021
b31c072
Remove __experimentalSanitizeBlockAttributes
stacimc Oct 13, 2021
3601ff3
Remove unnecessary attribute sanitization in ServerSideRender component
stacimc Oct 13, 2021
2a1afa4
Remove removeAttributesByRole
stacimc Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import { useSelect } from '@wordpress/data';
import { moreVertical } from '@wordpress/icons';

import { Children, cloneElement, useCallback } from '@wordpress/element';
import { serialize, store as blocksStore } from '@wordpress/blocks';
import {
serialize,
store as blocksStore,
__experimentalCloneSanitizedBlock,
} from '@wordpress/blocks';
import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
import { useCopyToClipboard } from '@wordpress/compose';

Expand All @@ -33,7 +37,12 @@ const POPOVER_PROPS = {
};

function CopyMenuItem( { blocks, onCopy } ) {
const ref = useCopyToClipboard( () => serialize( blocks ), onCopy );
const ref = useCopyToClipboard( () => {
blocks = blocks.map( ( block ) =>
__experimentalCloneSanitizedBlock( block )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my first time looking at this code in a few months and, reading this, I struggled to grok what __experimentalCloneSanitizedBlock does. In particular I didn't really understand it's relationship to cloneBlock.

I think that a better API might be to add optional settings to cloneBlock.

cloneBlock( block, null, null, { includeInternalAttributes: false } )

This makes the parallels to cloneBlock very explicit. It's also clearer, I think, to avoid the word "sanitize" which could mean all sorts of things.

These settings could also exist on serialize which would address @gziolo's comment.

serialize( blocks, { includeInternalAttributes: false } )

(Not a blocking comment—we can absolutely iterate on the API later.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this suggestion, especially because it provides a better answer than I was able to give @gziolo re: serialization. And +1 for concerns on 'sanitize'; I struggled to come up with a better name and would be much happier to avoid it!

);
return serialize( blocks );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you explored whether we could move __experimentalCloneSanitizedBlock to the serialize method? I see a similar logic together in another file in this PR. The way it works now is fine but we will have to ensure that we use the same logic any time we add new functionality or what’s worse when plugins use the current public API. I don’t know if that’s possible but it would be a nice improvement for developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but I don't think we can move the logic up any higher. If we move __experimentalCloneSanitizeBlock into serialize, we would end up stripping out internal attributes on much more than duplication, every time the post is reloaded.

In this file and in the copy-handler, I think it makes sense that when you want to make a copy of a block you should explicitly clone it before serializing. There is some danger that this could be missed with new functionality that involves block duplication, but I can't think of a way of enforcing it.

}, onCopy );
return <MenuItem ref={ ref }>{ __( 'Copy' ) }</MenuItem>;
}

Expand Down
8 changes: 7 additions & 1 deletion packages/block-editor/src/components/copy-handler/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useCallback } from '@wordpress/element';
import {
serialize,
pasteHandler,
__experimentalCloneSanitizedBlock,
store as blocksStore,
} from '@wordpress/blocks';
import {
Expand Down Expand Up @@ -121,7 +122,12 @@ export function useClipboardHandler() {
flashBlock( selectedBlockClientIds[ 0 ] );
}
notifyCopy( event.type, selectedBlockClientIds );
const blocks = getBlocksByClientId( selectedBlockClientIds );
let blocks = getBlocksByClientId( selectedBlockClientIds );
if ( event.type === 'copy' ) {
blocks = blocks.map( ( block ) =>
__experimentalCloneSanitizedBlock( block )
);
}
const serialized = serialize( blocks );

event.clipboardData.setData( 'text/plain', serialized );
Expand Down
51 changes: 42 additions & 9 deletions packages/blocks/src/api/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
isFunction,
isEmpty,
map,
reduce,
omit,
} from 'lodash';

/**
Expand All @@ -32,7 +34,7 @@ import {
} from './registration';
import {
normalizeBlockType,
__experimentalSanitizeBlockAttributes,
__experimentalGetBlockAttributesNamesByRole,
} from './utils';

/**
Expand All @@ -45,9 +47,37 @@ import {
* @return {Object} Block object.
*/
export function createBlock( name, attributes = {}, innerBlocks = [] ) {
const sanitizedAttributes = __experimentalSanitizeBlockAttributes(
name,
attributes
// Get the type definition associated with a registered block.
const blockType = getBlockType( name );

if ( undefined === blockType ) {
throw new Error( `Block type '${ name }' is not registered.` );
}

const sanitizedAttributes = reduce(
blockType.attributes,
( accumulator, schema, key ) => {
const value = attributes[ key ];

if ( undefined !== value ) {
accumulator[ key ] = value;
} else if ( schema.hasOwnProperty( 'default' ) ) {
accumulator[ key ] = schema.default;
}

if ( [ 'node', 'children' ].indexOf( schema.source ) !== -1 ) {
// Ensure value passed is always an array, which we're expecting in
// the RichText component to handle the deprecated value.
if ( typeof accumulator[ key ] === 'string' ) {
accumulator[ key ] = [ accumulator[ key ] ];
} else if ( ! Array.isArray( accumulator[ key ] ) ) {
accumulator[ key ] = [];
}
}

return accumulator;
},
{}
);

const clientId = uuid();
Expand Down Expand Up @@ -94,8 +124,9 @@ export function createBlocksFromInnerBlocksTemplate(
}

/**
* Given a block object, returns a copy of the block object while sanitizing its attributes,
* Given a block object, returns a copy of the block object,
* optionally merging new attributes and/or replacing its inner blocks.
* Attributes with the `internal` role are not copied into the new object.
*
* @param {Object} block Block instance.
* @param {Object} mergeAttributes Block attributes.
Expand All @@ -110,18 +141,20 @@ export function __experimentalCloneSanitizedBlock(
) {
const clientId = uuid();

const sanitizedAttributes = __experimentalSanitizeBlockAttributes(
block.name,
// Merge in new attributes and strip out attributes with the `internal` role,
// which should not be copied during block duplication.
const filteredAttributes = omit(
{
...block.attributes,
...mergeAttributes,
}
},
__experimentalGetBlockAttributesNamesByRole( block.name, 'internal' )
);

return {
...block,
clientId,
attributes: sanitizedAttributes,
attributes: filteredAttributes,
innerBlocks:
newInnerBlocks ||
block.innerBlocks.map( ( innerBlock ) =>
Expand Down
1 change: 0 additions & 1 deletion packages/blocks/src/api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ export {
isValidIcon,
getBlockLabel as __experimentalGetBlockLabel,
getAccessibleBlockLabel as __experimentalGetAccessibleBlockLabel,
__experimentalSanitizeBlockAttributes,
__experimentalGetBlockAttributesNamesByRole,
} from './utils';

Expand Down
183 changes: 176 additions & 7 deletions packages/blocks/src/api/test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,21 @@ describe( 'block factory', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
attributes: {
content: {
childrenContent: {
type: 'array',
source: 'children',
},
nodeContent: {
source: 'node',
},
},
} );

const block = createBlock( 'core/test-block' );

expect( block.attributes ).toEqual( {
content: [],
childrenContent: [],
nodeContent: [],
} );
} );

Expand Down Expand Up @@ -177,6 +181,14 @@ describe( 'block factory', () => {

expect( block.attributes ).toEqual( {} );
} );

it( 'throws error if the block is not registered', () => {
expect( () => {
createBlock( 'core/not-registered-test-block', {} );
} ).toThrowErrorMatchingInlineSnapshot(
`"Block type 'core/not-registered-test-block' is not registered."`
);
} );
} );

describe( 'createBlocksFromInnerBlocksTemplate', () => {
Expand Down Expand Up @@ -428,25 +440,182 @@ describe( 'block factory', () => {
} );

describe( '__experimentalCloneSanitizedBlock', () => {
it( 'should sanitize attributes not defined in the block type', () => {
it( 'should not duplicate internal attributes', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
attributes: {
align: {
internalAttribute: {
type: 'string',
__experimentalRole: 'internal',
},
contentAttribute: {
type: 'string',
__experimentalRole: 'content',
},
attributeWithNoRole: {
type: 'string',
},
},
} );

const block = createBlock( 'core/test-block', {
notDefined: 'not-defined',
internalAttribute: 'this-should-not-be-copied',
contentAttribute: 'some content',
attributeWithNoRole: 'another attribute',
} );

const clonedBlock = __experimentalCloneSanitizedBlock( block, {} );

expect( clonedBlock.attributes ).toEqual( {
contentAttribute: 'some content',
attributeWithNoRole: 'another attribute',
} );
} );

it( 'should merge attributes into the existing block', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
isDifferent: {
type: 'boolean',
default: false,
},
includesDefault: {
type: 'boolean',
default: true,
},
includesFalseyDefault: {
type: 'number',
default: 0,
},
content: {
type: 'array',
source: 'children',
},
defaultContent: {
type: 'array',
source: 'children',
default: 'test',
},
unknownDefaultContent: {
type: 'array',
source: 'children',
default: 1,
},
htmlContent: {
source: 'html',
},
},
save: noop,
category: 'text',
title: 'test block',
} );
const block = deepFreeze(
createBlock( 'core/test-block', { align: 'left' }, [
createBlock( 'core/test-block' ),
] )
);

const clonedBlock = __experimentalCloneSanitizedBlock( block, {
notDefined2: 'not-defined-2',
isDifferent: true,
htmlContent: 'test',
} );

expect( clonedBlock.attributes ).toEqual( {} );
expect( clonedBlock.name ).toEqual( block.name );
expect( clonedBlock.attributes ).toEqual( {
includesDefault: true,
includesFalseyDefault: 0,
align: 'left',
isDifferent: true,
content: [],
defaultContent: [ 'test' ],
unknownDefaultContent: [],
htmlContent: 'test',
} );
expect( clonedBlock.innerBlocks ).toHaveLength( 1 );
expect( typeof clonedBlock.clientId ).toBe( 'string' );
expect( clonedBlock.clientId ).not.toBe( block.clientId );
} );

it( 'should replace inner blocks of the existing block', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
isDifferent: {
type: 'boolean',
default: false,
},
},
save: noop,
category: 'text',
title: 'test block',
} );
const block = deepFreeze(
createBlock( 'core/test-block', { align: 'left' }, [
createBlock( 'core/test-block', { align: 'right' } ),
createBlock( 'core/test-block', { align: 'left' } ),
] )
);

const clonedBlock = __experimentalCloneSanitizedBlock(
block,
undefined,
[ createBlock( 'core/test-block' ) ]
);

expect( clonedBlock.innerBlocks ).toHaveLength( 1 );
expect(
clonedBlock.innerBlocks[ 0 ].attributes
).not.toHaveProperty( 'align' );
} );

it( 'should clone innerBlocks if innerBlocks are not passed', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
isDifferent: {
type: 'boolean',
default: false,
},
},
save: noop,
category: 'text',
title: 'test block',
} );
const block = deepFreeze(
createBlock( 'core/test-block', { align: 'left' }, [
createBlock( 'core/test-block', { align: 'right' } ),
createBlock( 'core/test-block', { align: 'left' } ),
] )
);

const clonedBlock = __experimentalCloneSanitizedBlock( block );

expect( clonedBlock.innerBlocks ).toHaveLength( 2 );
expect( clonedBlock.innerBlocks[ 0 ].clientId ).not.toBe(
block.innerBlocks[ 0 ].clientId
);
expect( clonedBlock.innerBlocks[ 0 ].attributes ).not.toBe(
block.innerBlocks[ 0 ].attributes
);
expect( clonedBlock.innerBlocks[ 0 ].attributes ).toEqual(
block.innerBlocks[ 0 ].attributes
);
expect( clonedBlock.innerBlocks[ 1 ].clientId ).not.toBe(
block.innerBlocks[ 1 ].clientId
);
expect( clonedBlock.innerBlocks[ 1 ].attributes ).not.toBe(
block.innerBlocks[ 1 ].attributes
);
expect( clonedBlock.innerBlocks[ 1 ].attributes ).toEqual(
block.innerBlocks[ 1 ].attributes
);
} );
} );

Expand Down
Loading