diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 142ffd6db1e50..bbb56ab72db8d 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/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,22 +218,39 @@ 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(); @@ -231,6 +258,7 @@ function LinkControl( { 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' ) } diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js index 4e2428fc0ac3f..5dfa5ad2f4587 100644 --- a/packages/block-editor/src/components/link-control/test/index.js +++ b/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( + + ); + + // 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(); diff --git a/packages/block-editor/src/components/link-control/use-internal-input-value.js b/packages/block-editor/src/components/link-control/use-internal-input-value.js deleted file mode 100644 index 5dd3c59f3e873..0000000000000 --- a/packages/block-editor/src/components/link-control/use-internal-input-value.js +++ /dev/null @@ -1,23 +0,0 @@ -/** - * WordPress dependencies - */ -import { useState, useEffect } from '@wordpress/element'; - -export default function useInternalInputValue( value ) { - const [ internalInputValue, setInternalInputValue ] = useState( - value || '' - ); - - // If the value prop changes, update the internal state. - useEffect( () => { - setInternalInputValue( ( prevValue ) => { - if ( value && value !== prevValue ) { - return value; - } - - return prevValue; - } ); - }, [ value ] ); - - return [ internalInputValue, setInternalInputValue ]; -} diff --git a/packages/block-editor/src/components/link-control/use-internal-value.js b/packages/block-editor/src/components/link-control/use-internal-value.js new file mode 100644 index 0000000000000..ac58c05b10a87 --- /dev/null +++ b/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, + ]; +} diff --git a/packages/e2e-tests/specs/editor/various/links.test.js b/packages/e2e-tests/specs/editor/various/links.test.js index 9c3e8a722a7f0..65b44ea9164c9 100644 --- a/packages/e2e-tests/specs/editor/various/links.test.js +++ b/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' );