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.

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: Disable editing of bound block attributes in editor UI #58085

Merged
merged 27 commits into from Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
2f071ad
Add actions and selectors to register new sources
SantosGuillamot Jan 22, 2024
57ab9b2
Add hook to read the bindings attribute in Edit
SantosGuillamot Jan 22, 2024
445405f
Add context to all the blocks with bindings
SantosGuillamot Jan 22, 2024
af7f870
Lock rich text when `isContentBound` is true
SantosGuillamot Jan 22, 2024
c3567a4
Adapt paragraph and heading blocks UI
SantosGuillamot Jan 22, 2024
35c8c64
Adapt button block UI
SantosGuillamot Jan 22, 2024
989f456
Adapt image block UI
SantosGuillamot Jan 22, 2024
a4dc34a
Register post meta source
SantosGuillamot Jan 22, 2024
3515538
Don't use placeholder if attribute is `src` or `href`
SantosGuillamot Jan 23, 2024
41709fa
Always share placeholder in case meta is empty
SantosGuillamot Jan 23, 2024
1567f17
Remove `keyToLabel` and use just label
SantosGuillamot Jan 23, 2024
1984749
Remove source component until it is needed
SantosGuillamot Jan 23, 2024
9644946
Use translations in the source label
SantosGuillamot Jan 23, 2024
478f861
Move `select` inside `useSource`
SantosGuillamot Jan 23, 2024
2acf6bd
Read `lockEditorUI` prop and add it for patterns
SantosGuillamot Jan 23, 2024
a8a6da3
Move logic to lock editing directly to RichText
SantosGuillamot Jan 23, 2024
30b635e
Improve `useSelect` destructuring
SantosGuillamot Jan 23, 2024
a6f5fde
Load all image controls if attributes are bound
SantosGuillamot Jan 23, 2024
7d0cb9a
Remove unnecessary condition
SantosGuillamot Jan 23, 2024
e6a5a4d
Move `lockAttributesEditing` to source definition
SantosGuillamot Jan 23, 2024
7c1ca5a
Move `useSelect` into existing hook
SantosGuillamot Jan 23, 2024
4246260
Fix `RichText` not being selected on click
SantosGuillamot Jan 23, 2024
5a0cee8
Lock button and image controls only when selected
SantosGuillamot Jan 23, 2024
54c313d
Remove unnecesarry optional chaining
SantosGuillamot Jan 24, 2024
aaf8652
Move `shouldDisableEditing` logic inside callback
SantosGuillamot Jan 24, 2024
1f34e08
Merge branch 'trunk' into add/block-bindings-support-in-the-editor
SantosGuillamot Jan 24, 2024
895e6dd
Fix formatting issue
SantosGuillamot Jan 24, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/rich-text/index.js
Expand Up @@ -376,7 +376,7 @@ export function RichTextWrapper(
useFirefoxCompat(),
anchorRef,
] ) }
contentEditable={ true }
contentEditable={ props.isContentBound ? false : true }
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

@ellatrix, what alternatives are there to put RichText in read-only mode that don't require introducing a new prop?

I'm also wondering what should happen for all these users using assistive technology. It feels like we should ensure it still possible to move focus when tabbing to the HTML element as in some cases RichText is the block wrapper so it could make it harder navigating to blocks like Paragraph or Heading.

Copy link
Member

Choose a reason for hiding this comment

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

Noting also that this new prop fails one of the unit tests:

Screenshot 2024-01-23 at 10 03 46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we can get directly the block attributes from the RichText component and read the metadata.bindings there. I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this commit to reflect what I was referring to. It seems cleaner to me, but I am not familiar enough with RichText to say if that's the right approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also notice that selecting the Paragraph block that is connected to a source requires a double click in the "edit" mode. In the "select" mode it seems that it works just fine.

output_56e741.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's not the best UX. I think that's the way the RichText works, right now when the editing is locked, so I am not sure if that's on purpose or we should look for a different approach.

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 should be fixed with this commit.

