Fix empty url value from unbinding entity from inspector sidebar (#72447)

When unsyncing a navigation link url, we now set the url as the synced entity url to avoid empty url states which cause bugs in the canvas. We also defer updating the url value in the store until the blur event to avoid the canvas receiving a temporarily empty value (such as when a user clears the URL in order to start typing a new URL). 


----

Co-authored-by: jeryj <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: ockham <[email protected]>

Author: 4 people

packages/block-library/src/navigation-link/shared/controls.js

@@ -11,7 +11,7 @@ import {
 	TextareaControl,
 } from '@wordpress/components';
 import { __, sprintf } from '@wordpress/i18n';
-import { useRef, useEffect } from '@wordpress/element';
+import { useRef, useEffect, useState } from '@wordpress/element';
 import { useInstanceId } from '@wordpress/compose';
 import { safeDecodeURI } from '@wordpress/url';
 import { __unstableStripHTML as stripHTML } from '@wordpress/dom';
@@ -77,6 +77,15 @@ export function Controls( { attributes, setAttributes, clientId } ) {
 	const inputId = useInstanceId( Controls, 'link-input' );
 	const helpTextId = `${ inputId }__help`;
 
+	// Local state to control the input value
+	const [ inputValue, setInputValue ] = useState( url );
+
+	// Sync local state when url prop changes (e.g., from undo/redo or external updates)
+	useEffect( () => {
+		setInputValue( url );
+		lastURLRef.current = url;
+	}, [ url ] );
+
 	// Use the entity binding hook internally
 	const { hasUrlBinding, clearBinding } = useEntityBinding( {
 		clientId,
@@ -95,12 +104,19 @@ export function Controls( { attributes, setAttributes, clientId } ) {
 		// setAttributes is actually setBoundAttributes, a wrapper function that
 		// processes attributes through the binding system.
 		// See: packages/block-editor/src/components/block-edit/edit.js
-		updateBlockAttributes( clientId, { url: '', id: undefined } );
+		updateBlockAttributes( clientId, {
+			url: lastURLRef.current, // set the lastURLRef as the new editable value so we avoid bugs from empty link states
+			id: undefined,
+		} );
 	};
 
 	useEffect( () => {
+		// Checking for ! hasUrlBinding is a defensive check, as we would
+		// only want to focus the input if the url is not bound to an entity.
 		if ( ! hasUrlBinding && shouldFocusURLInputRef.current ) {
-			urlInputRef.current?.focus();
+			// focuses and highlights the url input value, giving the user
+			// the ability to delete the value quickly or edit it.
+			urlInputRef.current?.select();
 		}
 		shouldFocusURLInputRef.current = false;
 	}, [ hasUrlBinding ] );
@@ -149,18 +165,20 @@ export function Controls( { attributes, setAttributes, clientId } ) {
 					__next40pxDefaultSize
 					id={ inputId }
 					label={ __( 'Link' ) }
-					value={ url ? safeDecodeURI( url ) : '' }
-					onChange={ ( urlValue ) => {
-						if ( hasUrlBinding ) {
-							return; // Prevent editing when URL is bound
-						}
-						setAttributes( {
-							url: encodeURI( safeDecodeURI( urlValue ) ),
-						} );
-					} }
+					value={ inputValue ? safeDecodeURI( inputValue ) : '' }
 					autoComplete="off"
 					type="url"
 					disabled={ hasUrlBinding }
+					onChange={ ( newValue ) => {
+						if ( hasUrlBinding ) {
+							return;
+						}
+
+						// Defer updating the url attribute until onBlur to prevent the canvas from
+						// treating a temporary empty value as a committed value, which replaces the
+						// label with placeholder text.
+						setInputValue( newValue );
+					} }
 					onFocus={ () => {
 						if ( hasUrlBinding ) {
 							return;
@@ -171,12 +189,19 @@ export function Controls( { attributes, setAttributes, clientId } ) {
 						if ( hasUrlBinding ) {
 							return;
 						}
+
+						const finalValue = ! inputValue
+							? lastURLRef.current
+							: inputValue;
+
+						// Update local state immediately so input reflects the reverted value if the value was cleared
+						setInputValue( finalValue );
+
 						// Defer the updateAttributes call to ensure entity connection isn't severed by accident.
-						updateAttributes(
-							{ url: ! url ? lastURLRef.current : url },
-							setAttributes,
-							{ ...attributes, url: lastURLRef.current }
-						);
+						updateAttributes( { url: finalValue }, setAttributes, {
+							...attributes,
+							url: lastURLRef.current,
+						} );
 					} }
 					help={
 						hasUrlBinding && (

packages/block-library/src/navigation-link/shared/test/controls.js

@@ -6,6 +6,7 @@
  * External dependencies
  */
 import { render, screen, fireEvent } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 
 /**
  * Internal dependencies
@@ -95,18 +96,22 @@ describe( 'Controls', () => {
 		expect( urlInput.value ).toBe( 'https://example.com/test page' );
 	} );
 
-	it( 'encodes URL values when changed', () => {
+	it( 'calls updateAttributes with new URL on blur', async () => {
+		const user = userEvent.setup();
 		render( <Controls { ...defaultProps } /> );
 
 		const urlInput = screen.getByLabelText( 'Link' );
 
-		fireEvent.change( urlInput, {
-			target: { value: 'https://example.com/test page' },
-		} );
+		await user.click( urlInput );
+		await user.clear( urlInput );
+		await user.type( urlInput, 'https://example.com/test page' );
+		await user.tab();
 
-		expect( defaultProps.setAttributes ).toHaveBeenCalledWith( {
-			url: 'https://example.com/test%20page',
-		} );
+		expect( mockUpdateAttributes ).toHaveBeenCalledWith(
+			{ url: 'https://example.com/test page' },
+			defaultProps.setAttributes,
+			{ ...defaultProps.attributes, url: 'https://example.com' }
+		);
 	} );
 
 	it( 'calls updateAttributes on URL blur', () => {
@@ -143,11 +148,11 @@ describe( 'Controls', () => {
 			target: { value: 'https://new.com' },
 		} );
 
-		// Blur should call updateAttributes with the current URL (since url exists)
+		// Blur should call updateAttributes with the new URL value from the input
 		fireEvent.blur( urlInput );
 
 		expect( mockUpdateAttributes ).toHaveBeenCalledWith(
-			{ url: 'https://different.com' }, // Current URL from attributes (not input value)
+			{ url: 'https://new.com' }, // New URL from input value
 			defaultProps.setAttributes,
 			{
 				...propsWithDifferentUrl.attributes,

test/e2e/specs/editor/blocks/navigation.spec.js

@@ -1069,7 +1069,7 @@ test.describe( 'Navigation block', () => {
 				} );
 				await unlinkButton.click();
 				await expect( linkInput ).toBeEnabled();
-				await expect( linkInput ).toHaveValue( '' );
+				await expect( linkInput ).toHaveValue( updatedPage.link );
 				await expect( linkInput ).toBeFocused();
 			} );
 		} );