-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Block Bindings: Communicate supported block attributes from server side #71820
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
Changes from all commits
6816262
76053ff
6b9045d
0a527a6
77daf10
6a9b106
26e7dff
42d5154
e7d278d
0c41d6c
7abc40c
1659414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| https://github.com/WordPress/wordpress-develop/pull/9992 | ||
|
|
||
| * https://github.com/WordPress/gutenberg/pull/71820 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,10 +22,10 @@ import { useCallback, useContext, useMemo } from '@wordpress/element'; | |
| import BlockContext from '../block-context'; | ||
| import isURLLike from '../link-control/is-url-like'; | ||
| import { | ||
| canBindAttribute, | ||
| hasPatternOverridesDefaultBinding, | ||
| replacePatternOverridesDefaultBinding, | ||
| } from '../../utils/block-bindings'; | ||
| import { store as blockEditorStore } from '../../store'; | ||
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| /** | ||
|
|
@@ -56,6 +56,8 @@ const Edit = ( props ) => { | |
|
|
||
| const EditWithFilters = withFilters( 'editor.BlockEdit' )( Edit ); | ||
|
|
||
| const EMPTY_ARRAY = []; | ||
|
|
||
| const EditWithGeneratedProps = ( props ) => { | ||
| const { name, clientId, attributes, setAttributes } = props; | ||
| const registry = useRegistry(); | ||
|
|
@@ -66,6 +68,17 @@ const EditWithGeneratedProps = ( props ) => { | |
| unlock( select( blocksStore ) ).getAllBlockBindingsSources(), | ||
| [] | ||
| ); | ||
| const bindableAttributes = useSelect( | ||
| ( select ) => { | ||
| const { __experimentalBlockBindingsSupportedAttributes } = | ||
| select( blockEditorStore ).getSettings(); | ||
| return ( | ||
| __experimentalBlockBindingsSupportedAttributes?.[ name ] || | ||
| EMPTY_ARRAY | ||
| ); | ||
| }, | ||
| [ name ] | ||
| ); | ||
|
|
||
| const { blockBindings, context, hasPatternOverrides } = useMemo( () => { | ||
| // Assign context values using the block type's declared context needs. | ||
|
|
@@ -90,8 +103,8 @@ const EditWithGeneratedProps = ( props ) => { | |
| } | ||
| return { | ||
| blockBindings: replacePatternOverridesDefaultBinding( | ||
| name, | ||
| attributes?.metadata?.bindings | ||
| attributes?.metadata?.bindings, | ||
| bindableAttributes | ||
| ), | ||
| context: computedContext, | ||
| hasPatternOverrides: hasPatternOverridesDefaultBinding( | ||
|
|
@@ -120,7 +133,10 @@ const EditWithGeneratedProps = ( props ) => { | |
| ) ) { | ||
| const { source: sourceName, args: sourceArgs } = binding; | ||
| const source = registeredSources[ sourceName ]; | ||
| if ( ! source || ! canBindAttribute( name, attributeName ) ) { | ||
| if ( | ||
| ! source || | ||
| ! bindableAttributes.includes( attributeName ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uncaught TypeError: Cannot read properties of undefined (reading 'includes')
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -172,6 +188,7 @@ const EditWithGeneratedProps = ( props ) => { | |
| }, | ||
| [ | ||
| attributes, | ||
| bindableAttributes, | ||
| blockBindings, | ||
| clientId, | ||
| context, | ||
|
|
@@ -197,7 +214,7 @@ const EditWithGeneratedProps = ( props ) => { | |
| ) ) { | ||
| if ( | ||
| ! blockBindings[ attributeName ] || | ||
| ! canBindAttribute( name, attributeName ) | ||
| ! bindableAttributes.includes( attributeName ) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
@@ -250,6 +267,7 @@ const EditWithGeneratedProps = ( props ) => { | |
| } ); | ||
| }, | ||
| [ | ||
| bindableAttributes, | ||
| blockBindings, | ||
| clientId, | ||
| context, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,6 @@ import { useBlockRefProvider } from './use-block-refs'; | |
| import { useIntersectionObserver } from './use-intersection-observer'; | ||
| import { useScrollIntoView } from './use-scroll-into-view'; | ||
| import { useFlashEditableBlocks } from '../../use-flash-editable-blocks'; | ||
| import { canBindBlock } from '../../../utils/block-bindings'; | ||
| import { useFirefoxDraggableCompatibility } from './use-firefox-draggable-compatibility'; | ||
|
|
||
| /** | ||
|
|
@@ -128,14 +127,13 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) { | |
|
|
||
| const blockEditContext = useBlockEditContext(); | ||
| const hasBlockBindings = !! blockEditContext[ blockBindingsKey ]; | ||
| const bindingsStyle = | ||
| hasBlockBindings && canBindBlock( name ) | ||
| ? { | ||
| '--wp-admin-theme-color': 'var(--wp-block-synced-color)', | ||
| '--wp-admin-theme-color--rgb': | ||
| 'var(--wp-block-synced-color--rgb)', | ||
| } | ||
| : {}; | ||
| const bindingsStyle = hasBlockBindings | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I recall correctly, the check was in place to cover for the case when someone adds bindings in metadata, but the block doesn't support bindings.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. This is the only part where I had to change the logic.
TBH I'm not even sure is I'm willing to look into tightening the logic so it'll really only highlight the block if it does support bindings, but it looked like it needed further research how to best do this, and it seemed like a small enough edge case that I didn't want to block this PR by it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't consider it a blocker. If you find a better place to inject this special styling, that would be the best solution for me. It can be handled seperately. |
||
| ? { | ||
| '--wp-admin-theme-color': 'var(--wp-block-synced-color)', | ||
| '--wp-admin-theme-color--rgb': | ||
| 'var(--wp-block-synced-color--rgb)', | ||
| } | ||
| : {}; | ||
|
|
||
| // Ensures it warns only inside the `edit` implementation for the block. | ||
| if ( blockApiVersion < 2 && clientId === blockEditContext.clientId ) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,11 +23,7 @@ import { useViewportMatch } from '@wordpress/compose'; | |||||||||||||||
| /** | ||||||||||||||||
| * Internal dependencies | ||||||||||||||||
| */ | ||||||||||||||||
| import { | ||||||||||||||||
| canBindAttribute, | ||||||||||||||||
| getBindableAttributes, | ||||||||||||||||
| useBlockBindingsUtils, | ||||||||||||||||
| } from '../utils/block-bindings'; | ||||||||||||||||
| import { useBlockBindingsUtils } from '../utils/block-bindings'; | ||||||||||||||||
| import { unlock } from '../lock-unlock'; | ||||||||||||||||
| import InspectorControls from '../components/inspector-controls'; | ||||||||||||||||
| import BlockContext from '../components/block-context'; | ||||||||||||||||
|
|
@@ -205,52 +201,62 @@ function EditableBlockBindingsPanelItems( { | |||||||||||||||
| export const BlockBindingsPanel = ( { name: blockName, metadata } ) => { | ||||||||||||||||
| const blockContext = useContext( BlockContext ); | ||||||||||||||||
| const { removeAllBlockBindings } = useBlockBindingsUtils(); | ||||||||||||||||
| const bindableAttributes = getBindableAttributes( blockName ); | ||||||||||||||||
| const dropdownMenuProps = useToolsPanelDropdownMenuProps(); | ||||||||||||||||
|
|
||||||||||||||||
| // `useSelect` is used purposely here to ensure `getFieldsList` | ||||||||||||||||
| // is updated whenever there are updates in block context. | ||||||||||||||||
| // `source.getFieldsList` may also call a selector via `select`. | ||||||||||||||||
| const _fieldsList = {}; | ||||||||||||||||
| const { fieldsList, canUpdateBlockBindings } = useSelect( | ||||||||||||||||
| ( select ) => { | ||||||||||||||||
| if ( ! bindableAttributes || bindableAttributes.length === 0 ) { | ||||||||||||||||
| return EMPTY_OBJECT; | ||||||||||||||||
| } | ||||||||||||||||
| const registeredSources = getBlockBindingsSources(); | ||||||||||||||||
| Object.entries( registeredSources ).forEach( | ||||||||||||||||
| ( [ sourceName, { getFieldsList, usesContext } ] ) => { | ||||||||||||||||
| if ( getFieldsList ) { | ||||||||||||||||
| // Populate context. | ||||||||||||||||
| const context = {}; | ||||||||||||||||
| if ( usesContext?.length ) { | ||||||||||||||||
| for ( const key of usesContext ) { | ||||||||||||||||
| context[ key ] = blockContext[ key ]; | ||||||||||||||||
| const { bindableAttributes, fieldsList, canUpdateBlockBindings } = | ||||||||||||||||
| useSelect( | ||||||||||||||||
| ( select ) => { | ||||||||||||||||
| const { __experimentalBlockBindingsSupportedAttributes } = | ||||||||||||||||
| select( blockEditorStore ).getSettings(); | ||||||||||||||||
| const _bindableAttributes = | ||||||||||||||||
| __experimentalBlockBindingsSupportedAttributes?.[ | ||||||||||||||||
| blockName | ||||||||||||||||
| ]; | ||||||||||||||||
| if ( | ||||||||||||||||
| ! _bindableAttributes || | ||||||||||||||||
| _bindableAttributes.length === 0 | ||||||||||||||||
| ) { | ||||||||||||||||
| return EMPTY_OBJECT; | ||||||||||||||||
| } | ||||||||||||||||
| const registeredSources = getBlockBindingsSources(); | ||||||||||||||||
| Object.entries( registeredSources ).forEach( | ||||||||||||||||
| ( [ sourceName, { getFieldsList, usesContext } ] ) => { | ||||||||||||||||
| if ( getFieldsList ) { | ||||||||||||||||
| // Populate context. | ||||||||||||||||
| const context = {}; | ||||||||||||||||
| if ( usesContext?.length ) { | ||||||||||||||||
| for ( const key of usesContext ) { | ||||||||||||||||
| context[ key ] = blockContext[ key ]; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| const sourceList = getFieldsList( { | ||||||||||||||||
| select, | ||||||||||||||||
| context, | ||||||||||||||||
| } ); | ||||||||||||||||
| // Only add source if the list is not empty. | ||||||||||||||||
| if ( Object.keys( sourceList || {} ).length ) { | ||||||||||||||||
| _fieldsList[ sourceName ] = { ...sourceList }; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| const sourceList = getFieldsList( { | ||||||||||||||||
| select, | ||||||||||||||||
| context, | ||||||||||||||||
| } ); | ||||||||||||||||
| // Only add source if the list is not empty. | ||||||||||||||||
| if ( Object.keys( sourceList || {} ).length ) { | ||||||||||||||||
| _fieldsList[ sourceName ] = { ...sourceList }; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| ); | ||||||||||||||||
| return { | ||||||||||||||||
| fieldsList: | ||||||||||||||||
| Object.values( _fieldsList ).length > 0 | ||||||||||||||||
| ? _fieldsList | ||||||||||||||||
| : EMPTY_OBJECT, | ||||||||||||||||
| canUpdateBlockBindings: | ||||||||||||||||
| select( blockEditorStore ).getSettings() | ||||||||||||||||
| .canUpdateBlockBindings, | ||||||||||||||||
| }; | ||||||||||||||||
| }, | ||||||||||||||||
| [ blockContext, bindableAttributes ] | ||||||||||||||||
| ); | ||||||||||||||||
| ); | ||||||||||||||||
| return { | ||||||||||||||||
| bindableAttributes: _bindableAttributes, | ||||||||||||||||
| fieldsList: | ||||||||||||||||
| Object.values( _fieldsList ).length > 0 | ||||||||||||||||
| ? _fieldsList | ||||||||||||||||
| : EMPTY_OBJECT, | ||||||||||||||||
| canUpdateBlockBindings: | ||||||||||||||||
| select( blockEditorStore ).getSettings() | ||||||||||||||||
| .canUpdateBlockBindings, | ||||||||||||||||
| }; | ||||||||||||||||
| }, | ||||||||||||||||
| [ blockContext ] | ||||||||||||||||
| ); | ||||||||||||||||
| // Return early if there are no bindable attributes. | ||||||||||||||||
| if ( ! bindableAttributes || bindableAttributes.length === 0 ) { | ||||||||||||||||
| return null; | ||||||||||||||||
|
|
@@ -260,7 +266,7 @@ export const BlockBindingsPanel = ( { name: blockName, metadata } ) => { | |||||||||||||||
| const filteredBindings = { ...bindings }; | ||||||||||||||||
| Object.keys( filteredBindings ).forEach( ( key ) => { | ||||||||||||||||
| if ( | ||||||||||||||||
| ! canBindAttribute( blockName, key ) || | ||||||||||||||||
| ! bindableAttributes.includes( key ) && | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @talldan, all e2e tests pass, but I would still appreciate some confirmation that this change doesn't introduce regressions. I noticed it when performing a post-merge check on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gziolo Do you mean the change from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it wasn't immediately apparent.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The With this change you only delete the attribute if it has a pattern overrides binding.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll file a fix for this tomorrow.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, looks like the second part of the criterion was already removed altogether: 2a9598c#diff-bff98e322c4269c50aa13e4b251beb9d53d2d0c3d4066406f735d2d49bed83a1L269-L270
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure the new version is quite correct TBH: gutenberg/packages/block-editor/src/hooks/block-bindings.js Lines 439 to 445 in 1c5976e
Maybe we did this accidentally, since the diff --git a/packages/block-editor/src/hooks/block-bindings.js b/packages/block-editor/src/hooks/block-bindings.js
index 87ab4e3632..a8cc0d369f 100644
--- a/packages/block-editor/src/hooks/block-bindings.js
+++ b/packages/block-editor/src/hooks/block-bindings.js
@@ -436,11 +436,14 @@ export const BlockBindingsPanel = ( { name: blockName, metadata } ) => {
if ( ! bindableAttributes || bindableAttributes.length === 0 ) {
return null;
}
- // Filter bindings to only show bindable attributes.
+ // Filter bindings to only show bindable attributes and remove pattern overrides.
const { bindings } = metadata || {};
const filteredBindings = { ...bindings };
Object.keys( filteredBindings ).forEach( ( key ) => {
- if ( ! bindableAttributes.includes( key ) ) {
+ if (
+ ! bindableAttributes.includes( key ) ||
+ filteredBindings[ key ].source === 'core/pattern-overrides'
+ ) {
delete filteredBindings[ key ];
}
} );I'll be AFK on Thu and Fri; @gziolo @cbravobernal Maybe y'all could look into this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit again: The pattern overrides should never appear. As it doesn't contain an editorUI attribute. And when a block contains a pattern override, the That check is not needed. I found another bug if there are no sources. We show the panel without a readonly attributes panel, which is not ideal. Drafted a PR: #72253 |
||||||||||||||||
| filteredBindings[ key ].source === 'core/pattern-overrides' | ||||||||||||||||
| ) { | ||||||||||||||||
| delete filteredBindings[ key ]; | ||||||||||||||||
|
|
||||||||||||||||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at code vitals, this PR made the "hover" metric slower. My guess is it's because of this addition here. I think it's also responsible of a small impact on the "type" metrics (even though it's hard to see there)
I think the reason (a guess, not 100% certain) is that we prefer to avoid adding new useSelect calls as much as possible to these components that run for every block. Now, it's possible to solve this as we have a "private edit context" (or something like that) to solve this. Basically we group all the useSelect calls that we need for a block at a single place.
Can we try moving this there to see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just saw the previous related discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbravobernal, is that on your radar for 6.9?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it to the "Bugfixes" section in #67520.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad Can you help me figure out where to put this specifically? The selector checks what attributes are supported by block bindings, which is information passed by the server (via
getSettings()). Potentially, any block can have such attributes, which is why we're running this code for all blocks...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in this useSelect
gutenberg/packages/block-editor/src/components/block-list/block.js
Line 558 in f23e6ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbravobernal has filed a PR to address this: #72351