-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Fix controlled inner blocks parent block attributes updates #35827
Fix controlled inner blocks parent block attributes updates #35827
Conversation
Looking at the diff here, there are some changes that make me worry. Let me dig deeper into the issue. |
Size Change: +28 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
It should be almost exactly the same, the only difference is that if only the block wrapper had updates, that gets added to clientIds list now. My thinking is that this needs to bubble up to the parent entity (e.g. if the only the template part block wrapper is edited, that should still save to the template). |
fdc41eb
to
1242644
Compare
So I think what I've unearthed is that there are two separate bugs.
|
The failing tests indicate there are more issues with this fix too. @youknowriad any other ideas how to solve the problems? |
@@ -246,15 +246,20 @@ function updateParentInnerBlocksInTree( state, tree, updatedClientIds ) { | |||
for ( const clientId of updatedClientIds ) { | |||
let current = clientId; | |||
do { | |||
clientIds.add( current ); | |||
|
|||
if ( state.controlledInnerBlocks[ current ] ) { | |||
controlledParents.add( current ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we avoid this on the first level of the loop (when clientId is part of updatedClientIds
) to avoid marking its innerBlocks as dirty. which should resolve your second point maybe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just moving this line to the check line 256?
Both of your points 1 and 2 are logical to me. That said, I feel the implementation we have is probably too complex for nothing. I'll see if there's a way to make it a bit more simple. |
f108d94
to
3e8fa4d
Compare
Ok, I think what's confusing is that sometimes the function is called with the actual blocks that were modified (attributes), but sometimes it's called with their parents, so it's hard to reason about, I'm thinking we should add another argument to separate these two use-cases. |
I think I remove the ambiguity with the last update, let's see. |
Thanks for iterating on this. Still some failing tests I see. I'll try to give this a bit more testing on Monday to see what the issue is there. |
@@ -1980,7 +1980,7 @@ export const __experimentalGetPatternTransformItems = createSelector( | |||
if ( | |||
blocks.some( | |||
( { clientId, innerBlocks } ) => | |||
innerBlocks.length || | |||
innerBlocks?.length || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad This should hopefully resolve the failing tests. 🤞
It looks like with the recent change, some blocks in state now won't have the innerBlocks
property. I think the reason is that if a block only ever gets updated using the false
option it won't ever reach this line of code that adds the property (only the parents will):
https://github.com/WordPress/gutenberg/pull/35827/files#diff-3c4b969f7547d02bc04a2c554e0f7d21da07c3f779714acf02800ee0341b50d9R276-R280
The alternative fix would be to ensure the innerBlocks property is present, but that might involve changing the updateParentInnerBlocksInTree
again. Let me know what you think.
edit: also updated the failing unit test in 14bcacf, that had the same issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there's also this code that expects innerBlocks
to be defined, which is causing the remaining failing test:
gutenberg/packages/blocks/src/api/serializer.js
Lines 265 to 283 in d209b27
export function getBlockInnerHTML( block ) { | |
// If block was parsed as invalid or encounters an error while generating | |
// save content, use original content instead to avoid content loss. If a | |
// block contains nested content, exempt it from this condition because we | |
// otherwise have no access to its original content and content loss would | |
// still occur. | |
let saveContent = block.originalContent; | |
if ( block.isValid || block.innerBlocks.length ) { | |
try { | |
saveContent = getSaveContent( | |
block.name, | |
block.attributes, | |
block.innerBlocks | |
); | |
} catch ( error ) {} | |
} | |
return saveContent; | |
} |
Perhaps it's better to look at fixing the root cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, It seems we should consider adding an empty inner blocks in some init code somewhere to avoid that. Some third-party code could also rely on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad Did some more staring at this problem, and I think the cause is the UPDATE_BLOCK
case, which isn't passing in a valid tree to updateParentInnerBlocksInTree
. Hopefully the change in 04d1853 is ok.
We could also add some initialization code, but I don't think it's necessary right now.
14bcacf
to
04d1853
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. Thanks Dan :)
Thanks for your help with this one, glad we could get it sorted! |
Thanks for catching and looking into this |
Description
Fixes #35664
I bisected and found that the problem was introduced in #34241. I scanned the code and I think I spotted the issue:
It looks like the intent here was to traverse up and stop when reaching a block that has controlled inner blocks.
It has an unwanted side effect that if it's only the parent block (and not the controlled inner blocks) that changed the parent is also skipped from being added to the clientIds list.
Instead I think it needs to check the parent on each loop, and only traverse up if the parent isn't a controlled inne rblock
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).