Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Link fields to be restricted to only internal or external URLs #5678

Closed
ghost opened this issue Jul 9, 2022 · 30 comments · Fixed by backdrop/backdrop#4130
Closed

Allow Link fields to be restricted to only internal or external URLs #5678

ghost opened this issue Jul 9, 2022 · 30 comments · Fixed by backdrop/backdrop#4130

Comments

@ghost
Copy link

ghost commented Jul 9, 2022

Description of the need

If you want to add a Link field to a content type for storing URLs to external websites (e.g. like on the BackdropCMS.org Showcase nodes), users can still enter URLs for internal pages. Additionally, there's an autocomplete that displays a list of matching pages/content on your site (you may not want users seeing this list of content in this situation).

Advocate: @olafgrabienski

Proposed solution

It'd be nice if there was an option for Link fields to restrict them to either external or internal URLs (or both, as is the situation currently). Then you wouldn't have the autocomplete showing when you only wanted users entering external URLs.

Alternatives that have been considered

Drupal has the Advanced Link contrib module that achieves this, however it also does a bunch of other things too.

Additional information

Here's a screencast showing the issue on Backdrop's Showcase node form:

Peek 2022-07-09 22-25

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now optionally allows Link fields to be restricted to just external or internal URLs.

@ghost
Copy link
Author

ghost commented Jul 9, 2022

Here's a PR that adds this feature to core: backdrop/backdrop#4130

image

@indigoxela
Copy link
Member

This makes sense... but I'm a bit concerned - would admins realize that they have to choose "Both", to get LINK_FRONT, LINK_EMAIL, LINK_NEWS - which link_validate_url() might also return?

How about aliases, or absolute urls to internal pages? Might need some more testing, and I'd assume we should get a little more feedback here.

@indigoxela
Copy link
Member

indigoxela commented Jul 10, 2022

I played in the sandbox 😄 ... There are some odd things with field setting combinations:

First I tried setting to "internal only" and had "Validate URL" checked. All seemed good, until I tried <front> which wasn't considered valid.

Then turned "Validate URL" off in the field settings, but validation still happened... not as intended, I suppose. I'd only expect validation (/node/add/post), if $instance['settings']['validate_url'] is set.

Same for the field settings form. I'd rather expect that these radios only show up, if "Validate URL" is checked (/admin/structure/types/manage/post/fields/field_a_link).

@stpaultim
Copy link
Member

I'm getting a Gateway Timeout message when I try to access the sandbox. Is this is temporary issue with Tugboat?

@ghost
Copy link
Author

ghost commented Jul 11, 2022

@indigoxela I've pushed a new commit that fixes the other link types. Now it'll only error if you've selected 'internal' but provide an 'external' URL, or if you've selected 'external' but provide an 'internal' or 'front' URL. In other words, 'front' is treated the same as 'internal' (since it is), and 'mail' and 'news' are always allowed (since it's too hard to determine if they're considered 'internal' or 'external').

