Fix Link Control 'Open in new tab' option not saving properly on committing link in buttons block

[hz-tyfoon]

This PR fixes #40243

[note: The mentioned issue was not only in buttons block but in the 'link-control' component]

[getdave]

@hz-tyfoon Thank you for the PR. It's much appreciated 👏

I tested this with the standard link in the Post Editor and I was unable to reproduce. I could however, reproduce with the buttons block. This is using latest Gutenberg trunk from Github.

Screen.Capture.on.2022-06-30.at.15-45-31.mov

I believe this is likely due to the fact that the <LinkControl> is a controlled component (like a form input) and therefore each consumer must provide their own handling implementation.

At first glance, your fix seems good, but I'm going to dig a little deeper and see if there's fundamental problem with the handler in richtext vs the button block.

Also please could you resolve the unit tests? 🙇

[getdave]

The issue is that LinkControl will call onChange for any change to the value. The "value" also includes settings such as opensInNewTab.

So when the user clicks "Open in new tab" onChange is called and the following code sets the Button block's linkTarget attribute to true.

gutenberg/packages/block-library/src/button/edit.js

Lines 229 to 230 in b0244c2

	url: newURL = '',
	opensInNewTab: newOpensInNewTab,

However, when the user clicks submit to "commit" the link then due to the problem that @hz-tyfoon identified the original value of opensInNewTab is not provided to the callback and thus in the Button block onChange handler newOpensInNewTab has a value of undefined. This then overwrites the original setting of the linkTarget attribute from true to undefined which is why the UI behaves as it does.

On balance I think we should update LinkControl to provide the stored value on commit as per this PR. We should update the tests to assert on the full value being provided. Please could you add opensInNewTab to the relevant tests to further improve them?

[hz-tyfoon]

Hey @getdave. 🙂
Hope you're doing well.
thanks a lot for your comment. 👍

@hz-tyfoon Thank you for the PR. It's much appreciated 👏

I tested this with the standard link in the Post Editor and I was unable to reproduce. I could however, reproduce with the buttons block. This is using latest Gutenberg trunk from Github.

Screen.Capture.on.2022-06-30.at.15-45-31.mov
I believe this is likely due to the fact that the <LinkControl> is a controlled component (like a form input) and therefore each consumer must provide their own handling implementation.

At first glance, your fix seems good, but I'm going to dig a little deeper and see if there's fundamental problem with the handler in richtext vs the button block.

Also please could you resolve the unit tests? 🙇

⬆️ I was talking about this issue #42080 with the standard link in the Post Editor. I'm not sure why the unit tests always so stubborn. In my first contribution few days earlier it failed 5-6 times and finally it passed after re-running the test again.

[hz-tyfoon]

The issue is that LinkControl will call onChange for any change to the value. The "value" also includes settings such as opensInNewTab.

So when the user clicks "Open in new tab" onChange is called and the following code sets the Button block's linkTarget attribute to true.

gutenberg/packages/block-library/src/button/edit.js

Lines 229 to 230 in b0244c2

	url: newURL = '',
	opensInNewTab: newOpensInNewTab,

However, when the user clicks submit to "commit" the link then due to the problem that @hz-tyfoon identified the original value of opensInNewTab is not provided to the callback and thus in the Button block onChange handler newOpensInNewTab has a value of undefined. This then overwrites the original setting of the linkTarget attribute from true to undefined which is why the UI behaves as it does.

On balance I think we should update LinkControl to provide the stored value on commit as per this PR. We should update the tests to assert on the full value being provided. Please could you add opensInNewTab to the relevant tests to further improve them?

The issue is that LinkControl will call onChange for any change to the value. The "value" also includes settings such as opensInNewTab.

So when the user clicks "Open in new tab" onChange is called and the following code sets the Button block's linkTarget attribute to true.

gutenberg/packages/block-library/src/button/edit.js

Lines 229 to 230 in b0244c2

	url: newURL = '',
	opensInNewTab: newOpensInNewTab,

However, when the user clicks submit to "commit" the link then due to the problem that @hz-tyfoon identified the original value of opensInNewTab is not provided to the callback and thus in the Button block onChange handler newOpensInNewTab has a value of undefined. This then overwrites the original setting of the linkTarget attribute from true to undefined which is why the UI behaves as it does.

On balance I think we should update LinkControl to provide the stored value on commit as per this PR. We should update the tests to assert on the full value being provided. Please could you add opensInNewTab to the relevant tests to further improve them?

Agreed. 👍
Thanks a ton for taking the time to check this. 💚
@getdave

[hz-tyfoon]

Looks like this PR also fixing this issue #40243 as well.

[talldan]

@hz-tyfoon Thanks for working on this. There's a unit test for link editing that's failing as a result of these changes and will need to be updated - https://github.com/WordPress/gutenberg/runs/7173641105?check_suite_focus=true.

There's a comment on the other PR that mentions the same issue, and has some details for solving it - #40244 (comment).

[hz-tyfoon]

There's a comment on the other PR that mentions the same issue, and has some details for solving it - #40244 (comment).

Sorry for late reply, I somehow didn't notice your comment earlier. @talldan
Updated the Unit test & looks like the failed JS test have passed now.
Thanks @talldan & @torounit

[getdave]

LGTM - tests now streamlined and the fix seems good. The controlled component should always pass the entire value when it calls the onChange handler. This PR addresses that so I think it's good.

[hz-tyfoon]

Shouldn't (issue: #42032) also be closed, as this PR was originally created to solve that? 🤔