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

Tags datatype - javascript error when including a space char in the 'tag group' name #13146

Closed
jono-k opened this issue Oct 9, 2022 · 12 comments

Comments

@jono-k
Copy link

jono-k commented Oct 9, 2022

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.2.1

Bug summary

Creating a Tags datatype with a space in the tag group name causes a javascript error when adding tags, and the tags aren't correctly saved. This is the js error when clicking on the property and creating a tag. Also happens when clicking into the tag field and then clicking somewhere else in the screen to remove focus:

angular.js:15697 TypeError: Cannot read properties of undefined (reading 'typeahead')
    at umbTagsEditorController.addTag (umbraco.directives.min.js?d=cfbce5a138cd15f23da237e5c8f8a98394e3a527:1:266252)
    at fn (eval at compile (angular.js:16548:15), <anonymous>:4:162)
    at e (angular.js:29123:13)
    at b.$eval (angular.js:19523:16)
    at b.$apply (angular.js:19622:20)
    at HTMLInputElement.<anonymous> (angular.js:29127:13)
    at HTMLInputElement.dispatch (jquery.min.js?d=cfbce5a138cd15f23da237e5c8f8a98394e3a527:2:43064)
    at v.handle (jquery.min.js?d=cfbce5a138cd15f23da237e5c8f8a98394e3a527:2:41048)
(anonymous) @ angular.js:15697
(anonymous) @ angular.js:11956
$apply @ angular.js:19627
(anonymous) @ angular.js:29127
dispatch @ jquery.min.js?d=cfbce5a138cd15f23da237e5c8f8a98394e3a527:2
v.handle @ jquery.min.js?d=cfbce5a138cd15f23da237e5c8f8a98394e3a527:2

Specifics

No response

Steps to reproduce

Create a tags datatype, enter a tag group name consisting of 2 words that includes a space.
Add the datatype to a document type, create a node and then click in the tag property.
Add a tag, or click out of the tag property, and a javascript error appears in the console.

Expected result / actual result

No response

@jono-k jono-k added the type/bug label Oct 9, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

Hi there @jono-k!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@jannikanker
Copy link
Contributor

I am seeing this also, on a fresh 10.2.1 install. Will see if I can also reproduce on 10.3 and if so, I'll see if I can figure out what's going on (and might even try to fix it) :-)

@jannikanker
Copy link
Contributor

Yup, same error on 10.3-RC. Investigating now.

@jannikanker
Copy link
Contributor

Okay, so it's failing because we're using the group name as part of the source "name" for typeahead. According to https://github.com/twitter/typeahead.js/blob/master/doc/jquery_typeahead.md#datasets the name "must only consist of underscores, dashes, letters (a-z), and numbers."

So, the question is - what's the best way to fix this? I think there should be a regex validator of sorts on the tags group name property, but I'm thinking that could be a breaking change (or at least break the backwards compatibility).

We could maybe try and do a replaceAll on the name when it's set in the controller here - but that could lead to weird issues as well.

Maybe @nul800sebastiaan can advise here? Or knows someone who can? :-P I'll be happy to help implementing whatever we come up with.

Although... How long might that error have been around? If, say, using group names with spaces hasn't ever worked (or rather never been supported), do we need to worry about breaking anything?

@soreng
Copy link
Contributor

soreng commented Oct 16, 2022

@jannikanker it seems that the name should just be distinct for the sources being passed on - it dosen't seem to be used afterwards

Maybe a solution could be to just replace everything not "underscores, dashes, letters (a-z), and numbers" with an underscore, since there is only a single source.

@markadrake
Copy link
Contributor

markadrake commented Oct 16, 2022

Is there a performance benefit of initializing multiple instances with the same name? If so, we could possibly use getSafeAlias on vm.config.group.

getSafeAlias

Otherwise, I'd suggest swapping vm.config.group for something else - such as the alias of the current property. Something we know will already meet these requirements.

@jannikanker
Copy link
Contributor

@markadrake not a bad shout at all to use getSafeAlias.

I may have gotten this wrong, but say there's one group called "group 1" and another called "group1", defined in separate datatypes. Whichever way we replace the illegal characters the typeahead will then look for suggestions in both groups, right? That's not what I would call expected behavior :-/

Hence the suggestion to validate the field value itself, so that if there are multiple groups with the same name, it would be somewhat apparent to the user if they check their tags datatypes.

Maybe we should do both, to ensure that things don't break as they do now even with invalid group names from older versions, risking some "wrong" typeahead suggestions.

@jannikanker
Copy link
Contributor

jannikanker commented Oct 19, 2022

I think I'll just do a PR for getting a safe alias for the name put into the typeahead component. For now - I can't properly grep the consequences nor figure out how to add a custom validator to the group name in the tags editor, so... I am, however, on a currently very spotty hotel wifi, so it's not that easy :-D

@nul800sebastiaan
Copy link
Member

So while reviewing @jannikanker's PR I noticed a js error every time I tried to do something, which is basically the reported error.

So for some reason typeahead is null at this point, but it doesn't matter if the tag group has a space in it or not, just any tag group that is not default doesn't work. The symptom is reproducible by using a tag editor with the group name green (for example!) and making a node with a few tags on it. Then create another node and observe in your browser's network tab that the tags you created are successfully pulled in:

image

But then when you start typing you don't get any autocomplete (you do when the tag eiditor is using the default tag group though).

I spent a few hours trying to debug but I can't for the life of me figure this out! Any additional help will be much appreciated 🙏

@jannikanker
Copy link
Contributor

I can take another stab at it sometime next week, but if anyone else fancies taking a look feel free :-)

@kjac
Copy link
Contributor

kjac commented Dec 16, 2022

Seems this one has gone a bit stale, so I have created a fix in #13589

@nikolajlauridsen
Copy link
Contributor

This has been fixed in the above PR and is heading for 10.4 and 11.1, so I'll go ahead and close this 😄

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