11/UI/add async autocomplete to tag input
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, withSortable, getSortable - 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
draggableto the framework. Draggable aims to be as generic as possible, while implementing as much logic as it can. draggableis 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 bindthisto 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.draggableis 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
Hi @kergomard
We are currently a bit understaffed. However, we are looking into this asap all coordinators are available again.
Thx for your patience!
Jour Fixe, 21 JUL 2025: We highly appreciate the suggested extension of the tag input and accept it for trunk.
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\Taggetters:
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
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 https://github.com/jsdom/jsdom/issues/2949). If you have pointers, I'm a happy taker.
Best, @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
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.
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
Jour Fixe, 13 OCT 2025: We accept the additional change of the PR as described above for trunk.