Toggling "Open in a New Tab" changes URL back to previous link

[samxmunoz]

Description

Open in a new tab is not working properly for links (tested with inline links & buttons).

You should be able to toggle open in a new tab on and off without it changing the destination URL.

Step-by-step reproduction instructions

1. Set a destination URL for a link.
2. Update the destination URL.
3. Toggle on "open in a new tab"

The URL will revert back to the original and it requires you to type it again.

Screenshots, screen recording, code snippet

Here is a screen recording:

https://i.getf.ly/v1uOZQGz

Environment info

WordPress 6.0.1
Frost Theme

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[Mamaduka]

Hi, @samxmunoz

Do you get the same behavior with and without the Gutenberg plugin active?

[talldan]

Yep, it'd be great to know that. It should be fixed by #42073. That fix will be in WordPress 6.1, but is already in the Gutenberg plugin.

[ntsekouras]

It should be fixed by #42073.

This is the exact opposite.

The issue described here exists, but it's also true(doesn't keep the edited value) if we have started updating the url and then just click somewhere else and making the LinkControl component unmount.

I think this is the intended behaviour and that's why we have the button to 'commit' the changes in the url input. --cc @getdave

[talldan]

@ntsekouras Oh, I think it's slightly different to what you described. I did misread the steps originally and misinterpreted the video because it shows the other bug.

The issue is that when you're typing a URL, if you then click 'Open in New Tab' without submitting, edits to the URL are removed. (after re-reading, this is exactly what was reported 🤦‍♂️)

Still seems to be a bug in trunk. Here's a quick video that shows it:

Kapture.2022-08-17.at.13.54.43.mp4

[ntsekouras]

The issue is that when you're typing a URL, if you then click 'Open in New Tab' without submitting, edits to the URL are removed.

That's what I said, but it occurs with every attribute change(inspector tools) and if it's unmounted.

Screen.Recording.2022-08-17.at.9.58.36.AM.mov

[talldan]

That's what I said, but it occurs with every attribute change(inspector tools) and if it's unmounted.

Ah, ok, I thought you just meant clicking outside the popover.

[phubner]

I am having a similar issue but instead its triggered off the Submit button in the LinkControl. When editing the link if you click the Submit button the Open in new tab toggle will be unset. I didn't want to file a new issue unwarranted

[getdave]

I am having a similar issue but instead its triggered off the Submit button in the LinkControl. When editing the link if you click the Submit button the Open in new tab toggle will be unset. I didn't want to file a new issue unwarranted

Although I can't be 100%, that looks like a customised <LinkControl> implementation. I can see additional controls below Open in new tab. Some Plugins (e.g. Yoast) modify LinkControl and that is outside of Core's control.

If you have a clean WP install (no Plugins) does the issue persist.

[getdave]

Note that <LinkControl> is a controlled component and thus it that can be implemented differently depending on the usage context. For example, the standard links within paragraph text seem to exhibit different behaviour to the links on a Button block. This is because the logic that governs setting values is outside of the component itself.

• Button handling.
• Inline text handling

I feel that instead of trying to fix individual bugs in individual implementations what we need is to

• agree on how the control should behave UX wise
• attempt to modify the component itself to handle this is a standard fashion
• update all implementations (where necessary)

The vision of the component was that links (and associated metadata) would not be modified until the user clicks the "submit" button. Until that point all changes within the component are saved to local component state. The problem is that we now have toggles such as "open in a new tab" which lie in a gray area. Should the toggles:

• act as a "submit" when toggled thereby committing any changes in the control?
• commit only their portion of the state leaving other elements unchanged?

I would vote for the later. So the behaviour should be:

• user enters link (uncommitted)
• user submits link (committed)
• user edits link but doesn't submit (uncommitted)
• user toggles open in new tab setting (commits only settings change not link value)
• user submits link value

I think the core issue is that in certain implementations:

• popover containing the link control will close when a setting is toggled (should remain open).
• the entire link value is also committed when a setting is toggled. It should only be the setting that is committed.

At this point it might be useful to get some input from Design expertise (cc @javierarce and @jasmussen).

[jasmussen]

This is a very unique issue, thank you all for the helpful videos demonstrating the main problem: clicking outside the link dialog without applying first acts as a dismissal and doesn't save the value in the URL.

The toggle is a light-switch, with immediate effect, i.e. it saves and commits at the same time. As clarified by this ticket, this goes against the intentional dismissal of changes made, by clicking outside. In that light, can we/should we change the toggle to a checkbox, requiring that it also be submitted, otherwise a click outside dismisses it?

[getdave]

should we change the toggle to a checkbox, requiring that it also be submitted, otherwise a click outside dismisses it?

One argument against this is that the submit button is very tied to the URL itself. There is no clear indication that the submit functions for the entire link.

Notice how the toggles are divorced from the submit.

For this reason I would be concerned that most folks would toggle the checkbox and then click off expecting things to be saved as they were previously with the toggle control.

[jasmussen]

Yes, that's a good point indeed. Another option is to remove the toggle from that state entirely, and have it something you are only able to do after the fact, here:

To me that seems like a quick and good fix, since opening in new tabs is arguably a pretty bad practice.

Another option is to add a cog of "link settings" before the submit button, which either opens a new modal or "slides" the contents of the window rightward, showing additional options. That seems like a lot of work for unclear benefit, though. What do you think?

[getdave]

I quite like the idea of separating the editing of a link from the changing of settings. We could explore the first option.

Another option is to add a cog of "link settings" before the submit button, which either opens a new modal or "slides" the contents of the window rightward, showing additional options. That seems like a lot of work for unclear benefit, though. What do you think?

I would agree. A lot of work for the potential ROI.

[getdave]

@jasmussen In light of recent updates to the UI (and the upcoming addition of a settings "drawer") should we consider making any changes to the link require clicking Apply?

This would eliminate a whole class of bugs but would require the user to know to submit their changes. However with the updates to the UI perhaps this requirement is now a lot more obvious?

Also noting this ties in nicely with @joedolson's feedback that all changes should require confirmation.

[jasmussen]

No strong opinion here. (Other than that Enter should submit)

[joedolson]

Definitely in favor of requiring confirmation on all changes.

[getdave]

Now #47328 is merged we can look to make this happen.

[hz-tyfoon]

I believe it's fixed by #50401
I'd really appreciate for any feedback if any change needed or if anything can be improved in the PR.

Need review for the PR please.

[note: created the PR in wp/6.2 branch as the trunk branch has few new works that's not merged with WP version 6.2. I believe the PR's fix can also be applied into the trunk if needed]

[getdave]

This was fixed in #50668