suppressContentEditableWarning={ true }
className={ classnames(
'block-editor-rich-text__editable',
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.js
Expand Up @@ -27,6 +27,7 @@ import contentLockUI from './content-lock-ui';
import './metadata';
import blockHooks from './block-hooks';
import blockRenaming from './block-renaming';
import './use-bindings-attributes';

createBlockEditFilter(
[
Expand Down
144 changes: 144 additions & 0 deletions packages/block-editor/src/hooks/use-bindings-attributes.js
@@ -0,0 +1,144 @@
/**
* WordPress dependencies
*/
import { createHigherOrderComponent } from '@wordpress/compose';
import { useRegistry, useSelect } from '@wordpress/data';
import { addFilter } from '@wordpress/hooks';
/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../store';
import { useBlockEditContext } from '../components/block-edit/context';
import { unlock } from '../lock-unlock';

/** @typedef {import('@wordpress/compose').WPHigherOrderComponent} WPHigherOrderComponent */
/** @typedef {import('@wordpress/blocks').WPBlockSettings} WPBlockSettings */

/**
* Given a binding of block attributes, returns a higher order component that
* overrides its `attributes` and `setAttributes` props to sync any changes needed.
*
* @return {WPHigherOrderComponent} Higher-order component.
*/

const BLOCK_BINDINGS_ALLOWED_BLOCKS = {
'core/paragraph': [ 'content' ],
'core/heading': [ 'content' ],
'core/image': [ 'url', 'title', 'alt' ],
'core/button': [ 'url', 'text' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add the linkTarget here?

Copy link
Member

Choose a reason for hiding this comment

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

There were some issues with linkTarget. Let's keep it out of the scope for now.

};

const createEditFunctionWithBindingsAttribute = () =>
createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const { clientId } = useBlockEditContext();

const {
getBlockBindingsSource,
getBlockAttributes,
updateBlockAttributes,
} = useSelect( ( select ) => {
return {
getBlockBindingsSource: unlock( select( blockEditorStore ) )
.getBlockBindingsSource,
getBlockAttributes:
select( blockEditorStore ).getBlockAttributes,
updateBlockAttributes:
select( blockEditorStore ).updateBlockAttributes,
};
}, [] );

const updatedAttributes = getBlockAttributes( clientId );
if ( updatedAttributes?.metadata?.bindings ) {
Object.entries( updatedAttributes.metadata.bindings ).forEach(
( [ attributeName, settings ] ) => {
const source = getBlockBindingsSource(
settings.source.name
);

if ( source ) {
// Second argument (`updateMetaValue`) will be used to update the value in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea or advance of how it's going to implement the updateMetaValue usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The technical implementation is almost there. However, there are many things to take into account, and it was decided not to rush things for the upcoming 6.5 to ensure that whatever we land is stable enough.

const {
placeholder,
useValue: [ metaValue = null ] = [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it metaValue a proper name here? In theory, the source can be arbitrary meaning it could belong to metadata, but also to very different origins.
Could we consider using something like sourceValue, and setSourceValue?

Copy link
Member

Choose a reason for hiding this comment

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

Good feedback. On the server, there is get_value_callback registered for every block bindings source.

} = source.useSource(
Copy link
Member

Choose a reason for hiding this comment

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

Is this a hook? You cannot use hooks inside conditions and loops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of post meta source, it uses useSelect and useEntityProp: link. Although each source could do whatever they want.

If that's a problem, do you have any ideas on how that should be handled? We need to iterate through the different bindings and get the value from each of the sources.

Copy link
Member

Choose a reason for hiding this comment

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

The only solution is to probably mount sub components, get the result, and set some state in this component, similar to how it is done here:

useBlockProps={ useBlockProps }

But I'd love to hear other people's thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @ellatrix! Yup, we'll have to change it because it is a hook.

props,
settings.source.attributes
);

if ( placeholder ) {
updatedAttributes.placeholder = placeholder;
updatedAttributes[ attributeName ] = null;
}

if ( metaValue ) {
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
updatedAttributes[ attributeName ] = metaValue;
}
}
}
);
}

const registry = useRegistry();

return (
<>
<BlockEdit
key="edit"
attributes={ updatedAttributes }
setAttributes={ ( newAttributes, blockId ) =>
registry.batch( () =>
updateBlockAttributes( blockId, newAttributes )
)
}
Comment on lines +93 to +97
Copy link
Member

@Mamaduka Mamaduka Feb 8, 2024

Choose a reason for hiding this comment

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

  • The blocks expect setAttributes to have a stable reference between rerenders. This override breaks that.
  • What's the reason for batching the single action? Technically, it shouldn't make a difference.

P.S. There is no need to wrap the returned BlockEdit in the fragment.

Copy link
Member

Choose a reason for hiding this comment

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

@Mamaduka, I think the issue at the moment is that users will only change the attribute when it is not bound. When the value is sourced from the block binding then the attribute becomes readonly. In effect, the batching isn't really necessary and even there should be no override set until it's possible to edit the external source in UI.

By the way, feel free to refactor the code here if you have some ideas how to optimize it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the issue at the moment is that users will only change the attribute when it is not bound. When the value is sourced from the block binding then the attribute becomes readonly.

Is that handled somewhere else? Because I don't see that logic here. If the block calls the setAttribute override, it will update the attribute, bound or not.

Sorry, I'm not super familiar with the internals of Block Binding API. I just came across this code while looking for something else.

Copy link
Member

Choose a reason for hiding this comment

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

it will update the attribute, bound or not

There is no way to trigger the update from UI at the moment for supported blocks and their selected attributes. Direct changes to the store don't matter because the value will get replaced on the frontend anyway.

You are totally right that batching is not needed at the moment and most likely the override for setAttributes is premature.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming. I can push refactoring PR later today.

{ ...props }
/>
</>
);
},
'useBoundAttributes'
);

/**
* Filters a registered block's settings to enhance a block's `edit` component
* to upgrade bound attributes.
*
* @param {WPBlockSettings} settings Registered block settings.
*
* @return {WPBlockSettings} Filtered block settings.
*/
function shimAttributeSource( settings ) {
if ( ! ( settings.name in BLOCK_BINDINGS_ALLOWED_BLOCKS ) ) {
return settings;
}
settings.edit = createEditFunctionWithBindingsAttribute()( settings.edit );
Copy link
Member

Choose a reason for hiding this comment

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

What stops us from using the new "api" here? This will cause a bit of a performance regression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to modify the attributes and setAttributes of the BlockEdit component with the new API? If that's possible I believe I could change it.


return settings;
}

addFilter(
'blocks.registerBlockType',
'core/editor/custom-sources-backwards-compatibility/shim-attribute-source',
shimAttributeSource
);

// Add the context to all blocks.
addFilter(
'blocks.registerBlockType',
'core/block-bindings-ui',
( settings, name ) => {
if ( ! ( name in BLOCK_BINDINGS_ALLOWED_BLOCKS ) ) {
return settings;
}
const contextItems = [ 'postId', 'postType', 'queryId' ];
const usesContextArray = settings.usesContext;
const oldUsesContextArray = new Set( usesContextArray );
contextItems.forEach( ( item ) => {
if ( ! oldUsesContextArray.has( item ) ) {
usesContextArray.push( item );
}
} );
settings.usesContext = usesContextArray;
return settings;
}
);
SantosGuillamot marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/private-actions.js
Expand Up @@ -360,3 +360,13 @@ export function stopEditingAsBlocks( clientId ) {
dispatch.__unstableSetTemporarilyEditingAsBlocks();
};
}

export function registerBlockBindingsSource( source ) {
return {
type: 'REGISTER_BLOCK_BINDINGS_SOURCE',
sourceName: source.name,
sourceLabel: source.label,
sourceComponent: source.component,
useSource: source.useSource,
};
}
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 8 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
Expand Up @@ -286,3 +286,11 @@ export const hasAllowedPatterns = createSelector(
export function getLastFocus( state ) {
return state.lastFocus;
}

export function getAllBlockBindingsSources( state ) {
return state.blockBindingsSources;
}

export function getBlockBindingsSource( state, sourceName ) {
return state?.blockBindingsSources?.[ sourceName ];
SantosGuillamot marked this conversation as resolved.
Show resolved Hide resolved
}
16 changes: 16 additions & 0 deletions packages/block-editor/src/store/reducer.js
Expand Up @@ -2017,6 +2017,21 @@ export function lastFocus( state = false, action ) {
return state;
}

function blockBindingsSources( state = {}, action ) {
if ( action.type === 'REGISTER_BLOCK_BINDINGS_SOURCE' ) {
return {
...state,
[ action.sourceName ]: {
label: action.sourceLabel,
component: action.sourceComponent,
useSource: action.useSource,
},
};
}

return state;
}

const combinedReducers = combineReducers( {
blocks,
isTyping,
Expand Down Expand Up @@ -2047,6 +2062,7 @@ const combinedReducers = combineReducers( {
blockRemovalRules,
openedBlockSettingsMenu,
registeredInserterMediaCategories,
blockBindingsSources,
} );

function withAutomaticChangeReset( reducer ) {
Expand Down
79 changes: 42 additions & 37 deletions packages/block-library/src/button/edit.js
Expand Up @@ -164,6 +164,7 @@ function ButtonEdit( props ) {
text,
url,
width,
metadata,
} = attributes;

const TagName = tagName || 'a';
Expand Down Expand Up @@ -276,6 +277,7 @@ function ButtonEdit( props ) {
onReplace={ onReplace }
onMerge={ mergeBlocks }
identifier="text"
isContentBound={ metadata?.bindings?.text }
/>
</div>
<BlockControls group="block">
Expand All @@ -287,7 +289,7 @@ function ButtonEdit( props ) {
} }
/>
) }
{ ! isURLSet && isLinkTag && (
{ ! isURLSet && isLinkTag && ! metadata?.bindings?.url && (
<ToolbarButton
name="link"
icon={ link }
Expand All @@ -296,7 +298,7 @@ function ButtonEdit( props ) {
onClick={ startEditing }
/>
) }
{ isURLSet && isLinkTag && (
{ isURLSet && isLinkTag && ! metadata?.bindings?.url && (
Copy link
Member

Choose a reason for hiding this comment

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

What if someone would like to unlink the button? In general, should it be possible to remove the binding in the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, should it be possible to remove the binding in the UI?

I wasn't sure about that one. My reasoning was that, as right now bindings can only be added through the code editor, it shouldn't be a blocker if they can only be removed through the code editor as well. When we improve the UI and add the possibility to create bindings, we should provide mechanisms to remove them as well.

Anyway, I don't know which is the correct approach. Any thoughts?

<ToolbarButton
name="link"
icon={ linkOff }
Expand All @@ -307,43 +309,46 @@ function ButtonEdit( props ) {
/>
) }
</BlockControls>
{ isLinkTag && isSelected && ( isEditingURL || isURLSet ) && (
<Popover
placement="bottom"
onClose={ () => {
setIsEditingURL( false );
richTextRef.current?.focus();
} }
anchor={ popoverAnchor }
focusOnMount={ isEditingURL ? 'firstElement' : false }
__unstableSlotName={ '__unstable-block-tools-after' }
shift
>
<LinkControl
value={ linkValue }
onChange={ ( {
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} ) =>
setAttributes(
getUpdatedLinkAttributes( {
rel,
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} )
)
}
onRemove={ () => {
unlink();
{ isLinkTag &&
isSelected &&
( isEditingURL || isURLSet ) &&
! metadata?.bindings?.url && (
<Popover
placement="bottom"
onClose={ () => {
setIsEditingURL( false );
richTextRef.current?.focus();
} }
forceIsEditingLink={ isEditingURL }
settings={ LINK_SETTINGS }
/>
</Popover>
) }
anchor={ popoverAnchor }
focusOnMount={ isEditingURL ? 'firstElement' : false }
__unstableSlotName={ '__unstable-block-tools-after' }
shift
>
<LinkControl
value={ linkValue }
onChange={ ( {
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} ) =>
setAttributes(
getUpdatedLinkAttributes( {
rel,
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} )
)
}
onRemove={ () => {
unlink();
richTextRef.current?.focus();
} }
forceIsEditingLink={ isEditingURL }
settings={ LINK_SETTINGS }
/>
</Popover>
) }
<InspectorControls>
<WidthPanel
selectedWidth={ width }
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/heading/edit.js
Expand Up @@ -33,7 +33,8 @@ function HeadingEdit( {
style,
clientId,
} ) {
const { textAlign, content, level, placeholder, anchor } = attributes;
const { textAlign, content, level, placeholder, anchor, metadata } =
attributes;
const tagName = 'h' + level;
const blockProps = useBlockProps( {
className: classnames( {
Expand Down Expand Up @@ -138,6 +139,7 @@ function HeadingEdit( {
onRemove={ () => onReplace( [] ) }
placeholder={ placeholder || __( 'Heading' ) }
textAlign={ textAlign }
isContentBound={ metadata?.bindings?.content }
{ ...( Platform.isNative && { deleteEnter: true } ) } // setup RichText on native mobile to delete the "Enter" key as it's handled by the JS/RN side
{ ...blockProps }
/>
Expand Down