Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Block Editor: Fix 'isBlockSubtreeDisabled' private selector #54618

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Block Editor: Fix 'isBlockSubtreeDisabled' private selector #54618

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d5d020
Fix selector dependencies
Mamaduka Sep 19, 2023
37bb668
Improve child subtree check
Mamaduka Sep 19, 2023
440de7f
Add e2e tests
Mamaduka Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ export function getLastInsertedBlocksClientIds( state ) {
export const isBlockSubtreeDisabled = createSelector(
( state, clientId ) => {
const isChildSubtreeDisabled = ( childClientId ) => {
const mode = state.blockEditingModes.get( childClientId );
return (
( mode === undefined || mode === 'disabled' ) &&
getBlockEditingMode( state, childClientId ) === 'disabled' &&
Copy link
Member

@ramonjd ramonjd Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

getBlockOrder( state, childClientId ).every(
isChildSubtreeDisabled
)
Expand All @@ -58,7 +57,12 @@ export const isBlockSubtreeDisabled = createSelector(
getBlockOrder( state, clientId ).every( isChildSubtreeDisabled )
);
},
( state ) => [ state.blockEditingModes, state.blocks.parents ]
( state ) => [
state.blocks.parents,
state.blocks.order,
state.blockEditingModes,
state.blockListSettings,
]
);

/**
Expand Down
Loading