Fix the update of link and suggestions in LinkControl

[ralucaStan]

When switching between two anchors in the same text block, the text input from the edit link window will not show the correct value.
See video with bug here: https://www.loom.com/share/d18dba4f4c0c43b0adb33f943e77ce0f

Description

When switching between two links, the LinkControl component does not unmount.
The value prop will change but the component does not react to this change and does
not show the updated link & suggestions.

This PR does 2 things:

• it accounts for the value change inside of LinkControl and updates it's internal state
• it modifies the update of the suggestions when the value changes. The component only handled a new empty value, showing the initial suggestions if the user was to remove the value inside the input. But because the value can change and not be empty, we need to update the suggestions for this new non-empty value.

How has this been tested?

• go to a page and add two links in the same text block
• go to the first link and click it to see the edit link window
• click the Edit button and see the text input with the URL and the suggestions list
• without clicking anything else, click the second link
• observe the values in the text input and the suggestion

Screenshots

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• 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 (please manually search all *.native.js files for terms that need renaming or removal).

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @ralucaStan! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[getdave]

Ill take a look

[getdave]

Update: just had a thought here. I think the perceived "delay" in the update is due to the URLInput re-fetching the search suggestions.

Screen.Capture.on.2021-08-12.at.14-48-25.mov

It's actually not doing a network request because the value.url is identified as being "URL-like" so it ends up in a direct entry handler

gutenberg/packages/block-editor/src/components/link-control/use-search-handler.js

Lines 22 to 47 in 29d286f

	export const handleDirectEntry = ( val ) => {
	let type = 'URL';
	const protocol = getProtocol( val ) || '';
	if ( protocol.includes( 'mailto' ) ) {
	type = 'mailto';
	}
	if ( protocol.includes( 'tel' ) ) {
	type = 'tel';
	}
	if ( startsWith( val, '#' ) ) {
	type = 'internal';
	}
	return Promise.resolve( [
	{
	id: val,
	title: val,
	url: type === 'URL' ? prependHTTP( val ) : val,
	type,
	},
	] );
	};

This handler resolves with a Promise. That's why we see the delay in the update.

I guess we could:

1. Find a way to reset the suggestions before we do the "fetching" logic.
2. Deliberating introduce a delay in the handleDirectEntry promise resolution to force the UI to go into a loading state.

---

Thanks for catching this bug and for the PR. I encounter the same problem working on #33849 and it's really important we get it solved.

I tested it and for the most part it seemed to work well. However, it is possible to observe the component updating after the fact once you switch to the 2nd link.

In the video below you can clearly observe the link in the input changing from X -> Y as I switch between them. Ideally this wouldn't happen.

Screen.Capture.on.2021-08-12.at.14-28-59.mp4

I think perhaps we should consider force remounting the component based on the value of the URL.

We could try to do this by adding a key prop to the component and assigning it's value to value.url. That way if the value changes the component will force remount.

I'd like to get a second opinion on this - perhaps from @kevin940726?

The reason this is not happening at the moment is - I believe - due to the fact that Popover doesn't remount. As a result any child component doesn't re-mount either. Thus we get stale states.

You'll know this is true because if you fully unmount <LinkControl> (for example by clicking away from the text entirely and then clicking back on the next link) then all the state will be updated correctly.

[getdave]

Could you explain this change and why it's necessary? This change removes documentation in terms of:

1. The code comment explaining what's happening.
2. A well named function call which self-documents the check logic.

[ralucaStan]

The intention here was to bypass shouldShowInitialSuggestions, which was returning false because of __experimentalShowInitialSuggestions, and just copy its logic regarding the display of suggestions.
I see now that there was a misunderstanding from my side regarding what initial suggestions meant so the ! this.props.__experimentalShowInitialSuggestions part of my change does not make sense.

I am not sure how to proceed further here as I don't understand why __experimentalShowInitialSuggestions is set to false inside shouldShowInitialSuggestions

gutenberg/packages/block-editor/src/components/url-input/index.js

Line 116 in bd514b8

__experimentalShowInitialSuggestions = false,

[getdave]

I am not sure how to proceed further here as I don't understand why __experimentalShowInitialSuggestions is set to false inside shouldShowInitialSuggestions

