Adds link target options to button block

[kadencewp]

Description

Similar PR: #10128.

This adds two new attributes for Button Block. Link target and link rel attribute. This also changes the button URL input to use a popover and unifies with other URL input styles.

Fixes #6392

How has this been tested?

Tested Manually

Screenshots

Previously:

After this Pull:

Types of changes

Added two new attributes for link target attribute and link rel attribute.

Checklist:

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

[gziolo]

This seems like a really good idea 👍

[jasmussen]

Thank you for this pull request.

Here's a GIF:

On the one hand, this is very clever and good use of the selected state. It also allows you to add the additional options, which is good.

On the other hand, it slightly goes against principles. The selected block can shows additional content when selected — the image can show a caption field, a Maps block can show the URL field right below.

But this is a popout, which usually is invoked only when clicking a specific button. That puts it square in a gray area.

I'm inclined to approve this (pending a few small tweaks), but I'd like a second opinion, which I'll get you asap. Thank you for the PR!

Assuming this goes right through, we should fix this margin issue that's present in the link button poput but not the normal one. Compare:

[kadencewp]

Should I update with some css tweaks to make the input the same or wait to hear if using a popup is the best option? Thanks!

[jasmussen]

Yep, sorry about the delay.

In those I've chatted with in Slack there doesn't seem to be a wide consensus on what is best. But given this solves a problem, I think we should accept this as an improvement for now and move forward. If you can fix the input field width issue, I think this can move on to the code review phase.

Thank you for your contribution and your patience!

[kadencewp]

ok, I just switch the class to use the same as the inline link popup and removed all the no longer needed css. Input width matches the style now.

[designsimply]

@mvpspl619 noted at #8000 (comment) that there's another open PR doing similar work to this one at #10128 (thanks @mvpspl619!) and that one does seem to have more recent comments and @kadencethemes I was wondering if you'd like to defer to #10128 and maybe do a code review there or maybe sync up with @nfmohit-wpmudev ?

[kadencewp]

Hey @designsimply

I'm not sure what you are asking me to do, while #10128 does offer the same solutions it doesn't use the core URLPopover component and it doesn't add settings into the sidebar. It does simplify the target to a boolean so the user can't define something other then "_blank" and it doesn't add a rel attribute editing but instead just inputs 'noreferrer noopener' if the target is a new tab.

I am not sure I'm qualified enough to make a case for what should be used.

I can offer my opinion and that is: I would suggest using the simplified attributes in #10128 but updating to use the URLPopover component.

But I honestly can see not doing this if someone doesn't like the UI offset that the URLPopover component adds ( see image above where the url input doesn't align all the way left ).

I hope that is helpful!

Ben

[designsimply]

Hi Ben! This is what I was after:

While #10128 does offer the same solutions it doesn't use the core URLPopover component and it doesn't add settings into the sidebar. It does simplify the target to a boolean so the user can't define something other then "_blank" and it doesn't add a rel attribute editing but instead just inputs 'noreferrer noopener' if the target is a new tab.

I can offer my opinion and that is: I would suggest using the simplified attributes in #10128 but updating to use the URLPopover component.

Your explanation helps me see the differences between the PRs better. I don't have a strong preference for one PR over the other and would like help getting a decision about which one to go forward with. If we continue with #10128, would you want to loop back afterward and add rel attribute editing at that point?

[kadencewp]

If we continue with #10128, would you want to loop back afterward and add rel attribute editing at that point?

Hard to say, I think it comes down to what Gutenberg is doing elsewhere and for normal text links, it's not adding this kind of editing options. So I don't think it's a feature that must be added. If it's a feature the Gutenberg team thinks the button should have I would be happy to add it. I put it there for completeness because I was adding in 'noreferrer noopener' automatically for the security and figured there may be people that want to edit it, but I'm not sure it is needed and looking at the simplicity of #10128 approach that might be better.

[designsimply]

Understood. ❤️

Thanks so much for following up here! I have heard that you're doing cool work btw. 😊

[gziolo]

@nfmohit-wpmudev, @jasmussen and @talldan - it looks like this PR is very similar to #10128. We should pick one and make sure that code from both is used. This PR removes dead styles and adds a proper deprecation strategy caused by the fact that new attributes are added to save function's output. It also handles the position of the popover properly. It seems to be closer to ready.

[talldan]

@gziolo Thanks for the reminder, I'd forgotten about this PR. Would definitely be good to avoid duplicated effort. I think some of my comments on the other PR also apply to this one.

Regarding the deprecation, I wouldn't have thought that strictly necessary for an additive change. linkTarget could handle being undefined.

[gziolo]

Regarding the deprecation, I wouldn't have thought that strictly necessary for an additive change. linkTarget could handle being undefined.

Well, I might be wrong. I think @Soean should know better as he added a few attributes to media-related blocks and I can't see any deprecations introduced. It might be fine to skip it, we would have to test.

[afercia]

But this is a popout, which usually is invoked only when clicking a specific button.

Worth considering keyboard accessibility.
In other components, focus is moved to the URLPopover when clicking a button or when using a keyboard shortcut. Instead, if the URLPopover is already open when the block gets selected, it's almost impossible to tab into it as it's very far in the DOM tab sequence.

Similarly, if initial focus is on the URL input field, then going back to the button text is almost impossible.

I think this component should be consistent with the interaction of other similar implementations:

• initial focus on the button text
• additional button and shortcut to invoke the URLPopover
• focus is moved to the URLPopover
• users add / edit link
• pressing Escape should close the URLPopover and move focus back to the button text
• pressing Enter (or the Apply button) should set the link and move focus back to the button text

See also #6841 (comment)
The button text editable needs a clear indication of focus.

[gziolo]

Let’s close in favor of #10128 where it’s still being discussed how to resolve accessibility issues.

@kadencethemes - thank you for your great contribution. This PR helped a lot to drive the implementation in the right direction and made it clear what accessibility issues need to be resolved 👍