11/UI/add async autocomplete to tag input

[kergomard]

Hi all

This PR contains a proposal to add the options to the tag-input to

• Load data asynchronously
• Be sorted.

I added bot option in one PR as this allowed me to keep it functional while working on it without having to move changes to the basic js-code forth and back. The two parts an in separate commits. We can split this up again, if needed.

The changes to the interface are rather small adding only four functions (withAsyncAutocomplete, getAsyncAutocomplete, withOrderable, getOrderable - see)

The implementation for the async autocomplete is also rather straight forward. Compared to the current implementation for the autocomplete text fields it does NOT allow to add a "more" option, so all possible entries need to be loaded at once. This is due to limitations in Tagify.

The implementation for sortable is more interesting as it contains many decisions that can also be taken very differently. The main point that need discussion from my point of view (there are surely more):

• I decided to add an internal facility draggable to the framework. Draggable aims to be as generic as possible, while implementing as much logic as it can.
• draggable is right now implemented as a single function containing the whole logic. This could also be done as a class, but as most of what happens in there is bound to event-handlers we would always need to bind this to a variable to be able to access it inside the handler-functions. This is also a possibility and I can rewrite this rather easily, if you want me to. In a first step I decided against it, as we would also need to assign the class to a variable (most probably an array) and we never need it again.
• draggable is rather complex, as it needs to handle three scenarios: We might be on a client that supports the draggable facility (i.e. we have a mouse), we might be on a touch client, or we might be operating the operation by keyboard. I tried to implement a solution for all of those. I also tried to implement a solution for people depending on a screen reader.

There are no tests contained, I will take care of those once we are clear if this is what we want and what the implementation is really going to look like.

What the async autocomplete looks like:
Bildschirmaufzeichnung vom 2025-05-06 14-36-23.webm

What sorting looks like:
Bildschirmaufzeichnung vom 2025-05-06 14-36-58.webm

Thank you very much and best,
@kergomard

[Amstutz]

Hi @kergomard

We are currently a bit understaffed. However, we are looking into this asap all coordinators are available again.

Thx for your patience!

[thibsy]

Hi @kergomard

Thank you a lot for contributing to ILIAS. This will be a great added value for the tags.

The interaction of sorting is smartly solved, thanks for that.

Please answer the following questions:

• Example 6 «With autocomplete endpoint»: I can’t reproduce the «Expected Output». I am not able to see a tag «Interesting» at the beginning. I also can’t select any by typing A, B, I or R. If I add a new one, it disappears directly after it was shown. Can you have a look on this, if the expected output need another description or is there something in the code?
• C\Field\Tag::withAsyncAutocomplete() I: does this feature take the existing ::withUserCreatedTagsAllowed() and withSuggestionsStartAfter() into account as well? Is this handled by the library we use?

Please consider the following suggestions. You do not need to follow those, but please indicate shortly why you prefer to do otherwise:

• It’s not part of your PR, but if we are doing something here for the Tag-Input, there are some other places, where a little change on the text would be welcome:
  • Example 5: In «Expected Output» the label of the input is called «Basic TagInput», but in the form it is called «Basic Tag». Please adjust the label or the description.
  • KS-Description «Purpose»: Is the information at the end about «[...](@see Tag::withUserCreatedTagsAllowed)» necessary to document? I can’t relate this information to somewhere in the description or Example. Is this an information that developers need and if so is it the right place in «Purpose»?
• C\Field\Tag::withAsyncAutocomplete() II: you mention in the method documentation that consumers MUST use the query parameter term. Is it at all possible to change this? We encountered some problems in the past with e.g. C\Modal\Interruptive, where we used a hardcoded value like this.

Please implement the following changes:

• Example 3 «Expected Output»: The following sentence has a missing part: «Tags can be sorted by dragging and dropping them in the ». Please add an ending to the sentence.
• Example 6: The label of the input in «Expected Output» is called «Basic TagInput», but in the form it is called «Tag Input with Autocomplete».
• There are 6 errors from HTML Validator (1 per example): «Element tags not allowed as child of element div in this context. (Suppressing further errors from this subtree.)». Thanks for taking a look.
• C\Field\Tag::withSortable(): please rename this method to withIsSortable(). This way it's' clear we are talking about a boolean state. (adjust getter as well)
• C\Field\Tag setters: please use self or static for their return types.
• C\Field\Tag getters: please remove them from the public interface.

Since these changes do not affect the public interface greatly, we already label this for presentation at next JF.

If you have any questions about something in this review, please contact us.

kindly,
@yvseiler and @thibsy

[matthiaskunkel]

Jour Fixe, 21 JUL 2025: We highly appreciate the suggested extension of the tag input and accept it for trunk.

[kergomard]

Hi @yvseiler and @thibsy

Thank you very much for your review. I went through the different points and fixed things with .

Here are a few remarks:

Example 6 «With autocomplete endpoint»:

Yeah, sorry, forgot to change the beginning of the text. Fixed

C\Field\Tag::withAsyncAutocomplete() I:

