The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: alecgeatches <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

I think this is probably not the right fix. The idea here is that the "block-editor" store was not built originally to support rendering the same block twice. So without the cloning of UID which tricks the block-editor store to think that these blocks are different, we had bugs when rendering the same "template part" or same "synced pattern" twice in a page.

It is also a use-case that can happen with postContent blocks, for instance when you have multiple query blocks in a page with the same posts rendered on each query...

So I think for this change we need to thread very carefully. I'm actually a bit surprised that no e2e test failed, cause I'm pretty sure we had this covered (maybe that e2e test has been skipped or something)

I think we should test this a bit more, I'd love to be able to remove the cloning, but I feel like it requires more important changes to the block-editor store and its selectors.

To clarify, like what does it mean to "select a block" using a clientUID if that block is present twice in the block tree, which block are you going to highlight, where to show the block toolbar. This is just an example, the same question can be asked for any selection/action that relies on clientUID in the block editor.

@youknowriad Thank you for the feedback on this change! I assumed there was purpose to the original cloning, and you gave me a good place to start with patterns. More detail below, but in short: I was able to reproduce the issues you mentioned if I changed the code to never runcloneBlocks(), but with the scoped core/post-content changes in this PR I wasn't able to.

Reproducing block cloning issues with synced patterns

Here's how to reproduce render issues with synced patterns if cloneBlock() is disabled entirely:

1. First, create a pattern under WP Admin -> Appearance -> Editor -> Patterns sidebar -> "Add Pattern" button.

   I created a synced pattern called "My Pattern":

   

2. Next, I created a post with my pattern twice:

   

3. Next, add this code change to packages/block-editor/src/components/provider/use-block-sync.js to disable all block cloning:

   - const storeBlocks = isPostContentBlock
   -     ? controlledBlocks
   -     : controlledBlocks.map( ( block ) => cloneBlock( block ) );
   +
   + const storeBlocks = controlledBlocks;

4. Loading my page with 2x"My Pattern" causes only one of the blocks to render correctly:

   broken-clone.mov

   Only the second pattern renders.

5. This is also reproducible if I add the same pattern in the surrounding template. Below, I've added a new template that also has the pattern twice within it:

   

6. Let's try loading the prior post with 2x"My Pattern", also using the template with "Show Template" mode. This renders the pattern a total of 4 times:

   

   As before, only one of these patterns renders when we disable cloneBlocks(). The rest fail to render.

However

So far these issues can be recreated by completely disabling block cloning. If we switch back to this PR's code that does not clone blocks scoped within post-content blocks, the 4x"My Pattern" page above renders fine:

I can also confirm via DOM that each of these pattern blocks are still assigned an unique UUID.

This problem doesn't seem to be reproducible with the limited scope of the changes here, only if we remove cloning blocks altogether. I can understand the potential problem, but I can not reproduce it within this PR.

Follow-up questions

1. Are there any other test cases like patterns I should also test? I was unable to reproduce any issues with repeated query loop blocks.
2. If you're still feeling iffy about this PR, can you recommend a place I should focus? Do we want the goal that we shouldn't have to clone any blocks?

@alecgeatches Did you try the same steps you did here for the "pattern block" buy inserting the "post content" block twice within the template editor (in the site editor)?

So I'm having trouble reproducing the issue myself. In theory the steps would be:

• Create a template, and add the "post content" block twice to that template.
• Open a page/post, add title and content, save and then go to the template panel in the sidebar,
• Trigger "show template"
• try to assign the "template with two post contents" to that post

In theory, this should be showing the post with template in question, so the post content should be showing twice.
What's happening right now is very weird:

• The template selector disappears
• The show template toggle disappears
• It feels like the template is not assigned at all to the post.

I think there's probably an unrelated bug here (unrelated to the current PR), but if that bug didn't exist and we were able to assign that template properly, I bet we'll see a very similar issue to the "pattern" block. (one or both of the post content block would appear empty)

I'm honestly not sure what's the path forward here.

@youknowriad That reproduces it! If I create two post content blocks:

and then use "Show Template" mode, typing in either post content jumps around and doesn't work correctly:

Blocks in post content that have the same UUID cause the issue you suspected.

I can think of some possible next steps:

1. Change our plugin to not rely on client UUIDs, and instead use block indexes. This comes with some potential trade-offs and temporary off-by-one inaccuracies in awareness state when blocks are added or removed. With UUIDs we just don't draw a cursor in a block that hasn't been received, but that wouldn't be the case with an index.

2. Change the block store to no longer require unique IDs. This sounds like a lot of work, and comes with the questions you asked above like "where to show the block toolbar" when a duplicated block is selected.

3. A third option?

I'll play around some more in the code to see if there's another way we can do this, but I'm happy to hear if you have any other ideas.

Change our plugin to not rely on client UUIDs, and instead use block indexes. This comes with some potential trade-offs and temporary off-by-one inaccuracies in awareness state when blocks are added or removed. With UUIDs we just don't draw a cursor in a block that hasn't been received, but that wouldn't be the case with an index.

Why do the uuids used within the block-editor store matter? Maybe these are temporary uids, and the cloning persists but it also can persist in both ways. In other words when onChange (or onInput is called), useBlockSync would actually restore the original uids when calling the onChange and the "cloned" ones are only internal to block-editor store. In other words, we'd need to keep two "uids" per block in the block-editor store or have a map of uids (external to internal) in useBlockSync. I'm not certain which issues this could cause (I think there are probably issues related to immutability as we rely on immutability of blocks in a few places), but maybe it's a decent approach that we can try.

The suggestion I made above could potentially lead to a small performance impact (mapping through blocks multiple times), worth exploring but there could be solutions where we keep two block trees in the block-editor store (once with cloned uids and one with the real ones) to avoid the mapping or something. Anyway, these are just crazy ideas :)

Why do the uuids used within the block-editor store matter? Maybe these are temporary uids, and the cloning persists but it also can persist in both ways. In other words when onChange (or onInput is called), useBlockSync would actually restore the original uids when calling the onChange and the "cloned" ones are only internal to block-editor store. In other words, we'd need to keep two "uids" per block in the block-editor store or have a map of uids (external to internal) in useBlockSync. I'm not certain which issues this could cause (I think there are probably issues related to immutability as we rely on immutability of blocks in a few places), but maybe it's a decent approach that we can try.

@youknowriad Thank you for the response and idea above! I just wanted to let you know I haven't forgotten about this PR, but I have been busy working on some other undo and diffing changes since your comment. I'll get back to this soon and explore how this could be implemented.