Updated: Comment #264
Problem/Motivation
#245: Regressions: Use jQuery UI Autocomplete
- Doesn't support HTML content for the label anymore (patch for review in this queue)
- Autocomplete callbacks return HTML, but the new autocomplete expects plain-text (works as designed as per #262)
Proposed resolution
- Patch jQuery UI Autocomplete to display HTML label
Remaining tasks
- Review Patch
User interface changes
Before:
After:
API changes
---
Original report by @Dave Reid
Let's get rid of our bad, old, custom JS and replace it with something that will be more supported, easier on the bus factor, and more flexible for development.
http://docs.jquery.com/UI/Autocomplete
http://wiki.jqueryui.com/Autocomplete
http://jquery-ui.googlecode.com/svn/branches/labs/autocomplete/
Related issues
Comment | File | Size | Author |
---|---|---|---|
#265 | after.jpg | 12.22 KB | droplet |
#265 | before.jpg | 13.64 KB | droplet |
#265 | 675446-autocomplete-follow-up.patch | 1.3 KB | droplet |
#248 | jquery_ui_autocompelte.patch | 1.32 KB | droplet |
#238 | autocomplete-bartik.png | 6.38 KB | Wim Leers |
Comments
Comment #1
Alan D. CreditAttribution: Alan D. commented+1 Subscribe. There are far too many roadblocks to the old version; the current js is lacking many useful features #365241: Add select event to autocomplete feature and has many strange bugs #590148: Autocomplete.js errors with Mozilla based browsers, due to a namespace conflicts between search terms and object properties
Comment #2
webchick"former"? Why was it removed?
Comment #3
Dave ReidHuh, the last I read it had been removed from jQuery UI, but they're revamping its development for jQuery UI 1.8.
Comment #4
RobLoachUsing jQuery UI's Autocomplete widget instead of Drupal's internal one will save us a lot of custom code.
Postponed by:
Comment #5
Dave ReidThe autocomplete widget is now officially in jQuery UI 1.8 Final.
Comment #6
corey.aufang CreditAttribution: corey.aufang commentedUnless I'm missing something, autocomplete functionality is still handled by a callback in Drupal 7.
If this is the case, then multiple values can be handled in PHP as opposed to JS.
While not optimal, it would still allow us to use the benefits of jQuery UI's autocomplete functionality, and I believe be an overall reduction in custom code.
Comment #7
RobLoachhttp://jqueryui.com/demos/autocomplete/multiple.html
Comment #8
Dave ReidOh dear god please!
Comment #9
RobLoachEven better:
http://net.tutsplus.com/tutorials/javascript-ajax/how-to-use-the-jquery-...
.... msonnabaum found that one.
Comment #10
Everett Zufelt CreditAttribution: Everett Zufelt commentedI quickly tested http://jqueryui.com/demos/autocomplete/multiple.html with FF 3.6 / JAWS 11. It doesn't seem to be any more accessible to screen-reader users than our custom solution. I would argue that we should not use the jQuery autocomplete until it is accessible. For any of the down sides to using our own we at least have the ability to improve its accessibility.
Comment #11
Everett Zufelt CreditAttribution: Everett Zufelt commentedFYI: #515262: Autocomplete requires ARIA for screen-reader users
Comment #12
Everett Zufelt CreditAttribution: Everett Zufelt commentedI posted a message jQuery UI autocomplete plugin to the jQuery-a11y mailing list. I indicated that the plugin isn't accessible and that I would be willing to help. I am not sure how much the list is used since the fforums were created. The forums are poorly accessible and I refuse to use them, rather am too annoyed with them to use them :)
Comment #13
Everett Zufelt CreditAttribution: Everett Zufelt commentedExcerpt of response from Scott González:
Comment #14
Everett Zufelt CreditAttribution: Everett Zufelt commentedCan we start to put together a list of requirements for autocomplete in D8 that we can compare with the jQuery UI multiple autocomplete component so that we can assess if it will meet our needs? This would make it easier for me to prioritize effort and to participate in the process of helping to improve the component.
Comment #15
marcoBauli CreditAttribution: marcoBauli commentedthe jQuery autocomplete supports copy/paste search criteria too... nice :)
A starting point for requirements can be found in the older issue "Enhance autocomplete feature" here: http://drupal.org/node/125231 please let me know if is useful that i put together a wrap up of things asked there.
Comment #16
Everett Zufelt CreditAttribution: Everett Zufelt commented@Marco
It would be useful to have a summary of requirements from the other issue. Please categorize them as either requirements (must have) and features (nice to have).
Thanks
Comment #17
Cyberwolf CreditAttribution: Cyberwolf commentedsubscribing
Comment #18
sepgil CreditAttribution: sepgil commentedsubscribing
And just in case someone needs the jquery autocomplete behavior for drupal 7: I've coded a module based on it: Autocomplete deluxe.
Comment #19
RobLoachThis patch replaces autocomplete.js with jQuery UI's Autocomplete. Works with multiple terms, keeps the same throbber loading design, etc.
Benefits of using jQuery UI instead of our own autocomplete.js:
Note that this requires at least jQuery 1.5 to run though (#1085590: Update to jQuery UI 1.9).
Comment #20
RobLoachFancy issue tag.
Comment #21
wmostrey CreditAttribution: wmostrey commentedComment #22
Dave Reidnot sure why we needed a term change, but ok but thanks for pinging everyone
Comment #23
mgiffordDave, well, Proudly Found Elsewhere is a nice spin on Not Invented Here. Kinda funny.
However, I thought I'd point folks to http://hanshillen.github.com/aegisdemo/
Which has a jQuery autocomplete function that has been well tested for accessibility. I do hope that this gets into jQuery's releases, but wanted to make the connection here anyways. And ya, I've posted it a few places as the demos there are pretty good. Sorry.
Comment #24
Dave ReidYeah Wim forgot to copy/paste the context of the term change in the comment. :)
Comment #25
Alan D. CreditAttribution: Alan D. commentedsub.
Comment #26
Everett Zufelt CreditAttribution: Everett Zufelt commentedI will be taking part in the jQuery ARIA hackathon July 11 - 12. If I can get a list of what we require in autocomplete I can see if I can get some people to spend some time ensuring that those features, at the least, are accessible.
Comment #27
klonos...subscribing.
Comment #28
mgiffordEverett, how did the hackathon go? Any good results from the discussion?
Comment #29
Everett Zufelt CreditAttribution: Everett Zufelt commentedI was unfortunately unable to attend, work got in the way.
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commentedI did a very quick (30 sec) test of http://jqueryui.com/demos/autocomplete/#default using JAWS 12 and FF 6 / IE 8.
This still needs some work, I could move through the list of options and tab to select, but I wasn't notified of the search / appearance of the suggestions. If a screen-reader user isn't notified of the suggestions then they have no idea they are in an autocomplete UI control or that suggestions have appeared, as there is no access to the visual affordance, namely a list appearing on the screen.
As far as an accessibility comparison goes the jQuery autocomplete currently falls somewhere between what we have in D6 and what we have in D7.
Comment #31
mgiffordWork can totally get in the way of a good thing. Too bad that the autocomplete demo doesn't exceed what we have accomplished in D7.
So someone must have some idea of the results of this effort:
http://wiki.jqueryui.com/w/page/38817541/ARIA%20Hackathon
I searched the list but only found budget requests.
http://groups.google.com/group/jquery-team-public
Someone must be known on the jQuery list and can enquire what the result was and what the chance of advancing this ahead in the next year.
Everett, you know anyone engaged with the jQuery accessibility initiaive?
http://groups.google.com/group/jquery-a11y
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commented@mgifford I'll ask Colin Clark
Comment #33
RobLoach@mgifford: Was great getting the chance to hit Drupalcon with you. Wish I knew about that ARIA Hackathon last month in Toronto, would've loved to have gone. Everett is the only one who reached the jQuery UI team on the mailing list regarding ARIA accessibility in the library. There is an initiative out there to help with accessibility though. Since then though, the conversations have moved to the jQuery Forum: http://forum.jquery.com/developing-jquery-ui .
@Everett: If you post some of the issues you ran into, I could try to find the related issues in the jQuery UI queue and try to get patches going. Thanks.
Comment #34
Everett Zufelt CreditAttribution: Everett Zufelt commented@Rob
The biggest issue, from my super fast review, was that screen-reader users have no idea that they are in an autocomplete field, or that the search suggestions have appeared.
In D7 we accomplished this with an aria live region, hidden w/ element-invisible (of course this will be visible to all w/ CSS turned off, but I'm sure that autocomplete is ugly then anyway).
When the search begins the text "Searching" is placed in the live region, when the suggestions are visible the text "Autocomplete popup" is placed in the live region. Any screen-reader supporting live regions will read this text.
I'm not quite sure how jQuery UI is making the suggestions read while arrowing through the list, my suspicion is that they are doing text replacement into the edit field. This is buggy (I experienced some bugs) as sometimes pressing the down arrow will take a screen-reader user out of the edit field. We solved this in D7 by wrapping the widget in a div w/ role="application". When in a region w/ application role screen readers pass most keyboard interaction to the browser, instead of using it to interact with the virtual buffer that is made of the page.
At the time that we committed this I mentioned that it wasn't the most elegant solution, but it does the trick for the moment.
Comment #35
gagarine CreditAttribution: gagarine commentedIt was postponed in #4 but
#5326: jQuery UI Autocomplete doesn't support multiple itemsis an invalid bug (not the right documentation). In fact autocomplete support multiple items http://jqueryui.com/demos/autocomplete/#multipleAbout the ARIA problems: It's definitely less work to fix this on jqueryUI than maintaining custom code. With the benefit than all projects using jquerUI will be more accessible.
Comment #36
RobLoachThe patch at #19 still applies cleanly and works, and supports multiple items as gagarine mentioned. It's postponed because jQuery is extremely out of date: #1085590: Update to jQuery UI 1.9
We need people to review that patch, and get it in before this one works.
Comment #37
RobLoachRE: ARIA. It looks like the Autocomplete plugin is getting a bunch of ARIA classes and attributes in both the textfield itself, and the autocomplete pop up. I personally haven't tested in the ARIA software, but it does look quite promising.
Comment #38
Everett Zufelt CreditAttribution: Everett Zufelt commented@Rob
Is there a demo site where I can test the same version of jQuery UI autocomplete that we have in 7.x, or expect to have in 8.x?
Comment #39
RobLoachHere you go!!! http://8.robloach.net .... This is on a shared server, so don't get too pissed off if you see some slow performance :-) .
Comment #40
Everett Zufelt CreditAttribution: Everett Zufelt commentedI tested http://8.robloach.net very quickly using FF 6.0.2 / JAWS 12.
In the programming language field I typed:
j - nothing happened.
a - nothing happened
v - nothing happened
up / down arrows - nothing happened
tab - jav was in the field.
* nothing happened == I perceived nothing.
Comment #41
webchickThanks for testing, Everett. Could you explain how that experience differs from using the current autocomplete field in Drupal 7? (i.e. what you would expect)
Comment #42
Everett Zufelt CreditAttribution: Everett Zufelt commented@webchick
Using the D7 autocomplete:
1. Start typing.
2. Screen-reader announces 'Searching for matches...' when throbber starts.
3. Screen-reader announces 'Autocomplete popup' when the results appear.
4. Screen-reader announces each result when arrowing up and down through the list.
5. Pressing tab with a result selected populates the field with that result and moves focus to next focusable element.
Comment #43
RobLoachSorry about that, partly my fault. I limited the auto-complete in this patch to only search if you typed more than two characters. I thought I documented that on there, guess not... Try typing "Vi", or "Ja".
Comment #44
Everett Zufelt CreditAttribution: Everett Zufelt commented@Rob
I also typed a, v (that is to say 'jav' in all. :)
Comment #45
RobLoachWeird, this is what I get...
Comment #46
wroxbox CreditAttribution: wroxbox commentedMy 2c for this discussion.
http://www.lullabot.com/articles/module-monday-chosen reminds of http://harvesthq.github.com/chosen/ Great usability.
edit: Duplicate #1271622: Seek a better autocomplete replacement for core (jQuery TokenInput / Select2 / Typeahead.js by Twitter)
Comment #47
klonos...just noting that the implementation in the demo site allows to enter the same thing(s) more than once. Don't know if this is intentional, but I think it shouldn't happen.
Comment #48
scott.gonzalez CreditAttribution: scott.gonzalez commentedCan someone, perhaps Everett, list out the current problems with single word autocomplete in jQuery UI either on http://groups.google.com/group/jquery-a11y or http://forum.jquery.com/developing-jquery-ui or in IRC (#jqueryui-dev on freenode)? We have a known issue where the ARIA from the menu isn't used because we keep focus in the text field. However, because the default behavior is to update the value of the text field as you navigate the menu, it appears to work, even though it's not doing what we really want. This is the cause of multi-word autocomplete completely failing for accessibility tests, since we don't update the text field. If we can work through the single word issues, we might get multi-word support automatically.
Comment #49
Everett Zufelt CreditAttribution: Everett Zufelt commented@Scott
Can you please provide me a link to a demo of the most recent stable snapshot of jQuery UI autocomplete? It doesn't need to be a release, since we are planning this for the D8 release, which is quite a way away.
Comment #50
nagiek CreditAttribution: nagiek commentedCan't wait for this! Would love to see
formatItem
,formatMatch
, andformatResult
support!http://docs.jquery.com/Plugins/Autocomplete/autocomplete#url_or_dataoptions
Comment #51
nagiek CreditAttribution: nagiek commentedSee #1322664: Add support for formatResult, formatItem and formatMatch in autocomplete.js
Comment #52
Everett Zufelt CreditAttribution: Everett Zufelt commented@Scott pointed me to http://view.jqueryui.com/ off list. I have a tentative plan to do a full jQuery UI accessibility evaluation, including autocomplete, the week of Nov. 7.
Comment #53
mgiffordThis is totally a good call. I've posted a note about this and hope to get some more feedback & encourage participation.
Comment #54
Everett Zufelt CreditAttribution: Everett Zufelt commentedSee http://zufelt.ca/blog/jquery-ui-accessibility-review-whats-plan#comment-334 for a very quick review of jQuery UI autocomplete 1.9pre (master) w/ Firefox 7 and JAWS 13.
Still not suitable for D8, but can possibly be fixed w/o too much effort.
Comment #55
RobLoachThanks for the review, Everett. Could you put together an action list of ARIA roles that would have to be added to the markup to help improve this? Considering there's an abundant lack of JAWS/ARIA markup documentation around, it makes it really hard to come up with a solution around it. Without a proposed solution, this just becomes a huge bikeshed.
Thanks.
.... Also, here's a review of your review:
The custom data demo is a demonstration of returning custom data back and displaying it in a fancy way. You can see this example gives you a title and a description below it. We won't be using this in Drupal's autocomplete widget.
The Categories demo shows you how to split results into different categories. It gives you different results in a set number of categories. Your listing of "andreas johnson" is provided under the "People" category. Again, this is not something we'll be using for the Drupal autocomplete widget.
Are there any roles that we could inject here to help the process? You're not really providing any direction. The demo for 1.9m6 gives you the "aria-activedescendant" attribute that will tell you where it is.
..... So, from the autocomplete review, it sounds like you ran into three issues. Two of them were issue with you not understanding what the demo was, and the third I have no clue what is wrong. More details and direction would be great.
Comment #56
Everett Zufelt CreditAttribution: Everett Zufelt commentedRob
Thanks for the feedback. As the article clearly states I will be filing bugs against the relevant components at the end of each day. I will provide a link to the bug(s) filed against autocomplete here.
Comment #57
RobLoachAwesomesauce!
Comment #58
Everett Zufelt CreditAttribution: Everett Zufelt commentedhttp://bugs.jqueryui.com/ticket/7840
Comment #59
RobLoachKeeping up to date now that jQuery 1.7 is in there... Would be nice to have:
Comment #60
Everett Zufelt CreditAttribution: Everett Zufelt commentedAs far as I know, jQuery UI autocomplete will not be accessible enough for Core until at least 1.9.
Comment #61
Everett Zufelt CreditAttribution: Everett Zufelt commentedhttp://bugs.jqueryui.com/ticket/7840#comment:9
Could use review and feedback.
Comment #63
quicksketchEverett, does that mean that our current autocomplete is better (accessibility-wise) than the jQuery UI autocomplete? If this is an improvement from the current situation, it seems like it should go in whether it's fully accessible or not.
Comment #64
Everett Zufelt CreditAttribution: Everett Zufelt commented@quicksketch
Drupal 7 autocomplete.js is significantly better than the current jQuery UI autocomplete. We are going to attempt to follow the Drupal 7 approach in jQuery UI, I just don't have enough hours in the day to re-roll the current jQuery UI patch.
Comment #65
sun1) While a minimum of 2 sounds good as default, the minimum term length should be configurable per autocomplete widget.
2) It looks like the code does something else than the comment states; it checks for non-zero length instead of 2.
I'm seriously surprised that all of this custom code is needed...? That belongs to the bare essentials of autocomplete functionality, so why is that needed, and that is jQuery UI's Autocomplete plugin actually doing, if we even need custom code for the most basic functionality...?
Not sure whether this is needed.
This looks like a regression to me - Drupal's autocomplete syntax supports commas within terms; e.g.:
Support for the term delimiter within terms is one of the main problem spaces for autocomplete/string parsers. That's why approaches like jQuery Tokeninput or jQuery UI ListBuilder, which convert and shift away all the existing terms outside of the input field, are much more reliable and mature.
Comment #65.0
sunUpdated issue summary.
Comment #66
mgiffordTagging. Also wanting to point folks here - http://drupal.org/node/1175830#comment-5747762
jQuery's the right place to do this.. So let's make sure jQuery's accessibility keeps ahead of (or at least keeps up with) Drupal's.
Comment #67
Everett Zufelt CreditAttribution: Everett Zufelt commentedI haven't done full testing, but I did test a bit, and it looks like the autocomplete now in jQuery-UI 1.9 (master branch) will have sufficient accessibility for D8.
http://bugs.jqueryui.com/ticket/7840
Comment #68
RobLoachLet's post-pone this for #1085590: Update to jQuery UI 1.9 then :-) .
Comment #69
g10 CreditAttribution: g10 commenteddon't forget the free-tagging taxonomy feature, chosen is very nice, but does not have the option for free-tagging (to my knowledge)
tag-it would be very nice: https://github.com/aehlke/tag-it
Comment #70
webchickAccording to Scott Gonzalez from the jQuery UI project, the autocomplete widget in 1.9 should have the accessibility concerns addressed, so as soon as 1.9 is out, we should be able to re-open this.
Comment #71
RobLoachComment #72
nod_updated patch, JS more readable and added a detach function. Named function my not "look good" but are better for profiling and debug.
Still doesn't work since JSON is busted in HEAD.
Comment #73
RobLoach#1619446: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON)
Comment #74
RobLoachComment #75
RobLoachTagging.
Comment #76
sun1) The summary could use some love. It's too cryptic, and due to the typo, almost wrong. We can concisely describe in one short sentence what is being converted into what.
2) That said, this does not seem to properly cater for quoted tags anymore? E.g.: Rome, "Karlsruhe, Germany", Vienna
3) A huge WTF on these anonymously-scoped functions! You do realize that split() is a native JavaScript function? This coding style makes it impossible to understand whether this split() is overriding the global/native split() within the scope/closure or not. It's also entirely not clear how split() ends up as Drupal.autocomplete.split() in the end. You have to read the entire file in order to figure out what the effect of some code in a particular context is. I'm not sure who is proposing this (I guess @nod_?), but anyway; I strongly, wholeheartedly disagree with this style for above reasons.
There's more, but I stopped after this.
Comment #77
nod_Yes, it was me.
1) I'm not good at comments, feel free to make it/them better. I changed the confusing
split
name toautocompleteSplitValues
for the function.2) 8.x head don't cater for quoted tags either, or I don't know where the new autocomplete breaks that, can you elaborate please? I've played around with it and couldn't find a significant difference.
3) I don't understand what you mean by "anonymously-scoped functions", these are all named functions. The split function is indeed confusing, I fixed that as 1) advised. I don't see how it can be mistaken now. As for
Drupal.autocomplete = Drupal.autocomplete || autocomplete;
, this will go away. It will be replaced byreturn autocomplete
or alike once we have JS modules also, usingautocomplete
throughout the file minify better than usingDrupal.autocomplete
.I have a feeling you're mainly not liking the syntax since, beside the quote issue, there is no functional problems. Having named function helps with debugging and profiling. As a maintainer that's a very big concern since if this breaks at some point, core and contrib will have an easier time reading through a stacktrace or a profiling dump.
I moved the
autocomplete
definition to the top, since function declaration are hoisted it will work. I have a problem with relying on this but if it can make things clearer let's go with that.Please go on with your comments so that they can be addressed as quickly as possible, we don't have the luxury of time regarding JS patches. Thanks for the feedback.
Comment #78
droplet CreditAttribution: droplet commentedfew suggestions.
#1.
can it use different vars name ? sourceHanlder, extractLastHandler..etc
#2.
can it add cache backend to "source". reduce same ajax request.
#3.
fixes
#4.
Maybe need a follow up issue to hack Seven and Bartik themes' popup styles.
(Sadly, using IME and typing 3 characters in normal speed, requested 2 times. Doesn't seem any good way to prevent it.)
Comment #80
nod_Yeah the naming could use some work.
source
is not an event that's why the function didn't get aHandler
suffix, same thing forextractLast
. It'll be confusing if we name event handlers and helpers the same way.For the requests we could use a throttling function for this kind of things.
Comment #81
droplet CreditAttribution: droplet commented2.
they can reset the cache object.
array is case sensitive in javascript.
Comment #82
nod_What I meant was: what are the rules for resetting the cache? how can we decide the cache needs to be rested?
Here is a scenario where caching can be an issue:
Let's say you want to add the tags: "Apple" and "April". When you open the page only the "Apple" tag already exists. If you type either "ap" or "Ap" you will get "Apple" as part of the result. Now you cache the results.
Now somewhere else on the site, someone adds the "April" tag.
If you search for "ap", because you already have a cache, "April" will not show up and you might end up writing "april", duplicating an already-existing tag because of the lowercase.
Now this might not be relevant, but we need to address that one way or an other either saying "don't care, let's cache" or "that's important, don't cache".
Keeping in mind that this can be overridden by contrib at any point if caching is necessary or if caching is not something we want. It can even be a
data-
attribute setting on the input tag to toggle the functionality.Comment #83
RobLoachTheming was over at #896728: Tweaks to jQuery UI Seven theme and #614494: Give Seven a nice looking default jQuery UI theme. Not sure where the Bartik issue is though. Same patch as #78, with the whitespace fix.
Comment #84
droplet CreditAttribution: droplet commented@nod_,
I see. for real time app and large sites, that's a problem.
@Rob Loach,
"jQuery UI hover effect & round radius", do it what we want? If Yes, that's no problem :)
Comment #85
nod_Still not happy with having sourceHandler and extractLastHandler, can we rename that sourceData and extractLastValue or something? Handler should be kept for actual events callbacks.
And from the JSHint issue we can't put the autocomplete object definition at the top of the closure. since the functions needs to be defined before being used.
Comment #86
Alan D. CreditAttribution: Alan D. commentedIf you change the widget to anything other than the autocomplete, the results are fixed, so big vote for "don't care, let's cache" from me! Small sites would have little chances of this happening, large sites would normally have minimal chances of this too as the source pool would quickly grow to contain most matches.
And on big sites, performance normally sucks, so no caching means you have a second or so delay getting results that would otherwise be cached!
Comment #87
nod_Agreed with that, let's open up another issue about that maybe? We got some details to work out like the cache format, what to chache, when to invalidate (maybe after 1 minutes since the last interaction with the field or something). If we can do that without delaying this one to much we can discuss it here, I don't really care.
Comment #88
droplet CreditAttribution: droplet commentedalso, can it use $.ajax instead of shorthand.
Comment #89
RobLoachThis patch fixes the strict warnings from JSLint. It also moves the minimum length of terms from a magic number to an option in the autocomplete object itself.
Comment #90
nod_moving some things around. autocomplete minify better than Drupal.autocomplete, about 10% better.
#88 totally need that stuff. $.ajax and an option object is the way to go. It's not in this patch.
Still need work.
Comment #91
nod_Comment #92
droplet CreditAttribution: droplet commentedWhen working on other issue, I realized the old script also cached search terms. It should be a new issue if it going to remove it.
Comment #93
RobLoachAnything else missing here? :-) We're getting pretty close.
Comment #94
nod_I think we just need to make it use
$.ajax
and exposing it's options inDrupal.autocomplete
.Comment #95
nod_Simpler title, since we don't replace autocomplete.js, we're just changing what's inside (ok, ok, I the real reason is that it's kind of too long for my cleanup recap table).
What needs to happen is to add an object to autocomplete variable with all the default ajax settings, merge that with whatever is in Drupal.settings for the current autocomplete field and make sure everything is exposed. That way we can use json, jsonp, anything jQuery provides for Ajax request. tagging as novice since it's mostly moving objects around might be a bit tricky for novice but you gotta start the harder things at some point :)
Comment #96
RobLoachAlong these lines?
Comment #97
droplet CreditAttribution: droplet commented- #94 $ajax
- #94 exposing options
- #76 tagging: Rome, "Karlsruhe, Germany", Vienna {this is broken in D7 too}
#992020: Taxonomy autocomplete does not respect cursor position.
#1329742: Autocomplete with tagging silently discards invalid input
- NEW: on tagging field -> select 2nd tag will replace 1st tag (that means you can only add ONE tag)
Anyone re-evaluate Chosen-like lib for tagging. I think it is more user friendly and don't need to write a "tag split parser" twice in PHP & JS.
#1271622: Seek a better autocomplete replacement for core (jQuery TokenInput / Select2 / Typeahead.js by Twitter)
( reference issues for more information only, not directly to JS/this issue. Above lists are real JS problem. )
EDIT: I write this comment alomst same time to #96 Rob Loach, #94 issues may sloved
Comment #98
RobLoachThis patch...
I'd consider #992020: Taxonomy autocomplete does not respect cursor position. and #1329742: Autocomplete with tagging silently discards invalid input out of scope for this issue and something we should look at after this gets in.
Comment #99
droplet CreditAttribution: droplet commentedYeah. all rel links are out of scope, just for more info.
This one broken in D7 as well, maybe also fix in a new issue ?
anyway, above regex will not work perfectly, eg:
http://jsfiddle.net/XPRNL/
I suppose use JS split and preg_split / str_getcsv in PHP would get better result. at least it won't drop strings.
Comment #100
droplet CreditAttribution: droplet commentedThis is the best I ever seen: regex
Comment #101
clemens.tolboomBoth 8.x and patch applied from #98 there is no autocomplete in Firefox but works on Chrome. Is this what @Everett Zufelt in #40 remarked too?
Steps done.
1. Add a term: admin/structure/taxonomy/tags/add
2. Add Article: #overlay=node/add/article
3. Type letter in Tags files
3a. FF: nothing happens with spinner
3b. Chrome: Spinner + autocomplete works
4. Test the file upload which works in both FF and Chrome.
Is is due to #1420798: autocomplete.js clean up?
Comment #102
Jelle_SRegarding #101, I've been debugging this together with @clemens.tolboom. Autocomplete seems to work, but still seems kind of buggy. It only reacts after entering two letters, while you'd expect it to react after entering the first letter right away...
Comment #103
Jelle_SLooking at the options passed, this behavior seems 'normal', but I'm wondering why the
minLength
option is set to 2. With the current autocomplete, you get results right after entering the first letter, so I would keep this behavior, so we don't confuse people (like me and @clemens.tolboom ;p)Comment #104
clemens.tolboomWhy do we needs this toSource?
Is it better to add a flag toSource flag then using the magically existance of toSource?
In FF I get TypeError: autocomplete.splitValues(terms).pop is not a function
[Break On This Error]
return autocomplete.splitValues(terms).pop();
toSource works here ok. Although the magic works is weird.
Comment #105
klonos...are we backporting this to D7?
Comment #106
oxyc CreditAttribution: oxyc commentedtoSource returns a string, not an array. See https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Obj...
Because of this there's no .pop() function later on
the data should be stored in the cache, after it's been converted to an array. Currently it will fail on a cache hit as object is returned when an array is expected.
Hmm this could be simplified into
$autocomplete.autocomplete(autocomplete.options);
As well as this into
$autocomplete.autocomplete('destroy');
Comment #107
Jelle_Sindeed, that was the point we were trying to make in #101 through #104
Comment #108
clemens.tolboom@oxyc: where is that cache behavior defined? I mean the object/array/string mixing ?!? Shouldn't the cache have a defined behaviour?
And why is this toSource used in the first place?
:-)
Comment #109
oxyc CreditAttribution: oxyc commentedI rerolled it to apply to current HEAD (i suck at git so I did not do this how it's supposed to be done :D)
This includes what I mentioned in my previous review. @clemens.tolboom hmm maybe autocomplete ui accepts both objects and arrays as responses, but anyway as we at one point returned it as an array I think we should keep consistent, you can see the update in the interdiff.
Also I added a fix not to error out if results aren't found
+ return value.match(/("[^"]+")|(\b\w+\b)/g) || [];
Basically if no match is found, instead of returning null, we return an empty array.
I noticed two more issues I dont really know how to address:
1. JShint complains about "Possible strict violation" due to us using this within a non-capitalized function. This is no violation as jQuery ui binds this.
2. We should probably not output html entities as follows:
I found "fix" but it looks kinda ugly, http://stackoverflow.com/a/3490609/319855
Comment #110
oxyc CreditAttribution: oxyc commentedI dont think this is a dependency now right?Scratch thatComment #111
Jelle_Scan someone explain to me why we are using complicated (and disfunctional) regular expressions here in stead of something way more simple like
$.map(value.split(','), $.trim);
?With this patch when I create an article and I have two terms (Term1, Another Term) and I enter them both, the js changes it to
Another, Term, Term1
...Comment #112
oxyc CreditAttribution: oxyc commentedOops I forgot to add the form.inc file to the commit diff.
@Jelle_S hmm true matcher definitely needs to be more robust.
Comment #113
Jelle_STrue, but when creating an article, the description clearly states:
Enter a comma-separated list of words to describe your content.
, but, when you have a space in one of your tags, it breaks. (which is kind of weird, because before this patch it didn't even though it's the same regex...)I still think this should be set to 1. When this patch isn't applied, autocompletion is triggered from the first letter.
Also, you can select the same value more than once, which isn't possible without this patch. Screenshots:
Before patch:
After patch:
Comment #114
clemens.tolboomThis patch should not change the behavior of D7 imho.
It currently does by waiting for 2 characters already :/
Comment #115
Jelle_SUnfortunatelly my weekend is very busy starting right about now. But if I find the time next weekend I'll try to have a look at the old autocomplete.js to see how the space vs comma issue was handled (because tags with spaces in them DID work there...), and I'll also have a look at how they filtered out already selected terms.
Of course, if anyone else finds the time to do that, feel free to do so.
PS: @oxyc sorry for all the critique, I don't mean to bring you down or anything because your work is really appreciated ;-), it's just some small/weird bugs :-)
Comment #116
oxyc CreditAttribution: oxyc commentedI appreciate the reviews Jelle_S, apparently I was slacking a bit with my testing.
I revised a patch, according to your suggestion about simply using comma separation, haven't checked how d7 did it either and wont have time to look into it anymore atm.
I also fixed the "bug" when autocomplete keeps suggestions terms already tagged.
[Edit] Probably needs more testing and more polishing, ie. I just noticed I'm missing one semicolon and have a trailing whiespace
Comment #117
oxyc CreditAttribution: oxyc commentedComment #118
sunOnce again, a simple split by comma or whitespace is not sufficient; see #65.
Comment #119
Jelle_S@sun true, but we need to find a reliable regex, because the current one doesn't seem to cut it either.
Comment #120
clemens.tolboom@sun the issue summary does not tell anything but to switch over to jquery autocomplete.
The feature mentioned in #65 and again in #118 is a real feature request. I do agree both ListBuilder and TokenInput looks way better just autocomplete but this sound like another issue right?
The ListBuilder PR for jquery_ui was rejected https://github.com/jquery/jquery-ui/pull/673 so how should we handle a component like that?
Same goes for jQuery Tokeinput which has a GPL & MIT license but is not part of jQuery UI either.
I think we should create a new issue for using either of those components so we can focus here to just using jQuery UI Autocomplete.
I hope I'm wrong but then the Issue Summary needs much more body and explanation.
Comment #121
sunWell, we don't necessarily need an entire second library for that (although it probably doesn't make sense to re-invent the wheel if there is one), but we definitely need a proper tag/term extraction for the autocomplete widget. That's not a new feature. Instead, omitting it would be a serious regression, since users won't be able to tag/untag/maintain all of their taxonomy term tags anymore.
The relevant backend function for tag extraction is drupal_explode_tags(). The initial inline comment in there describes the supported syntax for extracting tags delimited by commas (
foo, bar
), with built-in support for spaces (jQuery UI
), escaped commas ("Karlsruhe, Germany"
), and escaped quotes ("Daniel ""sun"" Kudwien"
).It's not entirely clear to me where this is handled in the current autocomplete.js. It's possible that the current simply doesn't care and just sends the entire string to the backend, and rebuilds and repopulates the entire list of tags based on the properly parsed backend response. (Alternatively, it's also perfectly possible that the current autocomplete.js diverged from the backend [which did not change since D6] and thus may be broken, since we don't have JS/frontend tests yet.)
If all fails, we might have to port the regex + parsing of drupal_explode_tags() into JS (which actually doesn't look very hard to do).
Comment #122
clemens.tolboom@sun thanks for some more context :)
Taken the examples from drupal_explode_tags
the whole string is send to the backend.
Deleting the first letter of the first tag (this) gives the last tag (foo bar) which is a WTF.
So I fully agree this can be done better and should be.
Comment #123
clemens.tolboomAttached is a buggy experimental version following along jquery token input. I failed to get it working in total:
The distinction between taxonomy and ie user autocomplete lies in it's multiplicity. Why haven't we a separate form widget for taxonomy? Getting rid of this would save us a split function in both js and php.
Problems I did not solve is how to transfer data from Drupal to JS for each auto complete field when we have no split. This should be json data through
then picked up through javascript
The binding above tries to saves all autocomplete fields with the same items. I did not manage to solve this.
How can we distinguish between a single value and multivalue autocomplete?
Comment #124
clemens.tolboomHere's a diff and some more remarks regarding multiple values
One solution would be to give autocomplete widget an indication to allows for multiple values.
- change form API?
- we still need magic split and join functions
Having JS disabled the user must know the split function as defined by drupal_explode_tags which is quite bad :/
- should the widget provide more text fields?
- this just sounds like multiple autocomplete fields for taxonomy. That is make autocomplete a single field and taxonomy using multiple instances.
How must data transfered between Drupal and JS without the need of a 'magic' split and similar join function?
- having multiple fields would help.
- Using #attributes and adding 'terms' of 'values' would that be enough?
I cannot build this myself as my Form API and Fields API knowledge is not good enough (yet?).
Comment #125
nod_Guys, can we focus on the issue please? upgrading the UX belongs in an other issue. Let's make a dumbed down version with a dumb split for now, let's get a decently working patch in and go from there.
This issue is to get rid of our custom autocomplete.js, that's the aim of the patch. The splitting thing can be a follow-up, It's a big enough change to put autocomplete UI in here. There are already 124 comments here and a patch somehow working since, you know, #59.
Thank you.
Comment #126
clemens.tolboomSee also another nice(r?) library for multiple select #1271622: Seek a better autocomplete replacement for core (jQuery TokenInput / Select2 / Typeahead.js by Twitter) tnx to @Bojhan
Comment #127
clemens.tolboomWe need to translate the regex used by drupal_explode_tags into javascript.
has a multiline variant (as we use the /x modifier) with added documentation like
I haven't found the look ahead construct
(?>...
on http://www.php.net/manual/en/regexp.reference.assertions.php(See http://www.php.net/manual/en/regexp.reference.subpatterns.php and ie #733192: Tokens enclosed in [ ] are not recognized comment #20 were I tried to document a regex)
So we just need to rewrite this into javascript :p
[edit] See http://www.w3schools.com/jsref/jsref_obj_regexp.asp [/edit]
Comment #128
Jelle_Sattached patch doesn't use regex for splitting but works with custom function, works with comma's within quotes and escaped quotes mentioned in #121. Reviews and manual tests needed, patch based on #116
Comment #129
nod_tag
Comment #130
Angry Dan CreditAttribution: Angry Dan commentedI noticed a minor issue with the styling of the menu items. I've removed the negative margin applied to each of the items in the auto-complete drop down which stops the items from resizing as you move down the list.
Everything else looks good, couldn't find any issues in Firefox (Mac)
Comment #132
mgiffordI'm having trouble applying this code locally.
Comment #133
Jelle_SReroll
Comment #134
Jelle_Sstatus...
Comment #135
mgifford#133: core-js-ui-autocomplete-675446.patch queued for re-testing.
Comment #136
mgiffordSeems like a worthwhile addition.
Comment #137
attiks CreditAttribution: attiks commentedFYI: #1801456: Remove jQuery UI from core
Comment #138
mgiffordThanks @attiks I'm watching that too..
Now, I could be completely wrong on this, but couldn't we just bring in certain elements from jQuery UI? Or is it all or nothing?
Comment #139
mparker17When testing the patch in #133 on IE8, it just sits there and spins forever.
Using Voiceover on Mac OS/X 10.7.5, there's no indication that matches are being suggested. You can navigate to them with the arrow keys, but it does not read the suggestions aloud.
Comment #140
webchickThat sounds like a "needs work."
Comment #141
scott.gonzalez CreditAttribution: scott.gonzalez commentedThis sounds like an issue in this specific implementation, but if you're having trouble tracking this down, let me know and I'd be happy to help.
Which browser were you testing in? Do you get the same behavior with http://view.jqueryui.com/master/demos/autocomplete/default.html? I'm also on OS X 10.7.5 and VoiceOver is properly reading the live region for me (testing in Safari and Chrome).
Comment #142
deviantintegral CreditAttribution: deviantintegral commentedDid some browser testing:
Works fine in Chrome and Firefox
Mostly works in IE9. There's a minor visual artifact where the throbber is showing. See the attached screenshot
It's broken in IE8, the autocomplete callback never finishes.
I also tested with Voiceover using Chrome, and according to mgifford it's doing everything it should for being accessible. Would be great to test with other screen readers, but probably need to fix the IE issues first.
Comment #143
mparker17@scott.gonzalez Sorry... I should have specified that I was using VoiceOver on Mac OS/X 10.7.5 in Safari 6.0.2.
I got different results on the demo page you mentioned: if I enter the letter "a", VoiceOver tells me that there are 10 results available and that I can use the up and down arrow keys to navigate.
I'm going to try re-applying the patch and testing again.
Comment #144
mparker17Interesting. I tried testing with VoiceOver again, and it partially worked. Here's what I did:
git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git
Administration -> Configuration -> Development -> Performance
and clear caches.Administration -> Structure -> Taxonomy
, click the drop-down in the Operations column beside theTags
vocabulary and chooseadd terms
.Administration -> Content
, clickAdd content
then Article.Tags
field and type the letter "a".Perhaps this is a bug in VoiceOver?
Comment #145
droplet CreditAttribution: droplet commentedre #142: #1069152: Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Comment #146
scott.gonzalez CreditAttribution: scott.gonzalez commented@mparker17: That's interesting. Are you able to create a reduced test case outside of Drupal that shows that behavior?
Comment #147
mgifford#133: core-js-ui-autocomplete-675446.patch queued for re-testing.
Comment #149
andypostpatch needs re-roll
PS: is there any changes in discussed select2 implementation, commerce kickcstart now ships with Chosen
Comment #150
mgiffordHadn't heard of select2, but by using WAVE & looking at the issue queue both it & Chosen need lots of accessibility work:
http://ivaynberg.github.com/select2/
https://github.com/ivaynberg/select2/issues/search?q=accessibility
http://harvesthq.github.com/chosen/
https://github.com/harvesthq/chosen/issues/search?q=accessibility
And ya, I figured it would need a re-roll, but was hopeful.
Comment #151
mgiffordJust a re-roll.
Comment #152
mgifford#151: core-js-ui-autocomplete-675446-151.patch queued for re-testing.
Comment #154
mgiffordReroll.
Comment #155
mgiffordInterdiff between @Jelle_S's patch in #133 & mine in the last post. Includes rejected text.
Comment #156
mgifford#154: core-js-ui-autocomplete-675446-154.patch queued for re-testing.
Comment #158
mgiffordre-roll. I think core/misc/autocomplete.js will need some work.
Comment #159
mgifford#158: core-js-ui-autocomplete-675446-158.patch queued for re-testing.
Comment #161
mgiffordreroll. Also, there seemed to be a redundant set of comments in the last code, so removed one of these:
Comment #162
shanly CreditAttribution: shanly commented#161: core-js-ui-autocomplete-675446-161.patch queued for re-testing.
Comment #163
hansyg CreditAttribution: hansyg commentedThe patch in #161 is broken. Found 4 errors with JSHint and the code was throwing errors. This patch addresses the errors but the autocomplete still doesn't seem to be firing correctly. Still working on it but I wanted to let you know that 161 is definitely broken, no need to re-test
Comment #164
tomyouds CreditAttribution: tomyouds commentedPatch to get jquery ui autocomplete hitting the correct source.
Comment #165
deviantintegral CreditAttribution: deviantintegral commentedThe patch in #164 is rather broken; it looks to have ASCII colour codes embedded. @tomyouds, could you re-reoll?
Comment #166
tomyouds CreditAttribution: tomyouds commentedRe-rolled to fix my broken patch.
Comment #167
jmolivas CreditAttribution: jmolivas commentedComment #169
hansyg CreditAttribution: hansyg commented#166: core-js-ui-autocomplete-675446-166.patch queued for re-testing.
Comment #170
mgiffordI've attached some interdiff's but this works fine in my testing with FireFox, Chrome & Safari (on the Mac). It's now working fine.
Comment #171
nod_Main issue with this patch was the value splitting. Was adressed in #128 but no testing has been visibly done.
There is a leftover declaration of
Drupal.ACDB
.At some point we have this:
And that means we're overrding
.success
for all ajax requests. need to find the specific instance ofDrupal.ajax
withDrupal.ajax[inputID]
.There should be a space between function name and ( in the declarations
function autocompleteSplitValues(value) {
should befunction autocompleteSplitValues (value) {
.jshint doesn't validate it (warning about a use of this) also the if / else if / else should be on a newline, not after the
}
Still some work left.
Comment #172
mgifford#166: core-js-ui-autocomplete-675446-166.patch queued for re-testing.
Comment #174
Sutharsan CreditAttribution: Sutharsan commentedPatch rerolled.
Comment #176
mgifford#174: core-js-ui-autocomplete-675446-174.patch queued for re-testing.
Comment #178
mgiffordChunk 1 of 2 was pretty big when rebuilding this patch:
1 out of 2 hunks FAILED -- saving rejects to file core/misc/autocomplete.js.rej
Sadly chunk 1 included one big change to all of these:
function autocompleteSplitValues(value), extractLastTerm(terms), searchHandler(event), sourceData(request, response), showSuggestions(suggestions), function sourceCallbackHandler(data), focusHandler(), selectHandler(event, ui) &.behaviors.autocomplete = { attach: function (context) {
This will need to be reviewed to determine what if anything was lost in patch chaos.
Comment #179
PanchoSending the patch to the testbot.
Comment #181
mgiffordReroll without rtl files.
Comment #182
eigentor CreditAttribution: eigentor commentedJust a question: given this lands, would it also resolve the issue with the ridiculous throbber.gif? (the sprite always showing the lower half even in core http://screencast.com/t/skWg1TC7lPah).
Was just thinking about filing an issue about replacing the gif with two, so it is at least independent of the field height.
Comment #183
eigentor CreditAttribution: eigentor commentedTried to test the patch, does not apply anymore as it seems. Which Drupal 8 version do I need? http://screencast.com/t/Ok3b8LwV5d
Comment #184
mgifford#181: core-js-ui-autocomplete-675446-181.patch queued for re-testing.
Comment #185
mgiffordI'm hoping that we can re-roll this patch for Core. Would be great if it were in D8.
Comment #186
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled
Comment #188
rteijeiro CreditAttribution: rteijeiro commentedIt seems that there is a weird error with user permissions to access autocomplete:
Ensure that the user does have access to the autocompletion
I will try to resend the patch to the testbot. If it fails again I will take a look but I don't know what could be the problem...
Comment #189
rteijeiro CreditAttribution: rteijeiro commented#186: core-js-ui-autocomplete-675446-186.patch queued for re-testing.
Comment #191
mgiffordI don't know why the unit tests are reporting that error.
Comment #192
longwaveThe tests are looking for the hidden autocomplete element, which is removed by this patch. Maybe they need updating to look for the data-autocomplete-path attribute instead?
Comment #193
longwaveI updated the tests, and fixed form_process_autocomplete to use the prepared $path instead of calling url() again.
Comment #195
larowlan#193: 675446-core-js-ui-autocomplete-193.patch queued for re-testing.
Comment #196
jibranWow it is green. Can we have new screen shot?
Comment #197
klonosScreenshots reveal that the busy throbber from the sprite is visible when the autocomplete is not busy:
before the patch:
with the patch from #193:
Comment #198
mcrittenden CreditAttribution: mcrittenden commentedLet's fix a typo while we're at it.
...should be this...
Updated patch attached.
Comment #199
jibranAfter #197 needs manual testing.
Comment #201
droplet CreditAttribution: droplet commentedWhat's the state now? Replace autocomplete with jQuery UI or solve the real problem ?
If the latter, can anyone test the #171, or even earlier about the tagging issue #97 :)
Comment #202
klonos@droplet: from your post above I take it that using jQuery UI does not solve the things mentioned in #97. Right? I'm all for solving the actual problem, but I'd rather we used jQuery and worked things out upstream. You see if we get jQuery autocomplete in D8 core, then we get to use some really neat features it brings along (like its autocomplete + drop-down combo for example) in order to rectify UX issues like:
#1933614: [META] Locale settings in Drupal make little (UX) sense
#1787540: Improve the Timezone Picker
#2083575: Provide better UX when selecting Country/timezone.
Anyways, I believe that it's better practice to use established 3rd party tools NIH/PFE then to craft our own. This way, we delegate some of our maintenance burden plus if we help solve things upstream, we help the open source community. That's the spirit after all. Right?
Comment #203
Jelle_S#198: 675446-core-js-ui-autocomplete-198.patch queued for re-testing.
Comment #205
longwaveRerolled
Comment #207
longwaveA new autocomplete test was added in the Views taxonomy filter, this probably needs a quick manual test as well.
Comment #208
mgiffordI tested this on SimplyTest.me and had great results. I've attached screenshots of this patch.
I also tested it with Safari/VoiceOver. Worked as expected. I think this is ready to be marked RTBC.
Comment #209
nod_some code style issues will rereoll today.
Comment #210
webchickThat sounds like "needs work."
Mike, did you test this for accessibility as well? That was the major deterrent of this in the past.
Comment #211
longwaveIf you can point to what/where the code style issues are I am happy to have a go at fixing them.
Comment #212
mcrittenden CreditAttribution: mcrittenden commentedRe: #210, just wanted to note that the issue that Everett Zufelt created a couple years ago against jQuery UI Autocomplete for screen readers (http://bugs.jqueryui.com/ticket/7840) was marked fixed 17 months ago. Still needs a review obviously, but hopefully things have improved a lot since his initial concerns at the beginning of this issue.
Comment #213
mgifford@webchick - yes, I tested it as a keyboard only user as well as with VoiceOver. Having more testing would be great, but I couldn't find any accessibility problems with the latest release.
@mcrittenden - yes, Everett worked to bring the enhancements we brought into D7 into jQuery UI's Autocomplete. It should be in the version that we're presently using with Drupal 8. Things have definitely improved thanks to Scott Gonzalez & Everett.
Comment #214
nod_with that and the dialog patches we're finally ready to try out the a11y improvements of the jQuery UI team made for us back in january :p
Sorry for the delay, here is the reroll. Drupal.ajax was used in a really bad way. kind of Drupal.ajax fault anyway :p
Comment #215
nod_Comment #216
jibranIt is RTBC for me. Here is some screenshots.
Tags Suggestion
Tag Selection
Tags Searching
Tags Dropdown
Tags Dropdown Select
What is this? Is it temporary?
Comment #217
nod_it's not temporary, it's to avoid what jshint shows as a possible strict mode violation about the "this". It is not and because we're using jquery ui there is no way to do otherwise. Basically, in this case, we know better than jshint.
Comment #218
amateescu CreditAttribution: amateescu commentedI also tested this manually and, unfortunately, it breaks Entity reference in multiple ways :(
I suspect this code is the culprit for the following problems:
- while it's true that Drupal returns an object now, I think we need to change Drupal to return data in a meaningful way for the new autocomplete handling, not mangle with the results client-side
- caching needs to be *per widget* on the form, not globally, because if you have multiple fields with different settings/access control/etc that use the autocomplete widget and you cache the results for the first field, when you type the same thing in another field you *should not* get the cached results from the first field
Comment #219
amateescu CreditAttribution: amateescu commentedThis interdiff fixes the caching problem, working on the rest now.
Comment #220
amateescu CreditAttribution: amateescu commentedAnd this fixes return data handling. Interdiff is to #214.
Comment #221
nod_Interdiff looks good to me. Thanks :)
Comment #222
amateescu CreditAttribution: amateescu commentedAnd the rest of the patch looks good to me. Accessibility testing was done in #213.
Comment #224
amateescu CreditAttribution: amateescu commentedOf course, test expectations need to be updated as well.
Comment #225
jibranBack to RTBC.
Comment #226
Xano#224: core-js-ui-autocomplete-675446-224.patch queued for re-testing.
Comment #227
alexpottCan't see any usage of drupalSettings is this necessary?
My validator says this in not an array but an object. Since it is called from sourceCallbackHandler and parameter passed in is an object.
Is this comment really documenting what comes next?
This is not used
Missing opening {
Comment #228
rteijeiro CreditAttribution: rteijeiro commentedApplied changes in #227.
Comment #229
longwaveAll of #227 has been addressed.
Comment #230
droplet CreditAttribution: droplet commentedit always search the last char. see:
Comment #231
Alan D. CreditAttribution: Alan D. commentedUnrelated feature request or bug depending on your point of view.
Same behavior can be mimicked in the tags autocomplete which goes back to Drupal 5 if not earlier.
Comment #231.0
Alan D. CreditAttribution: Alan D. commentedUpdated issue summary.
Comment #232
klonos@droplet, #230: I consider that a bug too - not a feature. It should search in the middle. Did you file an issue for it?
Comment #233
Xano228: core-js-ui-autocomplete-675446-228.patch queued for re-testing.
Comment #234
Xano228: core-js-ui-autocomplete-675446-228.patch queued for re-testing.
Comment #235
webchickNo longer applies, sorry. :(
Comment #236
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #237
webchickOk, cool, sat down to play with this today. It's very nice, and actually feels way faster than the old autocomplete, too!
I'm a little surprised the diffstat is not more stark, but it looks like we still need some pretty extensive Drupal-specific code for how to use the autocomplete widget. But the good news is that code got massively cleaned up, so that's great.
Committed and pushed to 8.x. Thanks!
We'll need a change notice about this.
Comment #238
Wim LeersI found several problems:
Steps to reproduce:
This problem was in fact implicitly being demonstrated in #230.
jquery.ui.theme.css
. Hence in the backend, the autocomplete continues to look fine. However, in the front-end (Bartik), it looks like utter crap (i.e. jQuery UI's ownjquery.ui.theme.css
is used). This means that when you use in-place editing or an exposed Views filter that filters on tags… it's going to look like crap:Comment #239
amateescu CreditAttribution: amateescu commentedI really fail to see the "utter crap" in Bartik.
Comment #240
Wim LeersIt used to be the case that the autocomplete looked the same in the front- and back-end, and now that's not anymore the case. The font size is ridiculously large with jQuery UI's own
jquery.ui.theme.css
.Comment #241
Wim LeersThe problem described in #238.1 is a critical regression. Updating priority.
Comment #242
catch@Wim please open a new issue for the regressions, unless you think we should roll this back an continue here?
Comment #243
droplet CreditAttribution: droplet commented#238.1,
Yeah, this is a regression in Drupal. But I bet this also a intent behavior designed by jQuery team.
Sometimes, we got a list of tags, and adding it one by one. You may not remember what you added into the box. (Sounds silly? but some editors really doing such data entries work)
Comment #244
RainbowArrayI'm going through this issue to write up a change notice. 3 years and 250 comments, so it might take me a bit. If anybody has any highlights that for sure go in, holler.
My understanding is that there can still be a change notice despite the regression concerns, but if that's wrong, please let me know.
Comment #245
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis introduced two real regressions, that might be severe enough to warrant a revert:
Bonnie & Clyde
will be rendered in the autocomplete asBonnie & Clyde
).Temporarily bumping to critical until we have a plan.
Comment #246
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #247
catchI'd be happy reverting here then trying to resolve those issues either way before we commit again. Just checked quickly and the original commit no longer cleanly reverts.
Comment #248
droplet CreditAttribution: droplet commentedQuick response to #245.
I think we also need to export $autocomplete to global, hence we can modify other private functions in the jQuery UI autocomplete.
Comment #249
Damien Tournoud CreditAttribution: Damien Tournoud commented#248 sounds very reasonable.
Comment #250
Alan D. CreditAttribution: Alan D. commentedhttp://bugs.jqueryui.com/ticket/5918 - this is marked as by design
A custom _renderItem() is probably required, something like this may resolve one of those issues (sorry no setup to test / patch atm):
Effectively changes .text() to .html()
[Edit]
See patch above which does this. Takes way too long to skim read these long threads ;)
Comment #251
scott.gonzalez CreditAttribution: scott.gonzalez commentedChanging `_renderItem()` alone isn't enough for proper HTML support. Please ping me in IRC (scott_gonzalez) to work through the best solution.
Comment #252
RainbowArrayHere is a draft of the change notice. I've focused primarily on detailing the benefits of this change. I'm less clear on how to document how people used the custom autocomplete vs how people should now use jQuery UI Autocomplete. Suggestions welcome!
Summary
Previously, in order to provide autocomplete on fields, Drupal core relied upon custom JS, autocomplete.js. We are now handling this feature with code that is proudly found elsewhere: jQuery UI Autocomplete.
Our custom autocomplete.js lacked critical features, such as an autocomplete select event, and had strange bugs, like a namespace error in Mozilla browsers. In addition, switching to an open source solution that already solves this problem allows us to reduce the amount of custom code we need to maintain in core.
jQuery UI autocomplete also provides a number of other features, such as support formatItem, formatMatch and formatResult. We can also use the drop down combo box available in jQuery UI Autocomplete to solve a number of UX issues.
We conducted an accessibility review of jQuery UI Autocomplete as part of this patch and were able to contribute some changes upstream to ensure proper accessibility.
This implementation also provides caching on a per field basis to improve performance, as well as handling of multiple terms separated by commas, terms with spaces in them, terms with punctuation in them (delimited by quotation marks), and proper formatting of punctuation marks in the autocomplete box, rather than outputting HTML entity codes.
By using jQuery UI Autocomplete, we are also able to ensure that the style of the autocomplete box matches the rest of the theme when a custom jQuery UI theme is provide.
Finally, jQuery UI Autocomplete is regularly maintained and has its own documentation available: http://jqueryui.com/autocomplete/.
Comment #253
scott.gonzalez CreditAttribution: scott.gonzalez commentedThe `format*` options don't exist. Perhaps you're thinking of the old autocomplete plugin that wasn't part of jQuery UI and is no longer supported?
Comment #254
RainbowArrayComment #255
RainbowArrayComment #256
droplet CreditAttribution: droplet commented@scott.gonzalez,
any updates from #251? Thanks
Comment #257
nod_yeah he gave me some code to start, need to make the patch.
Comment #258
nod_Comment #259
RainbowArray#251: We can take that out. It was something mentioned in #50. I'm going on what has been mentioned in comments on this issue.
Comment #260
nod_Running late so if someone wants to get to it before me here is the code scott gave me: https://github.com/scottgonzalez/jquery-ui-extensions/blob/master/src/au...
It needs to be updated to the latest jquery ui api.
Comment #261
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release (We probably will want to wait on the revert or the updated version of the patch.)
Regarding the draft in #252, it would be better to have a shorter, more scannable change record, with example code, rather than a long post. For a more thorough review, I direct you to AnyoneWhoKnowsADarnThingAboutJavaScriptAKANotMe. ;)
Comment #262
droplet CreditAttribution: droplet commentedI read through the old Autocomplete again. I don't think D7 and below is supposed to support (advanced) HTML label.
(advanced) HTML label example:
<strong>LABEL NAME</strong>
and Autocomplete will search only the text content:
(This is what the plugin in #260 do)
I don't count following example as HTML label:
LABEL & NAME
Drupal encoded the HTML strings to prevent XXS. This is the only reason that make the old Autocomplete script to use jQuery.html(). I'd said it's a beautiful misunderstanding that make you think Autocomplete is supporting HTML label :)
$matches[] = array('value' => $prefix . $name, 'label' => String::checkPlain($term->label()));
For jQuery UI Autocomplete, we added 'value' property. and Autocomplete search the terms via the "value", NOT the "label" property. Patch #248 provides same feature as old script already.
jquery.ui.autocomplete.html.js supposed you may not provide "VALUE", therefore add a custom filter function.
Comment #263
Alan D. CreditAttribution: Alan D. commentedI was a bit confused with everything going on in this thread, I think this is a summary of the issues here or referenced from here:
The change notice ;)
#675446-230: Use jQuery UI Autocomplete
- Always searches last tag. Related issue? An issue since the start of time of my Drupal experience.
#675446-238: Use jQuery UI Autocomplete
- No context of existing terms (regression)
- JSHint error (regression?)
- Theme may need to implement their own theme for consistent UI. Should core implement a simple themed version that is used by default?
#675446-245: Use jQuery UI Autocomplete
- Doesn't support HTML content for the label anymore (patch for review in this queue)
- Autocomplete callbacks return HTML, but the new autocomplete expects plain-text (works as designed as per #262)
UI related issue where this could be used to help
#1933614: [META] Locale settings in Drupal make little (UX) sense
#1787540: Improve the Timezone Picker
#2083575: Provide better UX when selecting Country/timezone.
Comment #264
droplet CreditAttribution: droplet commentedUPDATES OF #263:
#675446-230: Regressions: Use jQuery UI Autocomplete- Always searches last tag. Related issue? An issue since the start of time of my Drupal experience.
#2186643: Autocomplete always searches the last tag
#675446-238: Regressions: Use jQuery UI Autocomplete
- duplicates of existing terms are suggested (regression)#2186647: Autocomplete: duplicates of existing terms are suggested (regression)
- JSHint error (regression?)- Theme may need to implement their own theme for consistent UI. Should core implement a simple themed version that is used by default?
#2186649: Autocomplete: Clean up jQuery UI CSS & Styles
#675446-245: Regressions: Use jQuery UI Autocomplete
- Doesn't support HTML content for the label anymore (patch for review in this queue)
- Autocomplete callbacks return HTML, but the new autocomplete expects plain-text (works as designed as per #262)
UI related issue where this could be used to help
#1933614: [META] Locale settings in Drupal make little (UX) sense
#1787540: Improve the Timezone Picker
#2083575: Use jQuery UI autocomplete combo to provide better UX when selecting Country/timezone.
Comment #265
droplet CreditAttribution: droplet commentedReroll patch in #248
Comment #266
droplet CreditAttribution: droplet commentedComment #267
Damien Tournoud CreditAttribution: Damien Tournoud commentedJust to clarify, @droplet in #262 is just confirming what I have been saying in #245: the autocomplete doesn't support HTML content for the label anymore, and in addition we are currently returning HTML where it expects plain-text.
The patch in #265 is all that we need. We don't need a custom filter because we only support AJAX autocomplete anyway.
Comment #268
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe screenshots in #262 alone are very convincing. Code is what I expected to see.
Comment #269
catchThis is where we really miss js testing. Patch looks fine though.
Committed/pushed to 8.x, thanks!
Comment #270
nod_#2187779: Add a 'html' option to the autocomplete widget issue summary needs work but created to track it down.
Comment #271
Wim LeersIn hindsight, the sheer number of regressions, follow-up issues and confusion suggests this should've been reverted.
All of the follow-up issues are normal. The first two are also huge usability regressions.
These regressions have been known for over two months, yet they haven't been fixed. Shouldn't they all be release-blocking criticals? It looks like if we don't do that, nobody is going to solve them.
Marking active again to get committer feedback on how to best address those follow-up issues.
Comment #272
catchI'm fine rolling this back completely, as I said in #247: https://drupal.org/comment/8394603#comment-8394603
However there's no clean path to revert, so we'd need a patch.
Comment #273
droplet CreditAttribution: droplet commentedThese aren't regressions:
#2186643: Autocomplete always searches the last tag
#2187779: Handle HTML in Autocomplete options
These two needs some more work.
#2186647: Autocomplete: duplicates of existing terms are suggested (regression)
#2186649: Autocomplete: Clean up jQuery UI CSS & Styles
Revert patch, we may need another turn of fixes for the old script.
Comment #274
Wim Leers#2186643 is a regression AFAICT.
Comment #275
amateescu CreditAttribution: amateescu commented@Wim Leers, see #231 / #263. I guess you could call it a regression if you really want to (and it seems you do).. depends on when you start counting.
Comment #276
Wim Leersnod_ pointed out the same on chat immediately after I posted #274. So apparently this has been a long time bug in Drupal 5/6/7. But it was actually not a bug in Drupal 8 before this patch went in. I specifically tested it and I could not reproduce it.
Comment #277
nod_This is not worth an open critical. The two real regressions have patches and they're pretty close.
Comment #278
xjmI think the change record is still outstanding? At least, there's nothing for it in the sidebar.
Comment #279
cosmicdreams CreditAttribution: cosmicdreams commentedChange notice started: [#2202701]
https://drupal.org/node/2202701
Comment #280
jessebeach CreditAttribution: jessebeach commentedThanks cosmicdreams! I've updated the D8 description a little more. I think this is ready to go!
Comment #281
jessebeach CreditAttribution: jessebeach commentedResetting to #278.