Yes, this actually uses an interface offered by the library (See), but it did not respect ::withUserCreatedTagsAllowed(). I now added this support.

KS-Description «Purpose»:

I do not really have an much of an opinion on this. I think there is some information there, as it points you to the place were you can enable/disable it. I think the place in "purpose" is neither completely wrong nor completely correct, but I think it can be explained why the information is there. I can remove it (then I would suggest to remove the whole sentence "Besides the tags to choose..."), if you want me to.

C\Field\Tag::withAsyncAutocomplete() II:

I would personally suggest to stick with this approach. I can imagine that we would add a second parameter to withAsyncAutocomplete(), but personally I think here is a good place to go with convention over configuration. What would be issues, we expect?

There are 6 errors from HTML Validator (1 per example):

The tags and tag tags come from tagify (https://github.com/yairEO/tagify/blob/5219a7ac651ddf24d5845e57cde1672b00862a0e/src/parts/templates.js#L10) and the problem is that they are no valid custom html-tags as custustom html-tags need to contain a hyphen. This would be an upstream fix as far as I can see. How would you like to proceed?

C\Field\Tag getters:

I removed all getters, I personally always thought they were wrong, but with this we are actually breaking the rules: "Put getters for all properties on your interface." see.

I assume you are now going to take a deeper look at the implementation, right? As soon as we agree on the implementation details, I will take care of tests, rebasing, and so on.

Thanks again and best,
@kergomard

[thibsy]

Hi @kergomard,

As you mentioned in #9941, you will take over @nhaagen's PR and incorporate the relevant changes to address the Mantis issue, adopt the PHPUnit tests and implement a JavaScript unit tests to cover this issue inside your new structure yourself. According todo's will be added in this review.

During the technical review of this PR, we have noticed that most of our remarks regard the ordering-feature, not the autocomplete one. We therefore suggest to move the ordering-feature into a separate PR, where we can discuss these remarks separately. An according todo will be added to the review as well.

About the concrete changes, please answer the following questions:

• Custom HTML tags: you mentioned that the custom HTML elements, which are invalid, require an upstream fix. We are confused, because the <tag> and <x> elements are added in your JavaScript code as a template for Tagify. Could you explain what you meant with upstream fix, and why we cannot simply change these elements to e.g. <div>?

Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:

• @yaireo/tagify module: you can import Tagify from the node-module. However, we are currently missing the Dependency to include such imports properly into our rollup bundles. If #10059 is merged before this, you may incorporate this change, if not, we will do it afterwards.
• UI\URLBuilder: convention, regarding the query parameter, is fine for the time being. However, we would like to utilise the UI\URLBuilder more strongly. If this change is too much, feel free to add this to our roadmap instead. The idea was to change the method signature of/to Field\Tag::withAsyncAutocomplete(URLBuilder $url_builder, URLBuilderToken $term_parameter): self and incorporate the URL-builder (PHP and JS) for building the target URL of the autocomplete endpoint. You can take a look at how the data table utilises this mechanism for its actions.

Please implement the following changes:

• tagAutocompleteTriggerTimeout (tag.js:19): please include this value inside the I\Field\Tag::getConfiguration() for now. I can imagine that this value may vary depending on the use-case of the tag input, or even on server response time. This will allow for easy modification/configuration later on. Please also incorporate some information about the unit of this value into the name of the constant (i.e. ms).
• retrieveAutocomplete() (tag.js:92): why do you wrap things inside this context object? Please pass these things as dedicated parameters, or declare them e.g. as an outer-scope variable inside your module - if this is shared.
• Probable bug: the context.timeout.signal should probably be context.controller.signal, the other value is an integer.
• il.UI.Input.tagInput (input.factory.js:40): the previous functionality was exposed in this namespace. To make sure we don't cause any breaking changes somewhere, please stick to the old property name.
• il.UI.Input.tagInput.getTagifyInstance() (input.factory.js): this method was removed. Please make sure you expose this function and hereby access to the Tagify instances.
• Mantis issue I: please pick the relevant changes of @nhaagen's PR to address the issue mentioned above. Make sure to pick the PHPUnit test as well.
• Mantis issue II: please implement a JavaScript unit test which covers the issue mentioned above, so it fits your new structure.
• Ordering-feature: please move these changes to a separate PR. We can skip to the technical review immediately, once you opened it.
• Unit tests: please implement some PHP (see our new-ish guidelines) and JavaScript unit tests.

Kind regards,

@klees, @thibsy and @yvseiler

[kergomard]

Thank you very much for the review @thibsy, @thibsy and @yvseiler

Custom HTML tags: you mentioned that the custom HTML elements, which are invalid, require an upstream fix. We are confused, because the and elements are added in your JavaScript code as a template for Tagify. Could you explain what you meant with upstream fix, and why we cannot simply change these elements to e.g. <div>?

You are right, I didn't even think about that. We now override all custom tags with divs and spans and I didn't see any side effects.

il.UI.Input.tagInput.getTagifyInstance() (input.factory.js): this method was removed. Please make sure you expose this function and hereby access to the Tagify instances.

If I'm being honest with you, I think this is really wrong as we are now back to an unnecessary book-keeping and handing out instances of Tagify directly which really ties us closely to this implementation. I implemented this, but I would kindly ask you to reconsider. I didn't find anywhere where this was used in core right now.

UI\URLBuilder: convention, regarding the query parameter, is fine for the time being. However, we would like to utilise the UI\URLBuilder more strongly. If this change is too much, feel free to add this to our roadmap instead. The idea was to change the method signature of/to Field\Tag::withAsyncAutocomplete(URLBuilder $url_builder, URLBuilderToken $term_parameter): self and incorporate the URL-builder (PHP and JS) for building the target URL of the autocomplete endpoint. You can take a look at how the data table utilises this mechanism for its actions.

I hope I changed this as you expected it. I needed to move creating the URLBuilder and the URLBuilderToken out of the JSON. This changes the interface, so we should maybe go back to the Jour Fixe (at least for information)?

JavaScript unit tests

I tried to implement something but failed miserably as Tagify relies on the full document being there and I cannot test much without instantiating an instance of Tagify, ...at least I didn't see how. I used JSDOM, but then it just failed later (the furthest I got was to the moment where an Event was raised that was of another type than the expected Event; this has happened to others too, apparently, see jsdom/jsdom#2949). If you have pointers, I'm a happy taker.

Best,
@kergomard

[kergomard]

Hi @thibsy

Thank you very much for the hint on the tests! I first had the same idea to mock "Tagify", but then gave up on it, when I thought it all too complicated.

I now added a few js-tests, and moved to importing the node module.

Sorry for the force push, there was no way around it.

Looking forward to the review,
@kergomard

[thibsy]

Hi @kergomard,

Thx for splitting up the PR and implementing the requested changes, especially the UI\URLBuilder part :).

