Navigation: Show suggestions panel automatically when editing a link

[jeryj]

Would close #19647 if we go this route.

What it fixes

1. Show the suggestions panel automatically when editing a link.
   Before:
   

2. Fix suggestions reset when resetting from a URL-like search result Reset button removed in Block Editor: LinkControl: Incorporate settings in edit state #20052
   Before:
   

3. Applying focus to the search input on reset (currently it gets reset to the top of the document ) Reset button removed in Block Editor: LinkControl: Incorporate settings in edit state #20052
   Before:
   

Description

• Adds an onFocus handler to URLInput so it can be used by the LinkControl SearchInput to automatically show suggestions when the search field is focused.
• Resets search suggestions when X is clicked or the field is deleted
• Focuses search input when X is clicked

How has this been tested?

1. Manually
2. Integration Tests

Screenshots

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[jeryj]

@WunderBart @getdave I opened this draft PR to play around with. Feel free to take it over, abandon, or do whatever you'd like to it :)

It adds an onFocus handler to the URLInput that is just copy/pasted from the current onChange. That at least keeps them a little bit insulated from each other if we need them to diverge.

This implementation isn't an improvement though, IMO, as if you hit Enter, it will update the label to the link. I'd rather it bring up the matched Page than search the URL. It looks like it used to search the label (ie About instead of localhost:8889/etc)

If we could go back to that implementation, this could possibly be good to go.

[obenland]

It looks like it used to search the label (ie About instead of localhost:8889/etc) If we could go back to that implementation, this could possibly be good to go.

Agreed. Having the full url in the search input feels a little odd, I would have expected to see the link text.

[obenland]

@apeatling Do you have an opinion on this change?

It surfaces the lack of awareness that the link is actually an internal (page) resource and turns the entity into a normal absolute URL. Given that, does this PR feel like an improvement to you?

[jeryj]

It looks like it used to search the label (ie About instead of localhost:8889/etc)

I think I may have been wrong about this. It looks to me like the implementation of this PR is the same as it would be in master. Testing it out again now, both implementations (master vs this branch) are identical other than this branch automatically searches whatever is placed in the input.

master:

This branch:

It surfaces the lack of awareness that the link is actually an internal (page) resource and turns the entity into a normal absolute URL.

I completely agree. We'd need this PR (or the like) in order to get the functionality that is desired (return the page result), but I'm not sure if that functionality should be within this PR. I'm not sure what the best thing to do here is.

[obenland]

You're right, I noticed that, too, while testing another PR.

[apeatling]

I think this is an improvement, especially for the situation in #19647.

[retrofox]

I've tested and it works as expected.

very nice job, @jeryj

[obenland]

@getdave When you get a chance, would you mind giving these test changes a final 👍?

[getdave]

Thanks for these changes @Jery. Extra props for updating the tests 👍

I wonder whether we also ought to have a test which explicitly tests the new focus behaviour you've added?

For example, testing that when the input receives a focus event then the appropriate results are shown. You could also test that the mocked fetch handler has been called (or not!) as required.

[jeryj]

@getdave I've added a few more tests as recommended, including a check to make sure focus gets placed on the input after reset. Mind taking another look?

[getdave]

In general I feel this works but I have concerns about the implications of having to call this.updateSuggestions in the reset method.

Also as this component underpins all of the Link UI across the editor I'd recommend we get a 2nd opinion from @aduth before merging.

[jeryj]

@aduth @getdave, As an update to make this PR a little easier to review, I updated the PR description as it is now addressing three different things.

I think the biggest one is the focus-loss after clicking the X to reset the input. It'd be great to get this in sooner than later, especially since it is being used for all link searches now.

[jeryj]

I think something is broken with the tests in the Link Control. I've written a test that should definitely fail. I found a bug after removing the check for suggestions in shouldShowInitialSuggestions(): faf6ba1

When you use the down arrow to navigate into the links on a search, it does another search request via componentDidUpdate, then shouldShowInitialSuggestions() evaluates to true and runs the search again.

The test I've written should fail, but they don't. The only reason I can think of is that the test isn't actually waiting for another search result to finish. It seems only able to call a search one time in the tests.

Anyone with more testing experience have advice here?

[getdave]

Thanks for persisting with this 👍

In general it looks good. I'm slightly concern about changing the conditionals around shouldShowSuggestions() but I'm a lot happier now that we're not manually forcing the current focus.

⚠️ I will caution here that this is a change effecting a main API so we should probably look for approval from a Core team member.

[jeryj]

I'm slightly concern about changing the conditionals around shouldShowSuggestions()

That change was also related to fixing functionality around the reset button. As that has been removed entirely, I've reverted the shouldShowSuggestions() to its previous state.

[obenland]

Nice improvement over the existing experience. Tested a few posts functionally and works as expected.

[marekhrabe]

This is working well for me, based on testing instructions. Now the reset button is removed, it's much cleaner. I'd be happy to get this UX improvement in 👍

[aduth]

Why do we pass inputValue like this? Related to this, the API change should have been added to the component's documentation, describing the value that's being passed.

Per my previous comment, I would expect that this component enforces that the value prop it is given will be the one shown in the input. Thus, the parent wouldn't need to receive the value argument, because it's the same as it was already providing in its rendered value prop.

[jeryj]

Why do we pass inputValue like this?

I had copied the onChange implementation. I'm still learning the ropes on Gutenberg, so I was trying to follow existing code as much as I could. Do you think the onFocus( inputValue ) should be removed entirely?

the API change should have been added to the component's documentation, describing the value that's being passed.

Thanks for pointing out the documentation - sorry I did not update that! I'll get a PR in today to add it in.

[jeryj]

I've created the PR here: #20220

I did not add documentation as the this.props.onFocus prop has been removed. If I am misunderstanding this and still need to add this to the component documentation, please let me know 👍

[aduth]

In what circumstances would we expect this not to be the same as this.props.value?

[jeryj]

Great point. I was following the onChange implementation. I can replace event.target.value with this.props.value (or, rather value and add value to const { disableSuggestions, onFocus, value } = this.props).

I will add that in to the same PR that the documentation update is in, unless you'd rather they be separated out.

[aduth]

Ultimately, I question whether we need the argument at all.

With onChange, the value represents some intent to change the value. With onFocus, I don't think we're wanting to effect any change in the value?

[jeryj]

Completely agree. Thanks for walking me through this so clearly. Learning a lot from your comments! 💯