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

Inserter: Fix Block visibility manager #65700

Merged
merged 5 commits into from
Oct 2, 2024
Merged

Inserter: Fix Block visibility manager #65700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
deb6a90
Inserter: Fix Block visibility manager
youknowriad Sep 27, 2024
c15e396
Handle child blocks properly
youknowriad Sep 27, 2024
2efecbd
Reintroduce core/post-content exception
youknowriad Sep 30, 2024
5ea69f4
Restore patterns e2e test
youknowriad Sep 30, 2024
5fd6fb9
Fix typo
youknowriad Oct 1, 2024
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
42 changes: 14 additions & 28 deletions packages/block-editor/src/components/inserter/hooks/use-patterns-state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { store as noticesStore } from '@wordpress/notices';
import { store as blockEditorStore } from '../../../store';
import { unlock } from '../../../lock-unlock';
import { INSERTER_PATTERN_TYPES } from '../block-patterns-tab/utils';
import { getParsedPattern } from '../../../store/utils';
import { isFiltered } from '../../../store/utils';

/**
* Retrieves the block patterns inserter state.
Expand All @@ -31,48 +31,34 @@ const usePatternsState = (
selectedCategory,
isQuick
) => {
const { patternCategories, allPatterns, userPatternCategories } = useSelect(
const options = useMemo(
() => ( { [ isFiltered ]: !! isQuick } ),
[ isQuick ]
);
const { patternCategories, patterns, userPatternCategories } = useSelect(
( select ) => {
const {
getAllPatterns,
getSettings,
__experimentalGetAllowedPatterns,
} = unlock( select( blockEditorStore ) );
const { getSettings, __experimentalGetAllowedPatterns } = unlock(
select( blockEditorStore )
);
const {
__experimentalUserPatternCategories,
__experimentalBlockPatternCategories,
} = getSettings();
return {
allPatterns: isQuick
? __experimentalGetAllowedPatterns()
: getAllPatterns(),
patterns: __experimentalGetAllowedPatterns(
rootClientId,
options
),
userPatternCategories: __experimentalUserPatternCategories,
patternCategories: __experimentalBlockPatternCategories,
};
},
[ isQuick ]
[ rootClientId, options ]
);
const { getClosestAllowedInsertionPointForPattern } = unlock(
useSelect( blockEditorStore )
);

const patterns = useMemo(
() =>
isQuick
? allPatterns
: allPatterns
.filter( ( { inserter = true } ) => !! inserter )
.map( ( pattern ) => {
return {
...pattern,
get blocks() {
return getParsedPattern( pattern ).blocks;
},
};
} ),
[ isQuick, allPatterns ]
);

const allCategories = useMemo( () => {
const categories = [ ...patternCategories ];
userPatternCategories?.forEach( ( userCategory ) => {
Expand Down
155 changes: 107 additions & 48 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,6 +1540,59 @@ export function getTemplateLock( state, rootClientId ) {
return getBlockListSettings( state, rootClientId )?.templateLock ?? false;
}

/**
* Determines if the given block type is visible in the inserter.
* Note that this is different than whether a block is allowed to be inserted.
* In some cases, the block is not allowed in a given position but
* it should still be visible in the inserter to be able to add it
* to a different position.
*
* @param {Object} state Editor state.
* @param {string|Object} blockNameOrType The block type object, e.g., the response
* from the block directory; or a string name of
* an installed block type, e.g.' core/paragraph'.
*
* @return {boolean} Whether the given block type is allowed to be inserted.
*/
const isBlockVisibleInTheInserter = ( state, blockNameOrType ) => {
let blockType;
let blockName;
if ( blockNameOrType && 'object' === typeof blockNameOrType ) {
blockType = blockNameOrType;
blockName = blockNameOrType.name;
} else {
blockType = getBlockType( blockNameOrType );
blockName = blockNameOrType;
}
if ( ! blockType ) {
return false;
}

const { allowedBlockTypes } = getSettings( state );

const isBlockAllowedInEditor = checkAllowList(
allowedBlockTypes,
blockName,
true
);
if ( ! isBlockAllowedInEditor ) {
return false;
}

// If parent blocks are not visible, child blocks should be hidden too.
if ( !! blockType.parent?.length ) {
return blockType.parent.some(
Copy link
Member

Choose a reason for hiding this comment

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

At this point, how do we know blockType.parent is an array? Can't it be a string, for example, that also has a length property?

For context, I'm looking at an issue in the support forum that I believe to be related to this check: the report is recent, the users report issues when inserting blocks, and there's no other place in the codebase that uses parent.some.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the block registration, it seems parent should be null or an array, but we don't enforce that.

Even the recently added logic to block registration doesn't check that parent is an array. This:

if (
  1 === settings?.parent?.length &&
  name === settings.parent[ 0 ]
) { /**/ }

would still be fine for blocks that register parent as a string.

Copy link
Member

Choose a reason for hiding this comment

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

One thing we can do to fix this is to make the if check above explicit for what we want to do: verify that it is an array and that it has elements.

The more general question is: how parent can become an array? Are block authors mis-registering this property? Can we now enforce it being an array?

Copy link
Member

Choose a reason for hiding this comment

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

PR at #66234

Copy link
Contributor

Choose a reason for hiding this comment

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

Even the recently added logic to block registration doesn't check that parent is an array.

Regarding this conditional statement, I suggested a change in this comment. However, I noticed that, unlike my comment, the order of each conditional was reversed when it was merged, which is certainly not what was intended.

Copy link
Member

Choose a reason for hiding this comment

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

cc @gziolo for thoughts about enforcing the blockType.parent being an array at registration. Would that be feasible?

Copy link
Member

Choose a reason for hiding this comment

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

We are discussing it in this thread: #66234 (comment). The short answer is if we know that block types were registered with the parent set to a string, the best way would be to respect that and provide a fallback handling.

( name ) =>
isBlockVisibleInTheInserter( state, name ) ||
// Exception for blocks with post-content parent,
// the root level is often consider as "core/post-content".
// This exception should only apply to the post editor ideally though.
name === 'core/post-content'
);
}
return true;
};

