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

Block Bindings: Fix label in pattern overrides #65302

Closed
wants to merge 2 commits into from
Closed

Block Bindings: Fix label in pattern overrides #65302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f941e5
Add pattern context to block selection button
artemiomorales Sep 13, 2024
b0c7892
Revise paragraph logic to use pattern override content
artemiomorales Sep 13, 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
25 changes: 22 additions & 3 deletions packages/block-editor/src/components/block-tools/block-selection-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ import { store as blockEditorStore } from '../../store';
import BlockDraggable from '../block-draggable';
import { useBlockElement } from '../block-list/use-block-props/use-block-refs';
import { unlock } from '../../lock-unlock';
import { getBindingsValues } from '../../hooks/use-bindings-attributes';
import {
getBindingsValues,
replacePatternOverrideDefaultBindings,
} from '../../hooks/use-bindings-attributes';
import BlockContext from '../block-context';

/**
Expand Down Expand Up @@ -71,17 +74,33 @@ function BlockSelectionButton( { clientId, rootClientId }, ref ) {
getNextBlockClientId,
getPreviousBlockClientId,
canMoveBlock,
getBlockParents,
} = unlock( select( blockEditorStore ) );
const { getActiveBlockVariation, getBlockType } =
select( blocksStore );
const index = getBlockIndex( clientId );
const { name, attributes } = getBlock( clientId );
const block = getBlock( clientId );
const { name, attributes } = block;
const blockType = getBlockType( name );
const orientation =
getBlockListSettings( rootClientId )?.orientation;
const match = getActiveBlockVariation( name, attributes );

const blockBindings = attributes?.metadata?.bindings;
const blockBindings = replacePatternOverrideDefaultBindings(
name,
attributes?.metadata?.bindings
);

if ( blockBindings?.content?.source === 'core/pattern-overrides' ) {
const parents = getBlockParents( clientId );
const parentBlock =
parents && parents.length > 0
? getBlock( parents[ 0 ] )
: null;
blockContext[ 'pattern/overrides' ] =
parentBlock?.attributes?.content;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem like the most elegant solution to me. Is there another way to get the pattern attributes other than to look up the values manually and insert them like this?

Also, I'm unsure if we can assume that the first parent will be the pattern. Gutenberg seems to flatten the hierarchy of blocks in pattern instances, so this appears to work, but it'd be great to additional insight on this 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm unsure if we can assume that the first parent will be the pattern. Gutenberg seems to flatten the hierarchy of blocks in pattern instances, so this appears to work, but it'd be great to additional insight on this

Yep I think you'll need to iterate through all block parents to find the core/block block. Also need to take into account a pattern within a nested pattern, and ensure you find the nearest one.

There's a few places with this kind of code 😅

Just to note, this component would be removed if #65204 lands as is.


const boundAttributes = getBindingsValues(
blockBindings,
name,
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/use-bindings-attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const DEFAULT_ATTRIBUTE = '__default';
*
* @return {Object} The bindings with default replaced for pattern overrides.
*/
function replacePatternOverrideDefaultBindings( blockName, bindings ) {
export function replacePatternOverrideDefaultBindings( blockName, bindings ) {
// The `__default` binding currently only works for pattern overrides.
if (
bindings?.[ DEFAULT_ATTRIBUTE ]?.source === 'core/pattern-overrides'
Expand Down
13 changes: 8 additions & 5 deletions packages/block-library/src/paragraph/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,21 @@ export const settings = {
__experimentalLabel( attributes, { context } ) {
const customName = attributes?.metadata?.name;

if ( context === 'list-view' && customName ) {
return customName;
}

if ( context === 'accessibility' ) {
if ( customName ) {
if (
customName &&
! attributes?.metadata?.bindings?.source === 'pattern/overrides'
) {
Copy link
Contributor Author

@artemiomorales artemiomorales Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like the hardcoding of pattern/overrides here. I also don't like that the pattern special case would need to be accounted for in every relevant block. Should we explore an alternative solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name is wrong ('core/pattern-overrides').

On a different note, I'm wondering whether this is a pattern override specific issue. The problem seems to be that when a paragraph is named, the name is announced instead of the content. It is perhaps worse for pattern overrides since all editable blocks will be named, but the problem still exists for regular paragraphs too.

Either way it seems like screenreader users might miss valuable context (if the name isn't announced a user might wonder why given they explicitly gave it the name).

A compromize could be to announce both the name and the content if it can be made clear enough.

Copy link
Contributor Author

@artemiomorales artemiomorales Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the name is wrong ('core/pattern-overrides').

pattern/overrides is how it's defined in the block context, so I was just following that pattern:

gutenberg/lib/compat/wordpress-6.6/block-bindings/pattern-overrides.php

Lines 52 to 59 in b0c7892

register_block_bindings_source(
'core/pattern-overrides',
array(
'label' => _x( 'Pattern Overrides', 'block bindings source' ),
'get_value_callback' => 'gutenberg_block_bindings_pattern_overrides_callback',
'uses_context' => array( 'pattern/overrides' ),
)
);

gutenberg/packages/block-editor/src/hooks/use-bindings-attributes.js

Line 183 in b0c7892

const hasParentPattern = !! props.context[ 'pattern/overrides' ];

On a different note, I'm wondering whether this is a pattern override specific issue. The problem seems to be that when a paragraph is named, the name is announced instead of the content. It is perhaps worse for pattern overrides since all editable blocks will be named, but the problem still exists for regular paragraphs too.

Either way it seems like screenreader users might miss valuable context (if the name isn't announced a user might wonder why given they explicitly gave it the name).

A compromize could be to announce both the name and the content if it can be made clear enough.

This is a good point. I think announcing both the name and content is the right approach. I can implement that change as part of this PR if we decide to move forward with it, or create an issue and perhaps handle in a separate PR if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

pattern/overrides is how it's defined in the block context, so I was just following that pattern:

This particular line refers to the binding source, which is defined here:

gutenberg/packages/editor/src/bindings/pattern-overrides.js

Line 9 in df27bd9

name: 'core/pattern-overrides',

return customName;
}

const { content } = attributes;
return ! content || content.length === 0 ? __( 'Empty' ) : content;
}

if ( context === 'list-view' && customName ) {
return customName;
}
},
transforms,
deprecated,
Expand Down
Loading