From ba931e67768ba758f7e5f15ea721d89baa0d84ba Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 8 Oct 2025 09:47:32 +0100 Subject: [PATCH 1/2] Navigation Link: Fix custom links creating empty block bindings When manually inserting a Custom Link variation of the Navigation Link block, empty block bindings were being created. This occurred because the binding logic only checked for the presence of an 'id' attribute without verifying whether the link was actually an entity link vs a custom link. This commit refactors the binding logic to be based on the final computed state from updateAttributes rather than the input values, preventing the mismatch and ensuring custom links never create bindings. Changes: - Refactor updateAttributes to return metadata about final state - Update onChange handlers to use returned isEntityLink flag - Add comprehensive test coverage for return value metadata - Ensure binding decisions align with computed attributes Fixes the bug where Custom Link variations would incorrectly create bindings even though they have kind: 'custom' and should remain unbound. --- .../block-library/src/navigation-link/edit.js | 13 +- .../shared/test/update-attributes.test.js | 171 ++++++++++++++++++ .../shared/update-attributes.js | 15 ++ .../src/navigation-submenu/edit.js | 13 +- 4 files changed, 202 insertions(+), 10 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index a490ee94e77076..449f1ded557a7d 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -553,17 +553,20 @@ export default function NavigationLinkEdit( { anchor={ popoverAnchor } onRemove={ removeLink } onChange={ ( updatedValue ) => { - updateAttributes( + // updateAttributes determines the final state and returns metadata + const { isEntityLink } = updateAttributes( updatedValue, setAttributes, attributes ); - // Handle URL binding - if ( ! updatedValue?.id ) { - clearBinding(); - } else { + // Handle URL binding based on the final computed state + // Only create bindings for entity links (posts, pages, taxonomies) + // Never create bindings for custom links (manual URLs) + if ( isEntityLink ) { createBinding(); + } else { + clearBinding(); } } } /> diff --git a/packages/block-library/src/navigation-link/shared/test/update-attributes.test.js b/packages/block-library/src/navigation-link/shared/test/update-attributes.test.js index 6cb52a6162ac68..9337028e96f75f 100644 --- a/packages/block-library/src/navigation-link/shared/test/update-attributes.test.js +++ b/packages/block-library/src/navigation-link/shared/test/update-attributes.test.js @@ -1133,4 +1133,175 @@ describe( 'updateAttributes', () => { ); } ); } ); + + describe( 'Return value metadata', () => { + describe( 'isEntityLink', () => { + it( 'should return true for entity links with id and non-custom kind', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + id: 123, + kind: 'post-type', + type: 'page', + url: 'https://example.com/page', + title: 'Test Page', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + expect( result ).toEqual( { + isEntityLink: true, + } ); + } ); + + it( 'should return false for custom links even with id', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + id: 123, + kind: 'custom', + type: 'custom', + url: 'https://example.com/custom', + title: 'Custom Link', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + expect( result ).toEqual( { + isEntityLink: false, + } ); + } ); + + it( 'should return false for links without id', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + url: 'https://example.com', + title: 'Example', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + expect( result ).toEqual( { + isEntityLink: false, + } ); + } ); + + it( 'should return false when entity link is severed', () => { + const setAttributes = jest.fn(); + const blockAttributes = { + id: 123, + type: 'page', + kind: 'post-type', + url: 'https://example.com/original-page', + }; + + const updatedValue = { + url: 'https://example.com/different-page', + }; + + const result = updateAttributes( + updatedValue, + setAttributes, + blockAttributes + ); + + // Should return false because the link was severed and converted to custom + expect( result ).toEqual( { + isEntityLink: false, + } ); + } ); + + it( 'should return true when entity link is preserved through query string change', () => { + const setAttributes = jest.fn(); + const blockAttributes = { + id: 123, + type: 'page', + kind: 'post-type', + url: 'https://example.com/page', + }; + + const updatedValue = { + url: 'https://example.com/page?foo=bar', + }; + + const result = updateAttributes( + updatedValue, + setAttributes, + blockAttributes + ); + + // Should return true because entity link is preserved + expect( result ).toEqual( { + isEntityLink: true, + } ); + } ); + + it( 'should return false for mailto links', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + id: 'mailto:test@example.com', + type: 'mailto', + url: 'mailto:test@example.com', + title: 'mailto:test@example.com', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + // mailto links have kind: 'custom', so isEntityLink should be false + expect( result ).toEqual( { + isEntityLink: false, + } ); + } ); + + it( 'should return false for tel links', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + id: 'tel:5555555', + type: 'tel', + url: 'tel:5555555', + title: 'tel:5555555', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + // tel links have kind: 'custom', so isEntityLink should be false + expect( result ).toEqual( { + isEntityLink: false, + } ); + } ); + + it( 'should return true for taxonomy links', () => { + const setAttributes = jest.fn(); + const linkSuggestion = { + id: 5, + kind: 'taxonomy', + type: 'category', + url: 'https://example.com/category/news', + title: 'News', + }; + + const result = updateAttributes( + linkSuggestion, + setAttributes + ); + + expect( result ).toEqual( { + isEntityLink: true, + } ); + } ); + } ); + } ); } ); diff --git a/packages/block-library/src/navigation-link/shared/update-attributes.js b/packages/block-library/src/navigation-link/shared/update-attributes.js index 28fc2269b7af93..067e71ddf1cb16 100644 --- a/packages/block-library/src/navigation-link/shared/update-attributes.js +++ b/packages/block-library/src/navigation-link/shared/update-attributes.js @@ -208,4 +208,19 @@ export const updateAttributes = ( } setAttributes( attributes ); + + // Return metadata about the final state for binding decisions. + // We need to distinguish between: + // 1. Property not set in attributes (use blockAttributes fallback) + // 2. Property explicitly set to undefined (means "remove this") + // Using 'in' operator checks if property exists, even if undefined. + // This is critical for severing: attributes.id = undefined means "remove the ID", + // not "keep the old ID from blockAttributes". + const finalId = 'id' in attributes ? attributes.id : blockAttributes.id; + const finalKind = + 'kind' in attributes ? attributes.kind : blockAttributes.kind; + + return { + isEntityLink: !! finalId && finalKind !== 'custom', + }; }; diff --git a/packages/block-library/src/navigation-submenu/edit.js b/packages/block-library/src/navigation-submenu/edit.js index be38a0740e7e7c..54740c086bcf84 100644 --- a/packages/block-library/src/navigation-submenu/edit.js +++ b/packages/block-library/src/navigation-submenu/edit.js @@ -435,17 +435,20 @@ export default function NavigationSubmenuEdit( { speak( __( 'Link removed.' ), 'assertive' ); } } onChange={ ( updatedValue ) => { - updateAttributes( + // updateAttributes determines the final state and returns metadata + const { isEntityLink } = updateAttributes( updatedValue, setAttributes, attributes ); - // Handle URL binding - if ( ! updatedValue?.id ) { - clearBinding(); - } else { + // Handle URL binding based on the final computed state + // Only create bindings for entity links (posts, pages, taxonomies) + // Never create bindings for custom links (manual URLs) + if ( isEntityLink ) { createBinding(); + } else { + clearBinding(); } } } /> From 800ec2cdd0f10eacda041dae517c4d14335f56d6 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 8 Oct 2025 12:15:07 +0100 Subject: [PATCH 2/2] Fix: Use isEntityLink metadata for binding decisions in Navigation blocks - Updated Navigation Link and Navigation Submenu edit components to use isEntityLink metadata from updateAttributes - Fixed menu-inspector-controls to use useEntityBinding hook for consistency - Ensures bindings are only created for entity links (posts, pages, taxonomies) - Prevents empty bindings from being created for custom links - Fixes bug where severing entity links would incorrectly retain bindings --- .../block-library/src/navigation-link/edit.js | 1 - .../edit/menu-inspector-controls.js | 25 +++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 449f1ded557a7d..06e6878a3bae9b 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -553,7 +553,6 @@ export default function NavigationLinkEdit( { anchor={ popoverAnchor } onRemove={ removeLink } onChange={ ( updatedValue ) => { - // updateAttributes determines the final state and returns metadata const { isEntityLink } = updateAttributes( updatedValue, setAttributes, diff --git a/packages/block-library/src/navigation/edit/menu-inspector-controls.js b/packages/block-library/src/navigation/edit/menu-inspector-controls.js index 96356b0b29f243..5302acfe92d101 100644 --- a/packages/block-library/src/navigation/edit/menu-inspector-controls.js +++ b/packages/block-library/src/navigation/edit/menu-inspector-controls.js @@ -23,7 +23,11 @@ import { unlock } from '../../lock-unlock'; import DeletedNavigationWarning from './deleted-navigation-warning'; import useNavigationMenu from '../use-navigation-menu'; import LeafMoreMenu from './leaf-more-menu'; -import { LinkUI, updateAttributes } from '../../navigation-link/shared'; +import { + LinkUI, + updateAttributes, + useEntityBinding, +} from '../../navigation-link/shared'; const actionLabel = /* translators: %s: The name of a menu. */ __( "Switch to '%s'" ); @@ -43,6 +47,12 @@ function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) { const blockWasJustInserted = insertedBlock?.clientId === block.clientId; const showLinkControls = supportsLinkControls && blockWasJustInserted; + // Get binding utilities for the inserted block + const { createBinding, clearBinding } = useEntityBinding( { + clientId: insertedBlock?.clientId, + attributes: insertedBlock?.attributes || {}, + } ); + if ( ! showLinkControls ) { return null; } @@ -100,11 +110,22 @@ function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) { cleanupInsertedBlock(); } } onChange={ ( updatedValue ) => { - updateAttributes( + // updateAttributes determines the final state and returns metadata + const { isEntityLink } = updateAttributes( updatedValue, setInsertedBlockAttributes( insertedBlock?.clientId ), insertedBlock?.attributes ); + + // Handle URL binding based on the final computed state + // Only create bindings for entity links (posts, pages, taxonomies) + // Never create bindings for custom links (manual URLs) + if ( isEntityLink ) { + createBinding(); + } else { + clearBinding(); + } + setInsertedBlock( null ); } } />