Block Editor: Fix 'isBlockSubtreeDisabled' private selector #54618
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
Mamaduka
added
[Type] Bug
|
Size Change: +49 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
ramonjd
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
Thanks for catching this @Mamaduka
Before
I can't edit the content inside a contentOnly locked template!
2023-09-20.12.07.10.mp4
After
Now I can edit the content inside a contentOnly locked template!
2023-09-20.12.07.24.mp4
Just left a question about the changed condition inside the selector.
| return ( | ||
| ( mode === undefined || mode === 'disabled' ) && | ||
| getBlockEditingMode( state, childClientId ) === 'disabled' && |
There was a problem hiding this comment.
Just checking, is the mode === undefined check not required anymore?
getBlockEditingMode() can still try to fetch from the blockEditingModes Map: state.blockEditingModes.get( childClientId ) which will return undefined if the key returns no value.
Just by reading the previous check, it seems to say, "if the parent's mode is not yet known, or has never been set, flag the tree as disabled if all the children are disabled"
Then again, I went through the test description of the PR that introduced the selector, and things still work as expected.
What do you think?
There was a problem hiding this comment.
I read through all PRs that touched this code and couldn't find the exact reason why the first check used the getBlockEditingMode selector and the child subtree did not.
Just by reading the previous check, it seems to say, "if the parent's mode is not yet known, or has never been set, flag the tree as disabled if all the children are disabled"
This was causing false positives in the case. The paragraph mode is undefined, but the check for its children will also resolve to true because [].every( condition ) === true.
There was a problem hiding this comment.
This was causing false positives in the case. The paragraph mode is undefined, but the check for its children will also resolve to true because [].every( condition ) === true.
Ah okay. I just wasn't sure whether a block tree whose children are disabled but whose parent isn't could be classed as "disabled". The function name implies it, but as you say there'd have to be further checks. If it's not being used this way, then it seems safe.
Thanks for confirming!
Mamaduka
added
the
Backport to Gutenberg RC
mikachan
pushed a commit
that referenced
this pull request
Sep 22, 2023
* Fix selector dependencies * Improve child subtree check * Add e2e tests
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 9b9931f |
mikachan
removed
the
Backport to Gutenberg RC
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 #54561.
PR fixes a bug with the
isBlockSubtreeDisabledprivate selector incorrectly marking deeply nested blocks in thecontentOnlytemplate locked parent as non-editable.Why?
The selector had two issues:
getBlockEditingModechecks blocks for attributes with content roles.How?
getBlockEditingModefor the child block checks.Testing Instructions
Example content
Testing Instructions for Keyboard
Same
Screenshots or screencast
CleanShot.2023-09-19.at.21.42.08.mp4