[Block Editor]: Fix content loss from replaceInnerBlocks with controlled blocks
#41948
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
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
mcsf,
talldan,
noahtallen,
tellthemachines,
jorgefilipecosta and
a team
mtias
reviewed
Jun 24, 2022
|
Size Change: +247 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
tellthemachines
approved these changes
Jun 27, 2022
There was a problem hiding this comment.
Thanks for the prompt fix! Testing with the original reproduction steps everything now works as expected. I also tested adding/removing columns from a Columns block in a regular post, with both a Navigation block and a reusable block inside, and no content is lost. (On trunk, the same flow results in the content of both the Navigation and reusable block disappearing.)
Offhand I can't think how updating the state tree here might cause any harm, but I don't know this part of the code in-depth, so you might want to double check that 😅
talldan
reviewed
Jun 27, 2022
There was a problem hiding this comment.
Great work figuring this out. In trunk the tree is definitely computed incorrectly, and in this branch I couldn't find a situation where it's incorrect.
I had quite a deep dive into the code to try to understand it. In terms of the solution, I find it difficult to say whether the controlled block should be reattached to the tree as it is in this PR, or whether it shouldn't be removed in the first place. To explain what I mean, as part of replacing the inner blocks, the blocks are first removed and then reinserted. There's a keepControlledInnerBlocks property of the REMOVE_BLOCKS action:
gutenberg/packages/block-editor/src/store/reducer.js
Lines 678 to 682 in f6f0cea
But the withBlockTree higher order reducer doesn't seem to respect that prop and removes those clientIds anyway:
gutenberg/packages/block-editor/src/store/reducer.js
Lines 383 to 395 in f6f0cea
An alternative solution might be just to keep them in the root, but I don't know for sure—maybe that would have some incorrect consequences.
Whatever the solution, it'd be good to add some unit tests to this PR if it's not too difficult.
|
Thanks for the reviews!
That is something I wasn't sure as well, but all these flows are interconnected with multiple actions and also events in I will see to write some tests.. |
mcsf
approved these changes
Jun 27, 2022
ntsekouras
deleted the
fix/content-loss-on-replace-inner-blocks-with-controlled-entities
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.
What?
Fixes: #41887
When we dispatch
REPLACE_INNER_BLOCKSaction we have a special handling which consists of two steps:In this flow(
withReplaceInnerBlocks) we have some special handling for controlled blocks(reusable, postContent, etc..) where we keep them in state during the removal(keepControlledInnerBlocks) and we are updating theorderagain after insertion.What we are missing is the update of
block tree. I think the only reason we missed that until now is because in our code base we have just a few calls toreplaceInnerBlocks, where most of them(if not all) aren't affected by this bug - example use cases would be replacing blocks with entirely new blocks or replacing blocks that only specific blocks are allowed(non controlled blocks).Testing Instructions(also can be seen in the video)
editthe templatePostContenttoColumnsand add a new columns(this is where the call toreplaceInnerBlockshappens).Before
before.mov
After
after.mov
Notes
remove blocks and replace blockswith have similar handling in a different flow(REPLACE_BLOCKS_AUGMENTED_WITH_CHILDREN,REMOVE_BLOCKS_AUGMENTED_WITH_CHILDREN)controlledblocks are the blocks that are a separate entity likereusable blocks, template parts, etc...