The PR became much easier to review now, so I might have found some nitpicks after all. I added them to this iteration of the review.

About the concrete changes, please answer the following questions:

• UI\URLBuilder: It looks like I imagined. However, I am not quite sure what you mean by "I needed to move creating the URLBuilder and the URLBuilderToken out of the JSON". Which JSON? Could you elaborate? (And JF should be notified about the interface change, yes, but since it's minimal we can do this after merge.)
• @yaireo/tagify module: #10059 has been merged. It would be great if you could give this a try. The procedure should be documented.

Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:

• il.UI.Input.tagInput.getTagifyInstance() (input.factory.js): I am actually with you on this; I could not find usages of this function in core, so I am fine with removing it. I'll leave it up to you, since you implemented it already.

Please implement the following changes:

• typeof checks: please replace these 3 checks either by positive instanceof checks or by strict-comparisons against their actual possible values.
• JSDoc: there are some JSDoc annotations:
  • tag.js:26 should also be undefined
  • tag.js:30 should also be undefined
  • tag.js:35 should be string, and a * is missing on 34
  • tag.js:102 returns for void should be omitted
  • tag.js:156 should be HTMLInputElement
  • tag.js:156 should be Array (capital A)
  • tag.js:159 and tag.js:160 should also be undefined
• tag.js:20: please make this a Map instead.
• JS tests: I gave you some directions in kergomard#5, hope this helps.

Kind regards,
@klees and @thibsy

[thibsy]

P.S. the examples are not working because of the problems you outlined in #10043. Applying the PR makes the examples usable again. I'll try to look into this soon.

[kergomard]

Thank you very much for the review @thibsy !

UI\URLBuilder: It looks like I imagined. However, I am not quite sure what you mean by "I needed to move creating the URLBuilder and the URLBuilderToken out of the JSON". Which JSON? Could you elaborate? (And JF should be notified about the interface change, yes, but since it's minimal we can do this after merge.)

Sorry for not being clear on this: The problem is, that up to this point the URL was handed in as part of the parameter config that was a JSON encoded in PHP. This does not work with the Endpoint and the Token, so I had to move them out of the config even if they could be seen as part of the configuration. I think the current solution is correct, though.

il.UI.Input.tagInput.getTagifyInstance() (input.factory.js): I am actually with you on this; I could not find usages of this function in core, so I am fine with removing it. I'll leave it up to you, since you implemented it already.

I had separated this in one commit to make things easier, if you agreed with me. It is reverted.

typeof checks: please replace these 3 checks either by positive instanceof checks or by strict-comparisons against their actual possible values.

I was able to replace the AbortController. I think that works, but I didn't find a solution for the other two tests. number is a primitive. I changed the check to !== undefined. Not exactly a positive test, but I hope this goes in the right direction.

tag.js:20: please make this a Map instead.

This was removed when the commit was reverted.

Thanks again and best,
@kergomard

[thibsy]

Thx a lot @kergomard for implementing the changes. !== undefined is fine. Everything should be in order now. I will merge after JF this afternoon.

[matthiaskunkel]

Jour Fixe, 13 OCT 2025: We accept the additional change of the PR as described above for trunk.