Format Library: Make sure link UI state resets when switching between links.

[epiqueras]

Fixes #19394

Description

When editing a link's URL, if you click on another link, the popover will move to the new link, but keep the state and value from the old link. The cause was that the link UI only resets its state when closed through a loss of focus or the popover's close event. When switching between two links in the same block, it's the format library's isActive prop that does the toggling, so this PR uses it as a key for the link UI so that its state resets appropriately.

How has this been tested?

Testing instructions in #19394.

Types of Changes

Bug Fix: The link UI no longer keeps the old link's value when switching to another link in the same block.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[aduth]

Hm, I can see how the issue of lingering state can occur when the link UI is "reused", and I could see how these changes might resolve that because of how it would force a new element instance to be created when the active state changes. But I wonder if it's the best indication that the link for which the popover is shown has changed. Or, at least, I'd wonder if we could account for this within the inline link UI as a side-effect of the isActive prop change. Something like:

useEffect( resetState, [ isActive ] );

(This would require a hooks refactor of the component, for which I do have a local branch about ready to push, in case it's an agreeable approach)

[epiqueras]

I think it makes more sense to use a key. resetState only resets the part of the state it needs to. Here we shouldn't be reusing the component at all. We can always iterate when the hooks refactor lands, but this is a sound fix for 7.2.

[aduth]

I worry that, considering the component transitions (apparently immediate from the perspective of a user) from a state of isActive: true to isActive: true, this relies on an assumption that a render occurs where isActive: false (I'm not sure how strong a guarantee this is). And at the very least, I'd imagine we're creating a new instance of the component twice (once for isActive: false and a second time for the new isActive: true).

If it resolves the bug, it's a simple fix, and I expect shouldn't cause any harm outside some future hypothetical bug if these assumptions ever change.

[epiqueras]

If it resolves the bug, it's a simple fix, and I expect shouldn't cause any harm outside some future hypothetical bug if these assumptions ever change.

Yes, if an issue arises, we can explore fixing this higher upstream in the format library. I'm not sure if there is an actual requirement for these format components to be shared between selections in the same block.

[aduth]

One thing I've noticed in my ongoing hooks refactor is: Why do we render InlineLinkUI at all while it's inactive? The component itself won't actually render anything:

gutenberg/packages/format-library/src/link/inline.js

Lines 189 to 191 in 342f0c5

	if ( ! isActive && ! addingLink ) {
	return null;
	}

So the only difference is that the state is maintained while the link format is inactive... which is (at least in part) the original source of the problem we're seeking to address.

I bring it up because the changes here weren't enough to fix the issue in my local branch. I wonder if there might be a difference in the rendering behavior for the function component. What does work well (and I expect just as well in master) is to move this condition out of InlineLinkUI up into the link edit component.

Replacing:

gutenberg/packages/format-library/src/link/index.js

Lines 126 to 134 in 342f0c5

	<InlineLinkUI
	key={ isActive } // Make sure link UI state resets when switching between links.
	addingLink={ this.state.addingLink }
	stopAddingLink={ this.stopAddingLink }
	isActive={ isActive }
	activeAttributes={ activeAttributes }
	value={ value }
	onChange={ onChange }
	/>

...with something like:

-<InlineLinkUI
+{ ( isActive || this.state.addingLink ) && <InlineLinkUI
-	key={ isActive } // Make sure link UI state resets when switching between links.
	addingLink={ this.state.addingLink }
	stopAddingLink={ this.stopAddingLink }
	isActive={ isActive }
	activeAttributes={ activeAttributes }
	value={ value }
	onChange={ onChange }
-/>
+/> }

(cc @ellatrix to advise whether there's reason we continue to render the link while it's inactive)

[epiqueras]

Yeah, that makes more sense to me as well.

I assume there is or was a case where we need to preserve state between toggles of addingLink?

[ellatrix]

@aduth I'm not sure... this components has moved around a lot with lots of people working on it. If everything still works and the tests pass, then I guess not.

[aduth]

Follow-up at #19487