Link Control require user to manually submit any changes (#50668)

* Implement initial shallow checking

* Don’t assyne value when comparing value changes

* Fix recrusive re-renders in hook

Avoid passing a default when default is handled internally. Set default to be an object as that is what we are tracking.

* Fix ENTER submission bug

* Require settings changes to be “Applied”

* Refactor for readability and tidy

* Improve test naming

* Add test for new UX

* Improve fetching of settings

* Rename hook to convey new purpose

* Improve test coverage of test

* Fix tab stops now Apply is disabled when there are no changes

* Wait for settings drawer to open

* Extract retrival of settings keys

* Move setters to hook

* Make API clearer to consumers of hook

packages/block-editor/src/components/link-control/index.js

@@ -11,6 +11,7 @@ import { __ } from '@wordpress/i18n';
 import { useRef, useState, useEffect } from '@wordpress/element';
 import { focus } from '@wordpress/dom';
 import { ENTER } from '@wordpress/keycodes';
+import { isShallowEqualObjects } from '@wordpress/is-shallow-equal';
 
 /**
  * Internal dependencies
@@ -19,7 +20,7 @@ import LinkControlSettingsDrawer from './settings-drawer';
 import LinkControlSearchInput from './search-input';
 import LinkPreview from './link-preview';
 import useCreatePage from './use-create-page';
-import useInternalInputValue from './use-internal-input-value';
+import useInternalValue from './use-internal-value';
 import { ViewerFill } from './viewer-slot';
 import { DEFAULT_LINK_SETTINGS } from './constants';
 
@@ -136,13 +137,20 @@ function LinkControl( {
 	const textInputRef = useRef();
 	const isEndingEditWithFocus = useRef( false );
 
+	const settingsKeys = settings.map( ( { id } ) => id );
+
 	const [ settingsOpen, setSettingsOpen ] = useState( false );
 
-	const [ internalUrlInputValue, setInternalUrlInputValue ] =
-		useInternalInputValue( value?.url || '' );
+	const [
+		internalControlValue,
+		setInternalControlValue,
+		setInternalURLInputValue,
+		setInternalTextInputValue,
+		createSetInternalSettingValueHandler,
+	] = useInternalValue( value );
 
-	const [ internalTextInputValue, setInternalTextInputValue ] =
-		useInternalInputValue( value?.title || '' );
+	const valueHasChanges =
+		value && ! isShallowEqualObjects( internalControlValue, value );
 
 	const [ isEditingLink, setIsEditingLink ] = useState(
 		forceIsEditingLink !== undefined
@@ -160,6 +168,8 @@ function LinkControl( {
 		) {
 			setIsEditingLink( forceIsEditingLink );
 		}
+		// Todo: bug if the missing dep is introduced. Will need a fix.
+		// eslint-disable-next-line react-hooks/exhaustive-deps
 	}, [ forceIsEditingLink ] );
 
 	useEffect( () => {
@@ -208,29 +218,47 @@ function LinkControl( {
 	};
 
 	const handleSelectSuggestion = ( updatedValue ) => {
+		// Suggestions may contains "settings" values (e.g. `opensInNewTab`)
+		// which should not overide any existing settings values set by the
+		// user. This filters out any settings values from the suggestion.
+		const nonSettingsChanges = Object.keys( updatedValue ).reduce(
+			( acc, key ) => {
+				if ( ! settingsKeys.includes( key ) ) {
+					acc[ key ] = updatedValue[ key ];
+				}
+				return acc;
+			},
+			{}
+		);
+
 		onChange( {
-			...updatedValue,
-			title: internalTextInputValue || updatedValue?.title,
+			...internalControlValue,
+			...nonSettingsChanges,
+			// As title is not a setting, it must be manually applied
+			// in such a way as to preserve the users changes over
+			// any "title" value provided by the "suggestion".
+			title: internalControlValue?.title || updatedValue?.title,
 		} );
+
 		stopEditing();
 	};
 
 	const handleSubmit = () => {
-		if (
-			currentUrlInputValue !== value?.url ||
-			internalTextInputValue !== value?.title
-		) {
+		if ( valueHasChanges ) {
+			// Submit the original value with new stored values applied
+			// on top. URL is a special case as it may also be a prop.
 			onChange( {
 				...value,
+				...internalControlValue,
 				url: currentUrlInputValue,
-				title: internalTextInputValue,
 			} );
 		}
 		stopEditing();
 	};
 
 	const handleSubmitWithEnter = ( event ) => {
 		const { keyCode } = event;
+
 		if (
 			keyCode === ENTER &&
 			! currentInputIsEmpty // Disallow submitting empty values.
@@ -241,8 +269,7 @@ function LinkControl( {
 	};
 
 	const resetInternalValues = () => {
-		setInternalUrlInputValue( value?.url );
-		setInternalTextInputValue( value?.title );
+		setInternalControlValue( value );
 	};
 
 	const handleCancel = ( event ) => {
@@ -263,7 +290,8 @@ function LinkControl( {
 		onCancel?.();
 	};
 
-	const currentUrlInputValue = propInputValue || internalUrlInputValue;
+	const currentUrlInputValue =
+		propInputValue || internalControlValue?.url || '';
 
 	const currentInputIsEmpty = ! currentUrlInputValue?.trim()?.length;
 
@@ -306,7 +334,7 @@ function LinkControl( {
 							value={ currentUrlInputValue }
 							withCreateSuggestion={ withCreateSuggestion }
 							onCreateSuggestion={ createPage }
-							onChange={ setInternalUrlInputValue }
+							onChange={ setInternalURLInputValue }
 							onSelect={ handleSelectSuggestion }
 							showInitialSuggestions={ showInitialSuggestions }
 							allowDirectEntry={ ! noDirectEntry }
@@ -351,14 +379,18 @@ function LinkControl( {
 							showTextControl={ showTextControl }
 							showSettings={ showSettings }
 							textInputRef={ textInputRef }
-							internalTextInputValue={ internalTextInputValue }
+							internalTextInputValue={
+								internalControlValue?.title
+							}
 							setInternalTextInputValue={
 								setInternalTextInputValue
 							}
 							handleSubmitWithEnter={ handleSubmitWithEnter }
-							value={ value }
+							value={ internalControlValue }
 							settings={ settings }
-							onChange={ onChange }
+							onChange={ createSetInternalSettingValueHandler(
+								settingsKeys
+							) }
 						/>
 					) }
 
@@ -367,7 +399,9 @@ function LinkControl( {
 							variant="primary"
 							onClick={ handleSubmit }
 							className="block-editor-link-control__search-submit"
-							disabled={ currentInputIsEmpty } // Disallow submitting empty values.
+							disabled={
+								! valueHasChanges || currentInputIsEmpty
+							}
 						>
 							{ __( 'Apply' ) }
 						</Button>

packages/block-editor/src/components/link-control/test/index.js

@@ -1784,6 +1784,63 @@ describe( 'Addition Settings UI', () => {
 			} )
 		).toBeChecked();
 	} );
+
+	it( 'should require settings changes to be submitted/applied', async () => {
+		const user = userEvent.setup();
+
+		const mockOnChange = jest.fn();
+
+		const selectedLink = {
+			...fauxEntitySuggestions[ 0 ],
+			// Including a setting here helps to assert on a potential bug
+			// whereby settings on the suggestion override the current (internal)
+			// settings values set by the user in the UI.
+			opensInNewTab: false,
+		};
+
+		render(
+			<LinkControl
+				value={ selectedLink }
+				forceIsEditingLink
+				hasTextControl
+				onChange={ mockOnChange }
+			/>
+		);
+
+		// check that the "Apply" button is disabled by default.
+		const submitButton = screen.queryByRole( 'button', {
+			name: 'Apply',
+		} );
+
+		expect( submitButton ).toBeDisabled();
+
+		await toggleSettingsDrawer( user );
+
+		const opensInNewTabToggle = screen.queryByRole( 'checkbox', {
+			name: 'Open in new tab',
+		} );
+
+		// toggle the checkbox
+		await user.click( opensInNewTabToggle );
+
+		// Check settings are **not** directly submitted
+		// which would trigger the onChange handler.
+		expect( mockOnChange ).not.toHaveBeenCalled();
+
+		// Check Apply button is now enabled because changes
+		// have been detected.
+		expect( submitButton ).toBeEnabled();
+
+		// Submit the changed setting value using the Apply button
+		await user.click( submitButton );
+
+		// Assert the value is updated.
+		expect( mockOnChange ).toHaveBeenCalledWith(
+			expect.objectContaining( {
+				opensInNewTab: true,
+			} )
+		);
+	} );
 } );
 
 describe( 'Post types', () => {
@@ -2199,7 +2256,7 @@ describe( 'Controlling link title text', () => {
 
 	it( 'should allow `ENTER` keypress within the text field to trigger submission of value', async () => {
 		const user = userEvent.setup();
-		const textValue = 'My new text value';
+		const newTextValue = 'My new text value';
 		const mockOnChange = jest.fn();
 
 		render(
@@ -2218,14 +2275,14 @@ describe( 'Controlling link title text', () => {
 		expect( textInput ).toBeVisible();
 
 		await user.clear( textInput );
-		await user.keyboard( textValue );
+		await user.keyboard( newTextValue );
 
 		// Attempt to submit the empty search value in the input.
 		triggerEnter( textInput );
 
 		expect( mockOnChange ).toHaveBeenCalledWith(
 			expect.objectContaining( {
-				title: textValue,
+				title: newTextValue,
 				url: selectedLink.url,
 			} )
 		);
@@ -2236,7 +2293,7 @@ describe( 'Controlling link title text', () => {
 		).not.toBeInTheDocument();
 	} );
 
-	it( 'should reset state on value change', async () => {
+	it( 'should reset state upon controlled value change', async () => {
 		const user = userEvent.setup();
 		const textValue = 'My new text value';
 		const mockOnChange = jest.fn();

packages/block-editor/src/components/link-control/use-internal-value.js

@@ -0,0 +1,60 @@
+/**
+ * WordPress dependencies
+ */
+import { useState, useEffect } from '@wordpress/element';
+
+export default function useInternalValue( value ) {
+	const [ internalValue, setInternalValue ] = useState( value || {} );
+
+	// If the value prop changes, update the internal state.
+	useEffect( () => {
+		setInternalValue( ( prevValue ) => {
+			if ( value && value !== prevValue ) {
+				return value;
+			}
+
+			return prevValue;
+		} );
+	}, [ value ] );
+
+	const setInternalURLInputValue = ( nextValue ) => {
+		setInternalValue( {
+			...internalValue,
+			url: nextValue,
+		} );
+	};
+
+	const setInternalTextInputValue = ( nextValue ) => {
+		setInternalValue( {
+			...internalValue,
+			title: nextValue,
+		} );
+	};
+
+	const createSetInternalSettingValueHandler =
+		( settingsKeys ) => ( nextValue ) => {
+			// Only apply settings values which are defined in the settings prop.
+			const settingsUpdates = Object.keys( nextValue ).reduce(
+				( acc, key ) => {
+					if ( settingsKeys.includes( key ) ) {
+						acc[ key ] = nextValue[ key ];
+					}
+					return acc;
+				},
+				{}
+			);
+
+			setInternalValue( {
+				...internalValue,
+				...settingsUpdates,
+			} );
+		};
+
+	return [
+		internalValue,
+		setInternalValue,
+		setInternalURLInputValue,
+		setInternalTextInputValue,
+		createSetInternalSettingValueHandler,
+	];
+}

packages/e2e-tests/specs/editor/various/links.test.js

@@ -792,8 +792,11 @@ describe( 'Links', () => {
 			);
 			await settingsToggle.click();
 
+			// Wait for settings to open.
+			await page.waitForXPath( `//label[text()='Open in new tab']` );
+
 			// Move focus back to RichText for the underlying link.
-			await pressKeyTimes( 'Tab', 5 );
+			await pressKeyTimes( 'Tab', 4 );
 
 			// Make a selection within the RichText.
 			await pressKeyWithModifier( 'shift', 'ArrowRight' );