Keep "Open in New Tab" when changing link of Button block.

[ddryo]

What?

Fix #40243.

Why?

When editing a link in a Button block, linkTarget is reset even though only the url is changed.

This is a bit strange behavior.

So I created a PR that when only the URL is edited, it does not affect the linkTarget that has already been set.

How?

When opensInNewTab is undefined in the information returned by onChange of <LinkControl>, the update process of linkTarget is not performed.

opensInNewTab was not being passed in handleSubmit() of <LinkControl>.

Testing Instructions

1. Open a Post or Page.
2. Insert a Button Block.
3. Set the URL for the link and turn on "Open in new tab".
4. Edit the URL using the "Edit" button and click the "Submit" button to save.

Screenshots or screencast

This PR improves the behavior as follows

Kapture.2022-04-12.at.12.09.09.mp4

[talldan]

@ddryo Thanks for working on this. Looking at the code, my concern is that this is a bug in LinkControl. I'm wondering why it returns a different value for opensInNewTab if only the url has changed.

[ddryo]

@talldan
Oh indeed, you are right.

I undid the modification on the Button Block side and modified the handleSubmit in LinkControl

[ddryo]

@talldan
Hi, I would like to ask for confirmation on this matter.

[goaround]

@ddryo thank you for working on this issue. It bothered me since WordPress 5.9. Hope it will be fixed soon

[hz-tyfoon]

I did a PR (#42073) for issue(#40243). Looks like similar.

[getdave]

@talldan I've reviewed #42073. Looks like we've doubled up 😄

Who would be prepared to close their PR in favour of the other one?

[talldan]

Hi, I would like to ask for confirmation on this matter.

@ddryo Sorry, I was on vacation (and avoiding github) so couldn't reply to your earlier message.

@talldan I've reviewed #42073. Looks like we've doubled up 😄

Who would be prepared to close their PR in favour of the other one?

@getdave I did notice that. There are multiple issues now too.

I think whichever one is mergeable first is ok. It looks the @ddryo hasn't addressed some feedback in this PR from @torounit, so it isn't quite ready.

[getdave]

Just FYI that I will be AFK after today so perhaps @talldan can help with landing one of these? Apologies!

[ddryo]

@torounit
@talldan

Sorry for the delay in responding to review.
I have tried to correct it.

[torounit]

gutenberg/packages/block-editor/src/components/link-control/test/index.js

Lines 2420 to 2423 in cb32e26

	expect( mockOnChange ).toHaveBeenCalledWith( {
	title: textValue,
	url: selectedLink.url,
	} );

The test is failing and needs to be corrected.

It would be better to use expect.objectContaining.

@ddryo

[talldan]

A PR from a different contributor (#42073) was a little bit faster at solving the issue (including the failing unit test), and that has now been merged, so I'll close this PR.

Thanks for your work on this @ddryo. It is appreciated even if it didn't end up being merged.