/**
* Determines if the given block type is allowed to be inserted into the block list.
* This function is not exported and not memoized because using a memoized selector
Expand All @@ -1558,27 +1611,17 @@ const canInsertBlockTypeUnmemoized = (
blockName,
rootClientId = null
) => {
if ( ! isBlockVisibleInTheInserter( state, blockName ) ) {
return false;
}

let blockType;
if ( blockName && 'object' === typeof blockName ) {
blockType = blockName;
blockName = blockType.name;
} else {
blockType = getBlockType( blockName );
}
if ( ! blockType ) {
return false;
}

const { allowedBlockTypes } = getSettings( state );

const isBlockAllowedInEditor = checkAllowList(
allowedBlockTypes,
blockName,
true
);
if ( ! isBlockAllowedInEditor ) {
return false;
}

const isLocked = !! getTemplateLock( state, rootClientId );
if ( isLocked ) {
Expand Down Expand Up @@ -1972,6 +2015,7 @@ const buildBlockTypeItem =
description: blockType.description,
category: blockType.category,
keywords: blockType.keywords,
parent: blockType.parent,
variations: inserterVariations,
example: blockType.example,
utility: 1, // Deprecated.
Expand Down Expand Up @@ -2067,16 +2111,18 @@ export const getInserterItems = createRegistrySelector( ( select ) =>
)
);
} else {
blockTypeInserterItems = blockTypeInserterItems.map(
( blockType ) => ( {
blockTypeInserterItems = blockTypeInserterItems
.filter( ( blockType ) =>
isBlockVisibleInTheInserter( state, blockType )
)
.map( ( blockType ) => ( {
...blockType,
isAllowedInCurrentRoot: canIncludeBlockTypeInInserter(
state,
blockType,
rootClientId
),
} )
);
} ) );
}

const items = blockTypeInserterItems.reduce(
Expand Down Expand Up @@ -2348,37 +2394,50 @@ const getAllowedPatternsDependants = ( select ) => ( state, rootClientId ) => [
*/
export const __experimentalGetAllowedPatterns = createRegistrySelector(
( select ) => {
return createSelector( ( state, rootClientId = null ) => {
const { getAllPatterns } = unlock( select( STORE_NAME ) );
const patterns = getAllPatterns();
const { allowedBlockTypes } = getSettings( state );
const parsedPatterns = patterns
.filter( ( { inserter = true } ) => !! inserter )
.map( ( pattern ) => {
return {
...pattern,
get blocks() {
return getParsedPattern( pattern ).blocks;
},
};
} );

const availableParsedPatterns = parsedPatterns.filter(
( pattern ) =>
checkAllowListRecursive(
getGrammar( pattern ),
allowedBlockTypes
)
);
const patternsAllowed = availableParsedPatterns.filter(
( pattern ) =>
getGrammar( pattern ).every( ( { blockName: name } ) =>
canInsertBlockType( state, name, rootClientId )
)
);
return createSelector(
(
state,
rootClientId = null,
options = DEFAULT_INSERTER_OPTIONS
) => {
const { getAllPatterns } = unlock( select( STORE_NAME ) );
const patterns = getAllPatterns();
const { allowedBlockTypes } = getSettings( state );
const parsedPatterns = patterns
.filter( ( { inserter = true } ) => !! inserter )
.map( ( pattern ) => {
return {
...pattern,
get blocks() {
return getParsedPattern( pattern ).blocks;
},
};
} );

const availableParsedPatterns = parsedPatterns.filter(
( pattern ) =>
checkAllowListRecursive(
getGrammar( pattern ),
allowedBlockTypes
)
);
const patternsAllowed = availableParsedPatterns.filter(
( pattern ) =>
getGrammar( pattern ).every( ( { blockName: name } ) =>
options[ isFiltered ] !== false
? canInsertBlockType(
state,
name,
rootClientId
)
: isBlockVisibleInTheInserter( state, name )
)
);

return patternsAllowed;
}, getAllowedPatternsDependants( select ) );
return patternsAllowed;
},
getAllowedPatternsDependants( select )
);
}
);

Expand Down
8 changes: 2 additions & 6 deletions test/e2e/specs/editor/various/allowed-patterns.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ test.describe( 'Allowed Patterns', () => {
);
} );

test( 'should show all patterns even if not allowed', async ( {
test( 'should hide patterns with only hidden blocks', async ( {
admin,
page,
} ) => {
Expand All @@ -77,11 +77,7 @@ test.describe( 'Allowed Patterns', () => {
page
.getByRole( 'listbox', { name: 'Block patterns' } )
.getByRole( 'option' )
).toHaveText( [
'Test: Single heading',
'Test: Single paragraph',
'Test: Paragraph inside group',
] );
).toHaveText( [ 'Test: Single heading' ] );
} );
} );
} );
Loading