As for the 'Validate URL' checkbox, that's a separate thing. It determines if the URL is checked to make sure it's a valid URL (e.g. htps://...). So whether or not that's checked should have no effect on what type of URLs are allowed to be entered here. The admin could decide that only external links are allowed, but maybe there's an issue with certain types of links they want, so they disable 'Validate URL' to allow them. This should now work as expected.

@stpaultim Tugboat seems to be having issues ATM...

@indigoxela
Copy link
Member

As for the 'Validate URL' checkbox, that's a separate thing. It determines if the URL is checked to make sure it's a valid URL (e.g. htps://...). So whether or not that's checked should have no effect on what type of URLs are allowed to be entered here.

Hm, I'm not convinced, that's correct. Current behavior is, that if "Validate URL" is off, no validation happens.

This PR refines validation, using the same function. So link_validate_url runs twice if validation is turned on, only once when it's off. This seems odd to me.

And also the admin UI...
link-validation

Anyway, I won't block this PR - may other people decide, if that's better or worse UX. 😉

@quicksketch
Copy link
Member

quicksketch commented Jul 16, 2022

I think this is a great feature, and I really like the making an External only link field disables the auto-complete on the field.

I'm not sure if "Validate URL" should control the "URL type" field. Under-the-hood, I see that a URL basically must be validated in order to determine if it's internal or external, but as an end-user it feels like it's a different capability.

In any case, we will need new test coverage for this option.

@olafgrabienski
Copy link

I also like the feature, especially not using the autocomplete for external only link fields. Just wanted to look at it in the sandbox, but it's already expired.

@ghost
Copy link
Author

ghost commented Feb 6, 2023

@olafgrabienski I've reset the sandbox so you can test now.

@ghost
Copy link
Author

ghost commented Feb 7, 2023

I've just pushed some changes based on feedback in the PR.

@klonos
Copy link
Member

klonos commented Feb 7, 2023

Yes @kiamlaluno, I think that the more "relaxed" behaviour that @olafgrabienski is describing is intentional. It is to allow people to create links to pages that have not been created yet. If that is not desired, then we should have a separate issue, since that behavior is pre-existing - not introduced with this PR.

Speaking of separate issues, I believe that there has been some misunderstanding in the feedback left in the PR with re to the help text. @BWPanda I don't think that @quicksketch read @indigoxela's comment right. @indigoxela suggested that we should be adding this help text always - not to remove it altogether. Not having this does not provide any hints whatsoever for the content editors, which is not good UX (site builders will be forced to add descriptions in the field labels to distinguish them, like the screenshot):

The above should have some help text like the following (intentionally not using the more technical term "URL" here):

  • internal only: Only internal links are allowed. Start typing to see a list of suggestions.
  • external only: Only external links are allowed.
  • both: Enter an external link, or start typing to see a list of suggestions of internal links.

I also believe that this help text (which will be hard-coded) should be added to the URL part of the link field - it should not be concatenated with the generic field description (which is allowed to be created/edited per field instance). The reason being that if you keep adding more links in a field that allows multiple values, then that help text will eventually be lost from the visible portion of the screen:

Since repeating the same text for every field would increase noise and make the form longer, perhaps this hard-coded help text should only be added to the last instance of the link.

Another though is to have the help text be "sticky", same as the field label as you are scrolling down the page:

Anyway, all that can be a separate issue - don't want to hold this feature further.

@ghost
Copy link
Author

ghost commented Feb 7, 2023

I don't think that @quicksketch read @indigoxela's comment right. @indigoxela suggested that we should be adding this help text always - not to remove it altogether.

@klonos The previous version of this PR did have the help text always there, appended to whatever help text the user added manually. So since it was always there before, I think the suggestion was to remove it altogether... (but I'll let @indigoxela confirm/deny that).

@klonos
Copy link
Member

klonos commented Feb 7, 2023

Sure 👍🏼 ...but this is what @indigoxela said in the PR:

Should this always be there - even if the site builder decided to not add any help text?

@klonos
Copy link
Member

klonos commented Feb 7, 2023

...aah! English! ...I read that as "Shouldn't this always be there" 🤦🏼 🤦🏼 🤦🏼 ...mea culpa.

@klonos
Copy link
Member

klonos commented Feb 7, 2023

...back to the issue though: not having any hints/indications and then throwing validation errors on form submission is bad UX.

@ghost
Copy link
Author

ghost commented Feb 7, 2023

not having any hints/indications and then throwing validation errors on form submission is bad UX.

Yeah, but we don't do that in other situations either... For example, add a Number field and set the min/max values to 5/15 respectively. Create a node of that type and notice the lack of help text explaining what the min/max values are.

I don't think we need to do this for the number field or the link field. If the individual web developer decides users will need this indication, they can add the help text manually I think.

@olafgrabienski
Copy link

olafgrabienski commented Feb 7, 2023

Interesting question, if we need automatic help text, or let the site architect decide. I'm not sure about it, but as an example the image field comes to mind, which combines automatic help text (e.g. allowed file size, or allowed dimensions) with custom text.

@klonos
Copy link
Member

klonos commented Feb 7, 2023

Yeah, but we don't do that in other situations either...

Then we should be fixing these places too. Not having implemented best UX practices in other places should not be an excuse to not do it at all.

For example, add a Number field and set the min/max values to 5/15 respectively. Create a node of that type and notice the lack of help text explaining what the min/max values are.

Let me start by saying that things aren't perfect, but they are improving as technology evolves, and we should be using what we have. For instance in that scenario above, the HTML5 number element on the page will have min="5" and max="15" attributes specified, which prevents the field from being set to values below/above those limits. HOWEVER, this can be "bypassed" if someone manually types a number or copies/pastes it. Even then, very simple JavaScript can be used to:

  • either automagically revert the values to the specified min/max if the entered value exceeds those (see this and this for example) - I don't like this UX personally, as it seems to be "magically" doing things without informing the user why, which may be perceived as a browser glitch.
  • or to show inline, client-side validation errors (similar to the hints we are showing in password fields re strength and required length/characters). Now, there are two camps when it comes to this: some people prefer to see mistakes as they fill in the form and hate it when they have to re-scroll through it if errors are presented after form submission ([D8][UX] Add inline form errors #1040 will improve that), while others prefer to be shown a summary after the form has been submitted instead of being "annoyed" by constant validation errors as they type. I personally find the former method better, and user frustration and distraction can be avoided if validation happens after loss of input element focus instead of while the user is typing.

Even if no JS is to be used, best UX would still be to add automatic help text there, pointing out the min/max allowed values.

Anyway, if core committers have consensus to not include any help text with this change, I will file a follow-up to do that later (and for number fields with min/max restrictions 😉).

@smaiorana
Copy link

Hello! I'm in favor of this feature. I have a Backdrop site with external-only link fields, and the forced autocomplete box is not ideal. Is this PR still in consideration? I'd be more than happy to help test.

@olafgrabienski
Copy link

As discussed during the last weekly meeting (see https://www.youtube.com/live/HvOn6XVO_Z0?t=1573 for the topic of this issue), I'm advocating for the feature. I'll update the description accordingly.

@olafgrabienski
Copy link

As I'm advocating for this feature request, I wanted to check if the PR still works for me, but its sandbox is expired. @BWPanda – are you able to rebuild the sandbox somehow?

@quicksketch
Copy link
Member

I closed and reopened the PR, which should make a new sandbox.

@olafgrabienski
Copy link

olafgrabienski commented Aug 31, 2023

Thanks for providing the new sandbox. The PR still works like a charm.

Note: There are two failing checks. Not sure if / how related to the PR.

Apart of that, there was a discussion if we should provide some help text for editors, see #5678 (comment). @klonos agreed to file a separate follow-up issue, if we have consensus to not include help text at this point.

Conclusion: The PR is working. It would be great if somebody could take care of automated tests. We need also a code review.

@quicksketch
Copy link
Member

@olafgrabienski I added test coverage to the existing PR over at backdrop/backdrop#4130. The existing code looks good to me and if the tests pass, I think this can be approved.

@olafgrabienski
Copy link

olafgrabienski commented Sep 3, 2023

Thank you very much, @quicksketch, wonderful!

Tests are passing, with one exception: spell check.

edit: The PR is still working, btw.

@quicksketch
Copy link
Member

The spellcheck errors are unrelated; they seem to come from bootstrap.inc mostly. This might be the most relevant issue? #6060

But I think it stands that unless there are any further reasons, this is ready to go!

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4130 into 1.x for 1.26.0. Thanks @BWPanda and @indigoxela for the original PR work, and thanks @olafgrabienski for shepherding this to completion. Thanks @klonos and @kiamlaluno for reviewing and support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants