From 194740299305fd0cb2fd79bbc8e4fa2097939c24 Mon Sep 17 00:00:00 2001 From: Mario Santos Date: Mon, 2 Sep 2024 12:03:03 +0200 Subject: [PATCH 1/6] Add warning for invalid binding --- .../src/hooks/use-bindings-attributes.js | 53 +++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/hooks/use-bindings-attributes.js b/packages/block-editor/src/hooks/use-bindings-attributes.js index e1ebf5fda6b8e..7396e5dcac546 100644 --- a/packages/block-editor/src/hooks/use-bindings-attributes.js +++ b/packages/block-editor/src/hooks/use-bindings-attributes.js @@ -2,17 +2,21 @@ * WordPress dependencies */ import { store as blocksStore } from '@wordpress/blocks'; +import { Button } from '@wordpress/components'; import { createHigherOrderComponent } from '@wordpress/compose'; import { useRegistry, useSelect } from '@wordpress/data'; import { useCallback, useMemo, useContext } from '@wordpress/element'; import { addFilter } from '@wordpress/hooks'; +import { __, sprintf } from '@wordpress/i18n'; /** * Internal dependencies */ import isURLLike from '../components/link-control/is-url-like'; import { unlock } from '../lock-unlock'; +import { Warning } from '../components'; import BlockContext from '../components/block-context'; +import { useBlockBindingsUtils } from '../utils/block-bindings'; /** @typedef {import('@wordpress/compose').WPHigherOrderComponent} WPHigherOrderComponent */ /** @typedef {import('@wordpress/blocks').WPBlockSettings} WPBlockSettings */ @@ -103,6 +107,7 @@ export const withBlockBindingSupport = createHigherOrderComponent( const sources = useSelect( ( select ) => unlock( select( blocksStore ) ).getAllBlockBindingsSources() ); + const { updateBlockBindings } = useBlockBindingsUtils(); const { name, clientId } = props; const hasParentPattern = !! props.context[ 'pattern/overrides' ]; const hasPatternOverridesDefaultBinding = @@ -121,9 +126,9 @@ export const withBlockBindingSupport = createHigherOrderComponent( // used purposely here to ensure `boundAttributes` is updated whenever // there are attribute updates. // `source.getValues` may also call a selector via `registry.select`. - const boundAttributes = useSelect( () => { + const { boundAttributes, invalidBinding } = useSelect( () => { if ( ! blockBindings ) { - return; + return {}; } const attributes = {}; @@ -135,7 +140,15 @@ export const withBlockBindingSupport = createHigherOrderComponent( ) ) { const { source: sourceName, args: sourceArgs } = binding; const source = sources[ sourceName ]; - if ( ! source || ! canBindAttribute( name, attributeName ) ) { + if ( ! source ) { + return { + invalidBinding: { + source: sourceName, + attribute: attributeName, + }, + }; + } + if ( ! canBindAttribute( name, attributeName ) ) { continue; } @@ -190,7 +203,7 @@ export const withBlockBindingSupport = createHigherOrderComponent( } } - return attributes; + return { boundAttributes: attributes }; }, [ blockBindings, name, clientId, blockContext, registry, sources ] ); const { setAttributes } = props; @@ -286,6 +299,38 @@ export const withBlockBindingSupport = createHigherOrderComponent( ] ); + // Throw a warning if the block is connected to an invalid source. + if ( invalidBinding ) { + const removeAllBindingsButton = ( + + ); + return ( +
+ + { sprintf( + /* translators: %1$s: block attribute, %2$s: invalid block bindings source. */ + __( + 'Attribute "%1$s" is connected to undefined "%2$s" block bindings source. You can leave this block intact, modify the connection, or remove it.' + ), + invalidBinding.attribute, + invalidBinding.source + ) } + +
+ ); + } + return ( <> Date: Mon, 2 Sep 2024 12:03:03 +0200 Subject: [PATCH 2/6] Adapt e2e tests --- .../editor/various/block-bindings.spec.js | 422 +----------------- 1 file changed, 4 insertions(+), 418 deletions(-) diff --git a/test/e2e/specs/editor/various/block-bindings.spec.js b/test/e2e/specs/editor/various/block-bindings.spec.js index 3fa17bdf6163a..d1ea750e01e50 100644 --- a/test/e2e/specs/editor/various/block-bindings.spec.js +++ b/test/e2e/specs/editor/various/block-bindings.spec.js @@ -159,9 +159,8 @@ test.describe( 'Block bindings', () => { ); } ); - test( 'should lock the appropriate controls when source is not defined', async ( { + test( 'should throw a warning when source is not defined', async ( { editor, - page, } ) => { await editor.insertBlock( { name: 'core/paragraph', @@ -177,32 +176,10 @@ test.describe( 'Block bindings', () => { }, }, } ); - const paragraphBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Paragraph', - } ); - await paragraphBlock.click(); - - // Alignment controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Align text' } ) - ).toBeVisible(); - - // Format controls don't exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Bold', - } ) - ).toBeHidden(); - - // Paragraph is not editable. - await expect( paragraphBlock ).toHaveAttribute( - 'contenteditable', - 'false' + const warningMessage = editor.canvas.locator( + '.block-editor-warning__message' ); + await expect( warningMessage ).toBeVisible(); } ); } ); @@ -275,52 +252,6 @@ test.describe( 'Block bindings', () => { 'false' ); } ); - - test( 'should lock the appropriate controls when source is not defined', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/heading', - attributes: { - content: 'heading default content', - metadata: { - bindings: { - content: { - source: 'plugin/undefined-source', - args: { key: 'text_custom_field' }, - }, - }, - }, - }, - } ); - const headingBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Heading', - } ); - await headingBlock.click(); - - // Alignment controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Align text' } ) - ).toBeVisible(); - - // Format controls don't exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Bold', - } ) - ).toBeHidden(); - - // Heading is not editable. - await expect( headingBlock ).toHaveAttribute( - 'contenteditable', - 'false' - ); - } ); } ); test.describe( 'Button', () => { @@ -416,68 +347,6 @@ test.describe( 'Block bindings', () => { ).toBeVisible(); } ); - test( 'should lock text controls when text is bound to an undefined source', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/buttons', - innerBlocks: [ - { - name: 'core/button', - attributes: { - text: 'button default text', - url: '#default-url', - metadata: { - bindings: { - text: { - source: 'plugin/undefined-source', - args: { key: 'text_custom_field' }, - }, - }, - }, - }, - }, - ], - } ); - const buttonBlock = editor.canvas - .getByRole( 'document', { - name: 'Block: Button', - exact: true, - } ) - .getByRole( 'textbox' ); - await buttonBlock.click(); - - // Alignment controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Align text' } ) - ).toBeVisible(); - - // Format controls don't exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Bold', - } ) - ).toBeHidden(); - - // Button is not editable. - await expect( buttonBlock ).toHaveAttribute( - 'contenteditable', - 'false' - ); - - // Link controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Unlink' } ) - ).toBeVisible(); - } ); - test( 'should lock url controls when url is bound to a registered source', async ( { editor, page, @@ -538,66 +407,6 @@ test.describe( 'Block bindings', () => { ).toBeHidden(); } ); - test( 'should lock url controls when url is bound to an undefined source', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/buttons', - innerBlocks: [ - { - name: 'core/button', - attributes: { - text: 'button default text', - url: '#default-url', - metadata: { - bindings: { - url: { - source: 'plugin/undefined-source', - args: { key: 'url_custom_field' }, - }, - }, - }, - }, - }, - ], - } ); - const buttonBlock = editor.canvas - .getByRole( 'document', { - name: 'Block: Button', - exact: true, - } ) - .getByRole( 'textbox' ); - await buttonBlock.click(); - - // Format controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Bold', - } ) - ).toBeVisible(); - - // Button is editable. - await expect( buttonBlock ).toHaveAttribute( - 'contenteditable', - 'true' - ); - - // Link controls don't exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Link' } ) - ).toBeHidden(); - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { name: 'Unlink' } ) - ).toBeHidden(); - } ); - test( 'should lock url and text controls when both are bound', async ( { editor, page, @@ -712,34 +521,6 @@ test.describe( 'Block bindings', () => { ).toBeHidden(); } ); - test( 'should NOT show the upload form when url is bound to an undefined source', async ( { - editor, - } ) => { - await editor.insertBlock( { - name: 'core/image', - attributes: { - url: imagePlaceholderSrc, - alt: 'default alt value', - title: 'default title value', - metadata: { - bindings: { - url: { - source: 'plugin/undefined-source', - args: { key: 'url_custom_field' }, - }, - }, - }, - }, - } ); - const imageBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Image', - } ); - await imageBlock.click(); - await expect( - imageBlock.getByRole( 'button', { name: 'Upload' } ) - ).toBeHidden(); - } ); - test( 'should lock url controls when url is bound to a registered source', async ( { editor, page, @@ -809,75 +590,6 @@ test.describe( 'Block bindings', () => { expect( titleValue ).toBe( 'default title value' ); } ); - test( 'should lock url controls when url is bound to an undefined source', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/image', - attributes: { - url: imagePlaceholderSrc, - alt: 'default alt value', - title: 'default title value', - metadata: { - bindings: { - url: { - source: 'plugin/undefined-source', - args: { key: 'url_custom_field' }, - }, - }, - }, - }, - } ); - const imageBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Image', - } ); - await imageBlock.click(); - - // Replace controls don't exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Replace', - } ) - ).toBeHidden(); - - // Image placeholder doesn't show the upload button. - await expect( - imageBlock.getByRole( 'button', { name: 'Upload' } ) - ).toBeHidden(); - - // Alt textarea is enabled and with the original value. - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - ).toBeEnabled(); - const altValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - .inputValue(); - expect( altValue ).toBe( 'default alt value' ); - - // Title input is enabled and with the original value. - await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByRole( 'button', { name: 'Advanced' } ) - .click(); - - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - ).toBeEnabled(); - const titleValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - .inputValue(); - expect( titleValue ).toBe( 'default title value' ); - } ); - test( 'should disable alt textarea when alt is bound to a registered source', async ( { editor, page, @@ -941,69 +653,6 @@ test.describe( 'Block bindings', () => { expect( titleValue ).toBe( 'default title value' ); } ); - test( 'should disable alt textarea when alt is bound to an undefined source', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/image', - attributes: { - url: imagePlaceholderSrc, - alt: 'default alt value', - title: 'default title value', - metadata: { - bindings: { - alt: { - source: 'plguin/undefined-source', - args: { key: 'text_custom_field' }, - }, - }, - }, - }, - } ); - const imageBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Image', - } ); - await imageBlock.click(); - - // Replace controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Replace', - } ) - ).toBeVisible(); - - // Alt textarea is disabled and with the custom field value. - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - ).toHaveAttribute( 'readonly' ); - const altValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - .inputValue(); - expect( altValue ).toBe( 'default alt value' ); - - // Title input is enabled and with the original value. - await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByRole( 'button', { name: 'Advanced' } ) - .click(); - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - ).toBeEnabled(); - const titleValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - .inputValue(); - expect( titleValue ).toBe( 'default title value' ); - } ); - test( 'should disable title input when title is bound to a registered source', async ( { editor, page, @@ -1067,69 +716,6 @@ test.describe( 'Block bindings', () => { expect( titleValue ).toBe( 'text_custom_field' ); } ); - test( 'should disable title input when title is bound to an undefined source', async ( { - editor, - page, - } ) => { - await editor.insertBlock( { - name: 'core/image', - attributes: { - url: imagePlaceholderSrc, - alt: 'default alt value', - title: 'default title value', - metadata: { - bindings: { - title: { - source: 'plugin/undefined-source', - args: { key: 'text_custom_field' }, - }, - }, - }, - }, - } ); - const imageBlock = editor.canvas.getByRole( 'document', { - name: 'Block: Image', - } ); - await imageBlock.click(); - - // Replace controls exist. - await expect( - page - .getByRole( 'toolbar', { name: 'Block tools' } ) - .getByRole( 'button', { - name: 'Replace', - } ) - ).toBeVisible(); - - // Alt textarea is enabled and with the original value. - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - ).toBeEnabled(); - const altValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Alternative text' ) - .inputValue(); - expect( altValue ).toBe( 'default alt value' ); - - // Title input is disabled and with the custom field value. - await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByRole( 'button', { name: 'Advanced' } ) - .click(); - await expect( - page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - ).toHaveAttribute( 'readonly' ); - const titleValue = await page - .getByRole( 'tabpanel', { name: 'Settings' } ) - .getByLabel( 'Title attribute' ) - .inputValue(); - expect( titleValue ).toBe( 'default title value' ); - } ); - test( 'Multiple bindings should lock the appropriate controls', async ( { editor, page, From 0e370aeb009532db2e0c1cf51108cc159a6aacc2 Mon Sep 17 00:00:00 2001 From: Mario Santos Date: Mon, 2 Sep 2024 12:05:50 +0200 Subject: [PATCH 3/6] Change `removeInvalidBindingButton` variable name --- packages/block-editor/src/hooks/use-bindings-attributes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/hooks/use-bindings-attributes.js b/packages/block-editor/src/hooks/use-bindings-attributes.js index 7396e5dcac546..845cf3d77bc5b 100644 --- a/packages/block-editor/src/hooks/use-bindings-attributes.js +++ b/packages/block-editor/src/hooks/use-bindings-attributes.js @@ -301,7 +301,7 @@ export const withBlockBindingSupport = createHigherOrderComponent( // Throw a warning if the block is connected to an invalid source. if ( invalidBinding ) { - const removeAllBindingsButton = ( + const removeInvalidBindingButton = ( ); return ( -
+
{ sprintf( /* translators: %1$s: block attribute, %2$s: invalid block bindings source. */ __( - 'Attribute "%1$s" is connected to unrecognized "%2$s" source. You can leave this block intact, modify the connection, or remove it.' + 'Attribute "%1$s" is connected to unrecognized "%2$s" source.' ), invalidBinding.attribute, invalidBinding.source From f48c78e9ed77aec584a1cc8d551f8084431c0b98 Mon Sep 17 00:00:00 2001 From: Mario Santos Date: Tue, 3 Sep 2024 10:42:48 +0200 Subject: [PATCH 6/6] Try: Remove button --- .../src/hooks/use-bindings-attributes.js | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/packages/block-editor/src/hooks/use-bindings-attributes.js b/packages/block-editor/src/hooks/use-bindings-attributes.js index 573636fa6db4b..3bf78226f2264 100644 --- a/packages/block-editor/src/hooks/use-bindings-attributes.js +++ b/packages/block-editor/src/hooks/use-bindings-attributes.js @@ -2,7 +2,6 @@ * WordPress dependencies */ import { store as blocksStore } from '@wordpress/blocks'; -import { Button } from '@wordpress/components'; import { createHigherOrderComponent } from '@wordpress/compose'; import { useRegistry, useSelect } from '@wordpress/data'; import { useCallback, useMemo, useContext } from '@wordpress/element'; @@ -16,7 +15,6 @@ import isURLLike from '../components/link-control/is-url-like'; import { unlock } from '../lock-unlock'; import { Warning, useBlockProps } from '../components'; import BlockContext from '../components/block-context'; -import { useBlockBindingsUtils } from '../utils/block-bindings'; /** @typedef {import('@wordpress/compose').WPHigherOrderComponent} WPHigherOrderComponent */ /** @typedef {import('@wordpress/blocks').WPBlockSettings} WPBlockSettings */ @@ -107,7 +105,6 @@ export const withBlockBindingSupport = createHigherOrderComponent( const sources = useSelect( ( select ) => unlock( select( blocksStore ) ).getAllBlockBindingsSources() ); - const { updateBlockBindings } = useBlockBindingsUtils(); const { name, clientId } = props; const hasParentPattern = !! props.context[ 'pattern/overrides' ]; const hasPatternOverridesDefaultBinding = @@ -301,23 +298,9 @@ export const withBlockBindingSupport = createHigherOrderComponent( // Throw a warning if the block is connected to an invalid source. if ( invalidBinding ) { - const removeInvalidBindingButton = ( - - ); return (
- + { sprintf( /* translators: %1$s: block attribute, %2$s: invalid block bindings source. */ __(