Check for allowed blocks recursively in patterns #30366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +638 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
david-szabo97
changed the title
ntsekouras
reviewed
Mar 30, 2021
There was a problem hiding this comment.
In general, besides the recursion for innerBlocks that wasn't taken into account this solution seems to align with my previous commit here: 48b703f.
This was only there because __experimentalGetParsedPattern is called in BlockPreview. The allowed should be checked with canInsertBlockType I think. The problem though is that canInsertBlockType uses information from the state, that the parsed patterns are not in :)
Finally in __experimentalGetAllowedPatterns we still check for top level blocks only. canInsertBlockType makes more checks besides the allowedBlockTypes, based on the provided rootClientId.
A bit confusing, no 😄
ntsekouras
reviewed
Mar 31, 2021
ntsekouras
reviewed
Apr 2, 2021
david-szabo97
added
the
[Type] Bug
david-szabo97
requested review from
jeyip,
mattwiebe,
vindl,
Addison-Stavlo,
Copons,
creativecoder and
youknowriad
david-szabo97
requested review from
ajitbohra,
ellatrix,
nerrad and
ntwb
as code owners
ntsekouras
reviewed
Apr 5, 2021
ntsekouras
reviewed
Apr 5, 2021
david-szabo97
force-pushed
the
fix/patterns-check-allowed-block-types-recursively
branch
from
c1ab1f6
to
22cd6f9
Compare
|
I have a question for @david-szabo97 and @ntsekouras: would it be helpful to distinguish “stage I” parsing from “stage II” parsing when it comes to deciding which patterns to display? Let me explain:
I am asking this for two reasons:
What are your thoughts? |
|
stage 2 gutenberg/packages/blocks/src/api/parser.js Line 467 in b88ae76
Because of this, we might end up with different blocks in stage 1 and stage 2. So if we use stage 1 blocks to do the check, then that might report a false positive. Is that correct? |
@david-szabo97: no, the difference should only be in the amount of information available for each block in the pattern. See the screenshot below. On the left is the arrangement of blocks we are dealing with. On the right, in the console, we use the serialised markup of those blocks and run it through the stage I parser and then the full parser. Notice how both represent the same tree structure, but compare the attributes for the Heading block: in the first output, These are the differences at play, along with knowledge of whether each block is considered valid, i.e. in conformance with its own definitions.
|
|
@mcsf stage 1 only: 42ms stage 1 + 2: 11329ms let startTime;
startTime = Date.now()
for (let i = 0; i < 10_000; i++) wp.blockSerializationDefaultParser.parse(markup + `<p>Stage 1 ${i}</p>`);
console.log(Date.now() - startTime) // 42
startTime = Date.now()
for (let i = 0; i < 10_000; i++) wp.blocks.parse(markup + `<p>Stage 1 + 2 ${i}</p>`);
console.log(Date.now() - startTime) // 11329 |
I am not too surprised, but it's a delight to see it. 😄 (cc @dmsnell just for fun — no input required). |
david-szabo97
commented
May 10, 2021
david-szabo97
force-pushed
the
fix/patterns-check-allowed-block-types-recursively
branch
from
d70daf8
to
854d9e9
Compare
|
Had to fix a few usages of Any smoke tests are appreciated.
|
ntsekouras
reviewed
May 13, 2021
There was a problem hiding this comment.
Thanks for your work here David and @mcsf for the great stage 1 input 😄 !
I've left a comment for a change regarding the BlockPatternSetup component, but other than that this looks great!
packages/block-editor/src/components/block-pattern-setup/index.js
Outdated
Show resolved
Hide resolved
david-szabo97
deleted the
fix/patterns-check-allowed-block-types-recursively
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #23275
Description
Currently, patterns show up in the inserter even if they contain a block that's not on the allowed blocks list. Blocks aren't checked recursively in the patterns, only top level blocks. Even though they show up, we can't insert them since the blocks are disallowed. This PR checks recursively for disallowed blocks.
A new (private) selector is introduced:
getAllAllowedPatterns. This intermediary function memoizes the available allowed patterns. This is used to avoid iterating over patterns that contain disallowed blocks and to avoid parsing all the patterns every time we call the__experimentalGetAllowedPatternswith a differentclientId.We use "Stage I" parser here for performance reasons. See
#30366 (comment)
#30366 (comment)
Why don't we use
canInsertBlockTypeinstead ofcheckAllowListRecursive?Recursively checking all blocks with
canInsertBlockTypeevery time we select a different block would have a huge performance impact. That's why we only check top level blocks withcanInsertBlockTypeand the nested ones withcheckAllowListRecursive.How has this been tested?
Types of changes
Bug Fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).