Inserter: take into account children to disable blocks #8075

oandregal · 2018-07-20T10:59:16Z

When evaluating whether a block should be disabled or not, the inserter should take into account children of nested blocks ~~as well as the blocks referenced by the existing shared blocks, as well as the~~ - see #8075 (comment).

Testing

After this PR, it shouldn't be possible to add a More block if there is one nested More block:

jorgefilipecosta · 2018-07-20T19:45:12Z

editor/store/selectors.js

@@ -1539,7 +1575,7 @@ export const getInserterItems = createSelector(

 			let isDisabled = false;
 			if ( ! hasBlockSupport( blockType.name, 'multiple', true ) ) {
-				isDisabled = some( getBlocks( state ), { name: blockType.name } );
+				isDisabled = some( getBlocksUnfolded( state ), { name: blockType.name } );


Hi @nosolosw, the data structures we use on state are flat maybe we can take advantage of that. In state.editor.present.blocksByClientId we have an object with all the blocks including the nested ones. So we can just search for a block with that name on the list. Unfortunately, I think we have blocks that are previews so to make sure the block exists on the document if we find something we need to check if the UID is present in state.editor.present.blockOrder. This approach would take advantage of our already flat structures and avoid the new to create new arrays just to make a validation. Do you think this would work?

If we are storing blocks that are not part of the document in state.editor.present.blocksByClientId we should at least flag them with something like is preview blocks so we could make this check with just a search in an array. And plugin developers would be able to distinguish between blocks part of the document and blocks that are not part of the document.

I wish we could do that, but in my testing, I've found that editor.present.blockOrder and editor.present.blocksByUID end up containing blocks that are not present in the document, so we can't operate solely on those. The only reliable mechanism I've found is to build the block tree from the blockOrder[''].

In ae57038 I've modified the external API to expose the getBlockIdsUnfolded selectors instead of getBlocksUnfolded.

That way the API has only two ways to query blocks:

getBlocks for top-level blocks

getBlocksByUID for retrieving any kind of blocks by IDs

which is arguably simpler to understand, but still exposes a helper selector (getBlockIdsUnfolded) to query all the blocks present, including referenced by shared and children of nested.

If we find a block in blocksByUID, can we check if the block is, in fact, present in the document by checking if the block is referenced by some block or the root block in state.editor.present.blockOrder?

If that's not the case I feel plugin developers will also not be able to know what blocks are present in the document. So probably we should add some flags to specify if a block is a preview block or not (in a separate PR).

If we find a block in blocksByUID, can we check if the block is, in fact, present in the document by checking if the block is referenced by some block or the root block in state.editor.present.blockOrder?

Yes, that works. This is what I mean by "building the block tree from the blockOrder['']".

Note that if you want to check if a block is referenced by a shared block that wouldn't work because the relationship is encoded through the ref attribute in the block contents.

If that's not the case I feel plugin developers will also not be able to know what blocks are present in the document. So probably we should add some flags to specify if a block is a preview block or not (in a separate PR).

What's a preview block?

My idea would be to a check if a block is part of the document, by checking if the block is referenced by any other block in blockOrder or by blockOrder[''] without needing to iterate on the tree just checking the array.

forEach( blockOrder as blockUids) { if( blockUIs contains blockUid ) { //block uid is part of the document. } }

Do you think that this would work?

Edited: this would not work. We may have nested shared blocks not inserterd (as previews), and blocks inside the nested shared block would pass this validation.

What's a preview block?

I think the blocks present in blocksByUID that are not part of the document are used to display block previews. Maybe we can flag this blocks so just by iterating on blocksByUID we can know what blocks are part of the document or not.
I just discovered a bug related with this problem if we have shared blocks, we create an empty post we preview shared blocks on the inserter without previewing them our block counts are wrong because the counter thinks all blocks in blocksByUID are part of the document.

Summing up here some bits that Jorge and I discussed privately:

preview blocks are the ones displayed in the inserter when you hover over the shared blocks. See:

shared blocks (although not used) are appended to blocksByClientId.

blocksByClientId is updated every time any block attribute changes (like in any key press for paragraphs). The cache of any selector that is dependant on it will be invalidated every time it changes, rendering the cache unuseful.

can the getBlockIdsUnfolded selector only depend on blockOrder? No. Some plugin may update the blocksByClientId by changing the ref attribute of a shared block present in the document. That change wouldn't be reflected in blockOrder, and the selector uses the ref to compute the referenced blocks.

the outstanding blocker for merging this would be performance. We don't know the impact of this for real, but we are operating on this assumption: avoid array concatenation (prefer flatMap).

The selector has been refactored to use flatMap.

jorgefilipecosta · 2018-07-24T09:40:02Z

editor/store/selectors.js

+const unfoldBlockIds = ( state, block ) => {
+	const clientIds = [];
+	if ( block.name === 'core/block' ) {
+		clientIds.push( get( getSharedBlock( state, block.attributes.ref ), [ 'clientId' ], null ) );


I think we may need to apply an unfoldOperation to the result of getSharedBlock because that block may have other blocks nested inside.

Case and test added in 090209cb442966f30823a74c9c7416a9a004081a

Good catch!

jorgefilipecosta · 2018-07-24T09:41:18Z

editor/store/selectors.js

+ *
+ * @return {Array} All ids of top-level, referenced, and inner blocks.
+ */
+export const getBlockIdsUnfolded = createSelector(


Maybe we can call this select getAllBlockClientIdsReferenced and specify it returns the array of all block uids referenced/used in the document even the ones inside shared blocks.

Renamed to getClientIdsUnfolded which is in line with the other selector names (get + thing + modifier).

oandregal · 2018-07-26T14:28:06Z

@jorgefilipecosta I've addressed your feedback to date. Would you consider this mergeable?

oandregal · 2018-07-26T17:14:18Z

@noisysocks some e2e test related to SharedBlocks are failing for this PR https://travis-ci.org/WordPress/gutenberg/jobs/408598718 It's a bit obscure to me as to why they fail, would you be available to help?

noisysocks · 2018-07-27T01:48:20Z

@nosolosw: It looks like there's an exception being thrown when a new shared block is created.

Uncaught TypeError: Cannot read property 'name' of null
    at getClientIdsFromSharedBlock (selectors.js:579)
    at unfoldClientIds (selectors.js:593)
    at getClientIdsFromSharedBlock (selectors.js:583)
    at unfoldClientIds (selectors.js:593)
    at getClientIdsFromBlock (selectors.js:610)
    at arrayMap (lodash.2cfc2504.js:631)
    at map (lodash.2cfc2504.js:9546)
    at flatMap (lodash.2cfc2504.js:9249)
    at selectors.js:611
    at callSelector (rememo.js:248)

oandregal · 2018-07-27T08:27:44Z

Thanks Robert! Fixed at 90ff826.

jorgefilipecosta · 2018-07-27T18:10:43Z

editor/store/selectors.js

+				...unfoldClientIds( globalState, ...getBlocksByClientId( globalState, clientId ) ),
+			];
+		}
+		return [ null ];


If we just return [] here we avoid the need for the filter by id right?

I don't think so: in https://github.com/WordPress/gutenberg/pull/8075/files/90ff8269cf5999dd605cb71a51251846e3354efc#diff-4e1dd4776f75eb7a66b28280a56d8e0aR594 and https://github.com/WordPress/gutenberg/pull/8075/files/90ff8269cf5999dd605cb71a51251846e3354efc#diff-4e1dd4776f75eb7a66b28280a56d8e0aR585 we return null if the shared or inner block doesn't have a clientId. That's certainly unlikely, but I'd rather be cautious.

Thank you for clarigying, I missed that the "block" it self may be null because of the return of a selector.

jorgefilipecosta · 2018-07-27T18:16:08Z

I'm noticing that if for example, we create a shared block for more. We can insert that shared block multiple times. This verification for shared blocks is really hard to do as shared blocks can contain nested blocks inside and if one of the many nested blocks inside does not support multiple blocks and is present in the document (maybe via another shared block) the insertion cannot happen. Maybe given this complexity it may make sense to not unfold shared bocks and ignore them from this verifications.

oandregal · 2018-08-02T11:45:14Z

Now that I've been more exposed to Gutenberg, I think we should implement this in a coherent way: either take SharedBlocks into account or not take them (I was initially wary of taking the use case @jorgefilipecosta mentioned into account).

I lean towards adding them to this computation (and I can add more code to this PR to fix that), but I'm also open to remove them from this. Being a guten-newbie, I'm unsure about the best path here, so I'll invoke @noisysocks as I understand he's the most knowledgeable person about how SharedBlocks interact with the rest of Gutenberg - what are your thoughts, Robert?

noisysocks · 2018-08-06T00:26:49Z

This is a tough one...

With how reusable blocks are currently implemented, I'd lean towards taking them into account when computing whether or not a block is insertable because of a useOnce.¹

BUT, in #7453, we're considering changing our implementation of reusable blocks to use a nested instance of the Gutenberg editor. This would make iterating through "all of the blocks on the page" very difficult as it would involve iterating through different wp.data registries.

I think that, for now, let's not take reusable blocks into account. We can revaluate this later when #7453 is resolved.

¹ Note, however, that not everything in blocksByClientId is inserted into the page. Reusable blocks that have been loaded from the API but not yet used are stored in this object so that the inserter can display a preview of the block when you hover over it.

When deriving whether a block should be disabled or not, the inserter should take into account the blocks referenced by the existing top-level shared blocks.

By doing this, we reduce the number of API methods to retrieve blocks to: - getBlocks: top level blocks - getBlocksByClientId: blocks by id And we expose getBlockIdsUnfolded as an utility to retrieve the ids of top-level, referenced, and nested blocks.

A shared block can contain nested blocks, so we need to unfold them.

oandregal · 2018-08-06T07:51:14Z

Thanks for the feedback, Jorge & Robert. I'll shrink down the scope of this to Nested blocks, leaving Shared for a future iteration.

oandregal · 2018-08-06T09:25:02Z

@jorgefilipecosta @noisysocks da45c30 has got rid of the logic that was specific to Shared blocks. Updated testing instructions and PR description as well.

oandregal · 2018-08-13T09:28:00Z

@jorgefilipecosta @noisysocks does this need anything else to be ready to merge? Or can I go ahead and land this?

noisysocks

LGTM—thanks @nosolosw 👍

oandregal requested review from gziolo and noisysocks July 20, 2018 11:27

oandregal changed the title ~~Inserter: take into account referenced blocks~~ Multiple support: take into account referenced and children blocks Jul 20, 2018

oandregal changed the title ~~Multiple support: take into account referenced and children blocks~~ Inserter: take into account referenced and children to disable blocks Jul 20, 2018

jorgefilipecosta reviewed Jul 20, 2018

View reviewed changes

oandregal force-pushed the fix/multiple-support branch from 052827c to ae57038 Compare July 23, 2018 14:33

jorgefilipecosta reviewed Jul 24, 2018

View reviewed changes

oandregal force-pushed the fix/multiple-support branch from ae57038 to bbee93a Compare July 26, 2018 12:29

oandregal force-pushed the fix/multiple-support branch 2 times, most recently from 47a6d28 to 72b305b Compare July 26, 2018 16:49

oandregal force-pushed the fix/multiple-support branch from 72b305b to 90ff826 Compare July 27, 2018 08:27

jorgefilipecosta reviewed Jul 27, 2018

View reviewed changes

oandregal force-pushed the fix/multiple-support branch from 90ff826 to 707fc7a Compare July 31, 2018 10:27

oandregal self-assigned this Aug 2, 2018

oandregal added 8 commits August 6, 2018 09:39

Refs #7957 - Inserter: take into account referenced blocks

177cb4d

When deriving whether a block should be disabled or not, the inserter should take into account the blocks referenced by the existing top-level shared blocks.

Add test for getBlocksTopLevelAndReferenced selector

9f08042

Test that nested blocks are unfolded

9c2b1b2

Take into account also children of nested blocks

55bc814

Rename

14490ac

Refactor getClientIdsUnfolded to use flatMap

4c00311

Refactor unfoldClients to use flatMap

c6a2b57

oandregal added 3 commits August 6, 2018 09:39

Extract getClientIdsFromSharedBlock

3199792

Unfold also shared blocks

2c52fb7

A shared block can contain nested blocks, so we need to unfold them.

Bail early if no proper block

aefead7

oandregal changed the title ~~Inserter: take into account referenced and children to disable blocks~~ Inserter: take into account children to disable blocks Aug 6, 2018

Get rid of SharedBlocks logic

da45c30

oandregal force-pushed the fix/multiple-support branch from 707fc7a to da45c30 Compare August 6, 2018 09:03

noisysocks approved these changes Aug 14, 2018

View reviewed changes

oandregal merged commit 8dd93db into master Aug 14, 2018

oandregal deleted the fix/multiple-support branch August 14, 2018 07:37

oandregal mentioned this pull request Aug 14, 2018

multiple:false support is not honored for shared and nested blocks #7957

Closed

mtias added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Aug 16, 2018

mtias added this to the 3.6 milestone Aug 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inserter: take into account children to disable blocks #8075

Inserter: take into account children to disable blocks #8075

oandregal commented Jul 20, 2018 •

edited

Loading

jorgefilipecosta Jul 20, 2018

oandregal Jul 23, 2018

oandregal Jul 23, 2018 •

edited

Loading

jorgefilipecosta Jul 23, 2018

oandregal Jul 23, 2018

jorgefilipecosta Jul 23, 2018 •

edited

Loading

oandregal Jul 24, 2018 •

edited

Loading

oandregal Jul 26, 2018

jorgefilipecosta Jul 24, 2018

oandregal Jul 26, 2018

jorgefilipecosta Jul 24, 2018

oandregal Jul 26, 2018

oandregal commented Jul 26, 2018

oandregal commented Jul 26, 2018

noisysocks commented Jul 27, 2018

oandregal commented Jul 27, 2018

jorgefilipecosta Jul 27, 2018

oandregal Jul 31, 2018

jorgefilipecosta Aug 1, 2018

jorgefilipecosta commented Jul 27, 2018 •

edited

Loading

oandregal commented Aug 2, 2018

noisysocks commented Aug 6, 2018

oandregal commented Aug 6, 2018

oandregal commented Aug 6, 2018

oandregal commented Aug 13, 2018

noisysocks left a comment

Inserter: take into account children to disable blocks #8075

Inserter: take into account children to disable blocks #8075

Conversation

oandregal commented Jul 20, 2018 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Jul 26, 2018

oandregal commented Jul 26, 2018

noisysocks commented Jul 27, 2018

oandregal commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jul 27, 2018 • edited Loading

oandregal commented Aug 2, 2018

noisysocks commented Aug 6, 2018

oandregal commented Aug 6, 2018

oandregal commented Aug 6, 2018

oandregal commented Aug 13, 2018

noisysocks left a comment

Choose a reason for hiding this comment

oandregal commented Jul 20, 2018 •

edited

Loading

oandregal Jul 23, 2018 •

edited

Loading

jorgefilipecosta Jul 23, 2018 •

edited

Loading

oandregal Jul 24, 2018 •

edited

Loading

jorgefilipecosta commented Jul 27, 2018 •

edited

Loading