As far as I can see it isn't necessarily being set to false here. Rather false is used the default value if __experimentalShowInitialSuggestions is not provided as a prop (ie: is undefined). See MDN for more details.

I still don't quite understand why you need to bypass the this.shouldShowInitialSuggestions() logic here? What was it about that which was breaking the fix you've tried to introduce here?

[ralucaStan]

Hi @getdave,
I am sorry for the late reply.

What was it about that which was breaking the fix you've tried to introduce here?

You can see in the video below the behavior if I don't bypass this.shouldShowInitialSuggestions():
when I click on the second link I see the suggestions from the first link.
https://www.loom.com/share/79dedfab3b6942458d2b6fa6d2c9e34b

Let's look at what this.shouldShowInitialSuggestions() returns:

return 
(
! this.isUpdatingSuggestions && // suggestions aren't loaded
__experimentalShowInitialSuggestions && 
! ( value && value.length ) && // there is no value in the input
! ( suggestions && suggestions.length ) // there are no suggestions loaded
)

Please note that this would return true only when the user selects a text and clicks Add link.

In our case, when clicking the second link, this.shouldShowInitialSuggestions() always returns false because:

• there is a value in the input, namely the 2nd link
• there are suggestions available, from the 1st link.

What happens is that when I open the 1st link, the suggestions are updated with the onFocus callback. When clicking on the 2nd link, onFocus is no longer called and the suggestions aren't updated in componentDidUpdate because of this.shouldShowInitialSuggestions

[kevin940726]

Yeah, I feel like using key to force a remount might be the best way forward if there's no performance issue. We can wrap <LinkControl> with a wrapper component so that it's not required for users to update them. Something like this:

function LinkControlWrapper(props) {
  return <LinkControl {...props} key={props.value} />
}

Not sure about the delay thing you mentioned though 😅.

[getdave]

I've thought about this some more and whilst this change doens't fully resolve the issue (as described in my previous review) it does move us towards a better state.

We can look to tackle the other lagging visual update of the search result in a follow up PR.

If you could address the feedback here then we can look to merge this.

[getdave]

I am not sure how to proceed further here as I don't understand why __experimentalShowInitialSuggestions is set to false inside shouldShowInitialSuggestions

As far as I can see it isn't necessarily being set to false here. Rather false is used the default value if __experimentalShowInitialSuggestions is not provided as a prop (ie: is undefined). See MDN for more details.

I still don't quite understand why you need to bypass the this.shouldShowInitialSuggestions() logic here? What was it about that which was breaking the fix you've tried to introduce here?

[getdave]

Yeah, I feel like using key to force a remount might be the best way forward if there's no performance issue.

Let's tackle this in a follow up PR. I believe this PR moves us towards a better state.

[getdave]

I haven't forgotten about this one. Just trying out some other options.

[getdave]

I've left one change that I think should be made but other than that I'm happy to approve this.

You'll need another careful rebase.

Thanks for your patience here. I've taken far too long to get to this.

[getdave]

@tellthemachines We'd also appreciate it if you could review this one 🙇

[ralucaStan]

I've left one change that I think should be made but other than that I'm happy to approve this.

You'll need another careful rebase.

Thanks for your patience here. I've taken far too long to get to this.

@getdave I have pushed the changes that I thing fix this issue and updated the description.
With my last change I handled the update of the suggestions when the value changes in the URLInput component. The component only handled a new empty value, showing the initial suggestions, if if the user was to remove the value inside the input (only when __experimentalShowInitialSuggestions=true). But because the value can change and not be empty, we need to update the suggestions for this new non-empty value case.

The branch is also rebased on the latest trunk

@tellthemachines this is ready for review. Ping me with anything

[ralucaStan]

The component only handled a new empty value, showing the initial suggestions if the user was to remove the value inside the input. But because the value can change and not be empty, we need to update the suggestions for this new non-empty value.

[ralucaStan]

Account for the value change inside of LinkControl and updates it's internal state

[tellthemachines]

This is working well ✅

Ideally the popover shouldn't open in edit mode at all, but I think #34742 addresses that, so let's get this one in to start with!

[getdave]

Great work. LGTM

[noisysocks]

This doesn't need the back port label as it landed in 11.9.