Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attempting to sync state in controlled Link Control #49506

Closed
getdave opened this issue Mar 31, 2023 · 2 comments · Fixed by #49509
Closed

Fix attempting to sync state in controlled Link Control #49506

getdave opened this issue Mar 31, 2023 · 2 comments · Fixed by #49509
Assignees
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Type] Bug An existing feature does not function as intended

Comments

@getdave
Copy link
Contributor

getdave commented Mar 31, 2023

In packages/block-editor/src/components/link-control/use-internal-input-value.js we are syncing internal state back up with any changes to the controlled value.

useEffect( () => {
/**
* If the value changes then sync this
* back up with state.
*/
if ( value && value !== internalInputValue ) {
setInternalInputValue( value );
}
}, [ value ] );

The effect omits a dependency here of internalInputValue and reinstating it will cause a bug whereby you cannot modify the value of the inputs because they will be immediately synced back up with the controlled value.

I believe this code was added based on a bug whereby:

  • you change an input value
  • you modify the controlled value (for example when the LinkControl component remains mounted by receives a new value)
  • the input does not reflect the new controlled value

This was happening when you clicked around between Links on a RichText field. It was fixed by passing a unique key to each instance of the component so perhaps this fix is no longer needed.

Alternatively we need a smarter way of going about this.


Originally discovered in #48722

@getdave getdave added [Type] Bug An existing feature does not function as intended [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Mar 31, 2023
@getdave
Copy link
Contributor Author

getdave commented Mar 31, 2023

@kevin940726 I remember discussing this with you in the past. We added #34742 to fix the original issue.

Do you think we can safety remove the offending code?

useEffect( () => {
/**
* If the value changes then sync this
* back up with state.
*/
if ( value && value !== internalInputValue ) {
setInternalInputValue( value );
}
}, [ value ] );

@getdave
Copy link
Contributor Author

getdave commented Mar 31, 2023

@kevin940726 Here is a test that illustrates the scenario:

it.only( 'should reset state on value change', async () => {
	const user = userEvent.setup();
	const textValue = 'My new text value';
	const mockOnChange = jest.fn();

	const { rerender } = render(
		<LinkControl
			value={ selectedLink }
			forceIsEditingLink
			hasTextControl
			onChange={ mockOnChange }
		/>
	);

	await toggleSettingsDrawer( user );

	const textInput = screen.queryByRole( 'textbox', { name: 'Text' } );

	expect( textInput ).toBeVisible();

	await user.clear( textInput );
	await user.keyboard( textValue );

	// Was originall title: 'Hello Page', but we've changed it.
	rerender(
		<LinkControl
			value={ {
				...selectedLink,
				title: 'Something else',
			} }
			forceIsEditingLink
			hasTextControl
			onChange={ mockOnChange }
		/>
	);

	// The text input should not be showing as the form is submitted.
	expect( screen.queryByRole( 'textbox', { name: 'Text' } ) ).toHaveValue(
		'Something else'
	);
} );

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 31, 2023
@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants