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:
before

After:
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

CommentFileSizeAuthor
#265 after.jpg12.22 KBdroplet
#265 before.jpg13.64 KBdroplet
#265 675446-autocomplete-follow-up.patch1.3 KBdroplet
#248 jquery_ui_autocompelte.patch1.32 KBdroplet
#238 autocomplete-bartik.png6.38 KBWim Leers
#238 autocomplete-seven.png2.92 KBWim Leers
#236 core-js-ui-autocomplete-675446-236.patch34.26 KBamateescu
#230 auto-error.gif21.88 KBdroplet
#228 core-js-ui-autocomplete-675446-228.patch34.85 KBrteijeiro
#228 interdiff.txt2.08 KBrteijeiro
#224 core-js-ui-autocomplete-675446-224.patch35.04 KBamateescu
#224 interdiff.txt9.8 KBamateescu
#220 core-js-ui-autocomplete-675446-220.patch25.24 KBamateescu
#220 interdiff.txt4.49 KBamateescu
#219 interdiff.txt1.58 KBamateescu
#216 tags-suggestion.png1.49 KBjibran
#216 tag-selection.png1.51 KBjibran
#216 tags-searching.png2.59 KBjibran
#216 tags-dropdown.png1.87 KBjibran
#216 tags-dropdown-select.png2 KBjibran
#214 core-js-ui-autocomplete-675446-214.patch22.35 KBnod_
#214 interdiff.txt5.31 KBnod_
#208 Screen Shot 2013-09-28 at 10.13.54 PM.png30.54 KBmgifford
#208 Screen Shot 2013-09-28 at 10.14.52 PM.png21.19 KBmgifford
#207 675446-core-js-ui-autocomplete-207.patch22.4 KBlongwave
#207 interdiff.txt1003 byteslongwave
#205 675446-core-js-ui-autocomplete-205.patch21.42 KBlongwave
#198 675446-core-js-ui-autocomplete-198.patch21.43 KBmcrittenden
#197 jQuery_UI_autocomplete-no_patch.png11.19 KBklonos
#197 jQuery_UI_autocomplete-patch_applied.png10.29 KBklonos
#193 675446-core-js-ui-autocomplete-193.patch21.32 KBlongwave
#193 interdiff.txt3.99 KBlongwave
#186 core-js-ui-autocomplete-675446-186.patch18.03 KBrteijeiro
#181 core-js-ui-autocomplete-675446-181.patch18.2 KBmgifford
#178 core-js-ui-autocomplete-675446-178.patch18.78 KBmgifford
#174 core-js-ui-autocomplete-675446-174.patch18.71 KBSutharsan
#170 interdiff-161-166.txt2.51 KBmgifford
#170 interdiff-163-166.txt522 bytesmgifford
#170 Autocomplete.png42.83 KBmgifford
#166 core-js-ui-autocomplete-675446-166.patch18.65 KBtomyouds
#164 core-js-ui-autocomplete-675446-164.patch26.11 KBtomyouds
#163 core-js-ui-autocomplete-675446-163.patch18.65 KBhansyg
#161 core-js-ui-autocomplete-675446-161.patch18.66 KBmgifford
#158 core-js-ui-autocomplete-675446-158.patch18.74 KBmgifford
#155 interdiff-133-154.txt2.36 KBmgifford
#155 interdiff-1.aSxvlC.rej_.txt2.4 KBmgifford
#154 core-js-ui-autocomplete-675446-154.patch18.71 KBmgifford
#151 core-js-ui-autocomplete-675446-151.patch18.73 KBmgifford
#142 IE9 (clean) [Running] 2012-11-18 14-58-28.jpg36.66 KBdeviantintegral
#133 core-js-ui-autocomplete-675446.patch18.79 KBJelle_S
#130 675446-130.patch19.09 KBAngry Dan
#128 675446-128.patch19.06 KBJelle_S
#124 675446-116-123.diff.txt3.74 KBclemens.tolboom
#123 taxonomy-ux.png17.59 KBclemens.tolboom
#123 675446-123.txt19.85 KBclemens.tolboom
#123 bad-terms-transfer.png9.93 KBclemens.tolboom
#116 675446-116.patch18.2 KBoxyc
#116 interdiff.txt3.4 KBoxyc
#113 before_patch1.png10.73 KBJelle_S
#113 before_patch2.png10.56 KBJelle_S
#113 after_patch1.png11.26 KBJelle_S
#113 after_patch2.png11.59 KBJelle_S
#112 675446-112.patch17.72 KBoxyc
#109 interdiff.txt2.22 KBoxyc
#109 675446-109.patch16.32 KBoxyc
#109 Screenshot from 2012-09-01 16:57:55.png5.58 KBoxyc
#98 675446.patch18 KBRobLoach
#96 675446.patch17.54 KBRobLoach
#90 core-js-ui-autocomplete-675446-90.patch17.39 KBnod_
#89 675446.patch17.65 KBRobLoach
#83 core-js-ui-autocomplete-675446-78.patch17.37 KBRobLoach
#78 core-js-ui-autocomplete-675446-78.patch17.37 KBdroplet
#78 interdiff.patch1.88 KBdroplet
#77 core-js-ui-autocomplete-675446-77.patch17.2 KBnod_
#74 675446.patch17.04 KBRobLoach
#72 core-js-ui-autocomplete-675446-72.patch17.71 KBnod_
#71 autocomplete.patch16.47 KBRobLoach
#59 675446.patch15.12 KBRobLoach
#45 ja.png11.1 KBRobLoach
#37 Screenshot.png9.76 KBRobLoach
#19 jqueryui.patch14.96 KBRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan D.’s picture

+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

webchick’s picture

"former"? Why was it removed?

Dave Reid’s picture

Title: Replace autocomplete.js with the former jQuery UI Autocomplete plugin » Replace autocomplete.js with the jQuery UI Autocomplete plugin

Huh, the last I read it had been removed from jQuery UI, but they're revamping its development for jQuery UI 1.8.

RobLoach’s picture

Status: Active » Postponed

Using jQuery UI's Autocomplete widget instead of Drupal's internal one will save us a lot of custom code.

Postponed by:

Dave Reid’s picture

The autocomplete widget is now officially in jQuery UI 1.8 Final.

corey.aufang’s picture

Unless 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.

RobLoach’s picture

Dave Reid’s picture

Oh dear god please!

RobLoach’s picture

Everett Zufelt’s picture

Issue tags: +Accessibility

I 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.

Everett Zufelt’s picture

Everett Zufelt’s picture

I 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 :)

Everett Zufelt’s picture

Excerpt of response from Scott González:

The demo you were looking at is running the most recent stable release, so that's a good place to be looking. We also have http://view.jqueryui.com/master/demos/autocomplete/multiple.html which is running the same demo straight from the master branch of our git repo (currently down, GitHub just changed their API which broke our site; we're working on bringing it back up).

One thing to note is that the multiple word support is just a demo built on top of our single word autocomplete widget. What that means is there's no official support for it: no tests, no documentation, may not have a full API, etc. However, we do try to provide as much support for them as we reasonably can, so if you'd like to help improve the multiple word demos, we'd certainly appreciate any help.

...

We do have plans to release a multi word autocomplete (http://wiki.jqueryui.com/ListBuilder) which will be built on top of autocomplete (http://wiki.jqueryui.com/Autocomplete) which utilizes menu (http://wiki.jqueryui.com/Menu). Right now the menu plugin hasn't been officially released, but if you look at the bottom of jquery.ui.autocomplete.js, you'll see that there's an initial implementation of menu in there. That menu plugin has since had a lot of improvements in our menu branch (https://github.com/jquery/jquery-ui/tree/menu) and menu is slated for our next major version (1.9), though it has not yet been pulled into the master branch.

Everett Zufelt’s picture

Status: Postponed » Active

Can 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.

marcoBauli’s picture

the 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.

Everett Zufelt’s picture

@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

Cyberwolf’s picture

subscribing

sepgil’s picture

subscribing
And just in case someone needs the jquery autocomplete behavior for drupal 7: I've coded a module based on it: Autocomplete deluxe.

RobLoach’s picture

Status: Active » Postponed
FileSize
14.96 KB

This 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).

RobLoach’s picture

Fancy issue tag.

wmostrey’s picture

Dave Reid’s picture

not sure why we needed a term change, but ok but thanks for pinging everyone

mgifford’s picture

Dave, 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.

Dave Reid’s picture

Yeah Wim forgot to copy/paste the context of the term change in the comment. :)

Alan D.’s picture

sub.

Everett Zufelt’s picture

I 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.

klonos’s picture

...subscribing.

mgifford’s picture

Everett, how did the hackathon go? Any good results from the discussion?

Everett Zufelt’s picture

I was unfortunately unable to attend, work got in the way.

Everett Zufelt’s picture

I 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.

mgifford’s picture

Work 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

Everett Zufelt’s picture

@mgifford I'll ask Colin Clark

RobLoach’s picture

@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.

Everett Zufelt’s picture

@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.

gagarine’s picture

Status: Postponed » Active

It was postponed in #4 but #5326: jQuery UI Autocomplete doesn't support multiple items is an invalid bug (not the right documentation). In fact autocomplete support multiple items http://jqueryui.com/demos/autocomplete/#multiple

About 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.

RobLoach’s picture

Status: Active » Postponed
Issue tags: +Needs issue summary update

The 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.

RobLoach’s picture

FileSize
9.76 KB

RE: 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.

Everett Zufelt’s picture

@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?

RobLoach’s picture

Everett Zufelt: 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?

Here 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 :-) .

Everett Zufelt’s picture

I 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.

webchick’s picture

Thanks 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)

Everett Zufelt’s picture

@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.

RobLoach’s picture

Everett Zufelt: In the programming language field I typed: j - nothing happened.

Sorry 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".

Everett Zufelt’s picture

@Rob

I also typed a, v (that is to say 'jav' in all. :)

RobLoach’s picture

FileSize
11.1 KB

Weird, this is what I get...
ja.png

wroxbox’s picture

klonos’s picture

...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.

scott.gonzalez’s picture

Can 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.

Everett Zufelt’s picture

@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.

nagiek’s picture

Can't wait for this! Would love to see formatItem, formatMatch, and formatResult support!

http://docs.jquery.com/Plugins/Autocomplete/autocomplete#url_or_dataoptions

nagiek’s picture

Everett Zufelt’s picture

@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.

mgifford’s picture

This is totally a good call. I've posted a note about this and hope to get some more feedback & encourage participation.

Everett Zufelt’s picture

See 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.

RobLoach’s picture

http://zufelt.ca/blog/jquery-ui-accessibility-review-whats-plan#comment-334

Thanks 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:

Custom data and display
1. Not sure what 'custom' data / display is. Only receiving words like 'jQuery' as results read by JAWS. Note: must tab away and back to field to confirm result, see (3) above.

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.

Categories
1. When I type 'j' the only result seems to be for 'andreas johnson'. This does not seem very 'category' to me (didn't read anything about how this ought to work). When I type 'a' I receive more than one option, but still no sense of what a 'category' is.

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.

Multiple
Nothing other than to say that it works as designed, for getting more than one value in the field, even though I am guessing about what items I am picking from the list.

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.

Everett Zufelt’s picture

Rob

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.

RobLoach’s picture

Awesomesauce!

Everett Zufelt’s picture

RobLoach’s picture

Issue tags: +Less code
FileSize
15.12 KB

Keeping up to date now that jQuery 1.7 is in there... Would be nice to have:

Everett Zufelt’s picture

As far as I know, jQuery UI autocomplete will not be accessible enough for Core until at least 1.9.

Everett Zufelt’s picture

Status: Postponed » Needs review

http://bugs.jqueryui.com/ticket/7840#comment:9

Could use review and feedback.

Status: Needs review » Needs work

The last submitted patch, 675446.patch, failed testing.

quicksketch’s picture

As far as I know, jQuery UI autocomplete will not be accessible enough for Core until at least 1.9.

Everett, 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.

Everett Zufelt’s picture

@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.

sun’s picture

+++ b/core/misc/autocomplete.js
@@ -5,318 +5,65 @@
+        search: function() {
+          // Only search if there has been more then two characters in there.
+          var term = Drupal.autocompleteextractLast(this.value);
+          if (term.length < 1) {

1) 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.

+++ b/core/misc/autocomplete.js
@@ -5,318 +5,65 @@
+        focus: function() {
+          // Prevent value inserted on focus.
+          return false;
+        },
+        // It will select the search term based on the last item in the list.
+        select: function(event, ui) {
+          var terms = Drupal.autocompletesplit(this.value);
+          // Remove the current input.
+          terms.pop();
+          // Add the selected item.
+          terms.push(ui.item.value);
+          this.value = terms.join(", ");
+          return false;
         }

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...?

+++ b/core/misc/autocomplete.js
@@ -5,318 +5,65 @@
+    // Pop up the autocomplete if tab was entered.
+    $(this).bind('keydown', function(event) {
+      if (event.keyCode === $.ui.keyCode.TAB && $(this).data("autocomplete").menu.active) {
+        event.preventDefault();
       }
     });

Not sure whether this is needed.

+++ b/core/misc/autocomplete.js
@@ -5,318 +5,65 @@
+Drupal.autocompletesplit = function(val) {
+  return val.split(/,\s*/);
+};
+Drupal.autocompleteextractLast = function(term) {
+  return Drupal.autocompletesplit(term).pop();
 };

This looks like a regression to me - Drupal's autocomplete syntax supports commas within terms; e.g.:

"Boston, USA", "Karlsruhe, Germany"

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.

sun’s picture

Issue summary: View changes

Updated issue summary.

mgifford’s picture

Issue tags: +jQuery

Tagging. 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.

Everett Zufelt’s picture

I 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

RobLoach’s picture

Status: Needs work » Postponed

Let's post-pone this for #1085590: Update to jQuery UI 1.9 then :-) .

g10’s picture

don'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

webchick’s picture

According 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.

RobLoach’s picture

Status: Postponed » Needs work
FileSize
16.47 KB
nod_’s picture

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.

RobLoach’s picture

RobLoach’s picture

Status: Postponed » Needs review
FileSize
17.04 KB
RobLoach’s picture

Tagging.

sun’s picture

Status: Needs review » Needs work
Issue tags: -JavaScript, -Proudly Found Elsewhere
+++ b/core/misc/autocomplete.js
@@ -1,334 +1,140 @@
+ * Splits multi-values input text.
...
+function split(value) {

1) 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.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Proudly Found Elsewhere
FileSize
17.2 KB

Yes, it was me.

1) I'm not good at comments, feel free to make it/them better. I changed the confusing split name to autocompleteSplitValues 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 by return autocomplete or alike once we have JS modules also, using autocomplete throughout the file minify better than using Drupal.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.

droplet’s picture

few suggestions.

#1.

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,141 @@
+  extractLast: extractLast,
...
+    source: source,

can it use different vars name ? sourceHanlder, extractLastHandler..etc

#2.
can it add cache backend to "source". reduce same ajax request.

#3.

-  if (term.length < 1) {
+  if (term.length <= 1) {
     return false;
   }

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.)

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

nod_’s picture

Yeah the naming could use some work.

  1. source is not an event that's why the function didn't get a Handler suffix, same thing for extractLast. It'll be confusing if we name event handlers and helpers the same way.
  2. Why not, seems reasonable to me. How will you invalidate it in case a term was added from somewhere else? It can be useful to have this not cached to limit the spelling/capitalization variants.
  3. thanks :)
  4. i think that should be in here, let's just agree on the JS and deal with theming afterwards

For the requests we could use a throttling function for this kind of things.

droplet’s picture

2.

How will you invalidate it in case a term was added from somewhere else?

they can reset the cache object.

It can be useful to have this not cached to limit the spelling/capitalization variants.

array is case sensitive in javascript.

nod_’s picture

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.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.37 KB

Theming 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.

droplet’s picture

@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 :)

nod_’s picture

Status: Needs review » Needs work

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.

Alan D.’s picture

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".

If 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!

nod_’s picture

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.

droplet’s picture

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,149 @@
+  $.getJSON(url, sourceCallback);

also, can it use $.ajax instead of shorthand.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.65 KB

This 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.

nod_’s picture

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.

nod_’s picture

Status: Needs review » Needs work
droplet’s picture

When 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.

  // See if this key has been searched for before.
  if (this.cache[searchString]) {
    return this.owner.found(this.cache[searchString]);
  }
RobLoach’s picture

Anything else missing here? :-) We're getting pretty close.

nod_’s picture

I think we just need to make it use $.ajax and exposing it's options in Drupal.autocomplete.

nod_’s picture

Title: Replace autocomplete.js with the jQuery UI Autocomplete plugin » Use jQuery UI Autocomplete
Issue tags: +js-novice

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 :)

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.54 KB

Along these lines?

droplet’s picture

Status: Needs review » Needs work

- #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

RobLoach’s picture

Status: Needs work » Needs review
FileSize
18 KB

This patch...

  • Uses .match() to help with "Karlsruhe, Germany"
  • Uses $.ajax and exposes its options in Drupal.autocomplete.ajax.. Might be able to optimize it a bit.
  • Fixes "on tagging field -> select 2nd tag will replace 1st tag"

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.

droplet’s picture

Yeah. all rel links are out of scope, just for more info.

Uses .match() to help with "Karlsruhe, Germany"

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.

droplet’s picture

This is the best I ever seen: regex

clemens.tolboom’s picture

Status: Needs review » Needs work

Both 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?

Jelle_S’s picture

Regarding #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...

Jelle_S’s picture

Looking 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)

clemens.tolboom’s picture

+++ b/core/misc/autocomplete.js
@@ -1,334 +1,176 @@
+  // The toSource function, when available, will return an array of split terms.
+  if (typeof arr.toSource == "function") {
+    return arr.toSource();
   }
-  return $autocomplete.length === 0;
-};

Why 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();

+++ b/core/misc/autocomplete.js
@@ -1,334 +1,176 @@
+  var term = autocomplete.extractLastTerm(event.target.value);
+  return term.length >= autocomplete.minLength;

toSource works here ok. Although the magic works is weird.

klonos’s picture

...are we backporting this to D7?

oxyc’s picture

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,176 @@
+  // The toSource function, when available, will return an array of split terms.
+  if (typeof arr.toSource == "function") {
+    return arr.toSource();
   }

toSource 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

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,176 @@
+    autocomplete.cache[term] = data;
 
+    // Drupal returns an object, we need an array.
+    var terms = [];
+    for (var i in data) {
+      if (data.hasOwnProperty(i)) {
+        terms.push(data[i]);
       }
     }
+    // Send the new string array of terms to the jQuery UI list.
+    response(terms);

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.

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,176 @@
+    if ($autocomplete.length) {
+      $autocomplete.each(function () {
+        // Use jQuery UI Autocomplete on the textfield.
+        $(this).autocomplete(autocomplete.options);
+      });

Hmm this could be simplified into

$autocomplete.autocomplete(autocomplete.options);

+++ b/core/misc/autocomplete.jsundefined
@@ -1,334 +1,176 @@
+      if ($autocomplete.length) {
+        $autocomplete.each(function () {
+          $(this).autocomplete('destroy');
+        });
+      }

As well as this into

$autocomplete.autocomplete('destroy');

Jelle_S’s picture

toSource 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

indeed, that was the point we were trying to make in #101 through #104

clemens.tolboom’s picture

@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?

:-)

oxyc’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
16.32 KB
2.22 KB

I 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:
htmlentities
I found "fix" but it looks kinda ugly, http://stackoverflow.com/a/3490609/319855

oxyc’s picture

Status: Needs work » Needs review
+++ b/core/modules/system/system.moduleundefined
@@ -1336,6 +1336,7 @@ function system_library_info() {
       array('system', 'drupal.ajax'),

I dont think this is a dependency now right? Scratch that

Jelle_S’s picture

Status: Needs review » Needs work

can 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...

oxyc’s picture

FileSize
17.72 KB

Oops I forgot to add the form.inc file to the commit diff.

@Jelle_S hmm true matcher definitely needs to be more robust.

Jelle_S’s picture

Status: Needs review » Needs work
FileSize
11.59 KB
11.26 KB
10.56 KB
10.73 KB

True, 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...)

+++ b/core/misc/autocomplete.jsundefined
@@ -1,336 +1,164 @@
+  minLength: 2,

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:
before_patch1.png
before_patch2.png

After patch:
after_patch1.png
after_patch2.png

clemens.tolboom’s picture

This patch should not change the behavior of D7 imho.

It currently does by waiting for 2 characters already :/

Jelle_S’s picture

Unfortunatelly 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 :-)

oxyc’s picture

Status: Needs review » Needs work
FileSize
3.4 KB
18.2 KB

I 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

oxyc’s picture

Status: Needs work » Needs review
sun’s picture

Once again, a simple split by comma or whitespace is not sufficient; see #65.

Jelle_S’s picture

@sun true, but we need to find a reliable regex, because the current one doesn't seem to cut it either.

clemens.tolboom’s picture

@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.

sun’s picture

Well, 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).

clemens.tolboom’s picture

@sun thanks for some more context :)

Taken the examples from drupal_explode_tags

this, "somecompany, llc", "and ""this"" w,o.rks", foo bar

http://drupal.d8/index.php/taxonomy/autocomplete/field_tags/this%2C%20%22somecompany%2C%20llc%22%2C%20%22and%20%22%22this%22%22%20w%2Co.rks%22%2C%20foo%20bar

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.

clemens.tolboom’s picture

FileSize
9.93 KB
19.85 KB
17.59 KB

Attached 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.

Experiment

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

    $element['#attributes']['data-autocomplete-values'] = $element['#value'];

then picked up through javascript

      var $values = $autocomplete.attr('data-autocomplete-values');
      $autocomplete.data('terms', [$values]);

      $autocomplete.parents('form').first().submit(function() {
        restoreData($autocomplete);
      });

The binding above tries to saves all autocomplete fields with the same items. I did not manage to solve this.
Bad binding
How can we distinguish between a single value and multivalue autocomplete?

clemens.tolboom’s picture

FileSize
3.74 KB

Here'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?).

nod_’s picture

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.

clemens.tolboom’s picture

clemens.tolboom’s picture

We need to translate the regex used by drupal_explode_tags into javascript.

$regexp = '%(?:^|,\ *)("(?>[^"]*)(?>""[^"]* )*"|(?: [^",]*))%x';

has a multiline variant (as we use the /x modifier) with added documentation like

$regexp = '%
(?:^|,\ *)       # begin of line OR a comma , followed by zero or more spaces
(                # group start
  "              # start with a double quote "
  (?>[^"]*)      # anything but a double quote "
  (?>""[^"]* )   # followed by two double quote "" followed by anything but a double quote "
  *              # repeat the double quote "" 
  "              # ending with a double quote "
|                # alternative 
  (?: [^",]*)    # anything but a double quote " or a comma ,
)                # group end
%x';

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]

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
19.06 KB

attached 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

nod_’s picture

tag

Angry Dan’s picture

FileSize
19.09 KB

I 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)

Status: Needs review » Needs work

The last submitted patch, 675446-130.patch, failed testing.

mgifford’s picture

I'm having trouble applying this code locally.

$ git apply 675446-130a.patch
error: patch failed: core/themes/bartik/css/style.css:1
error: core/themes/bartik/css/style.css: patch does not apply
error: patch failed: core/themes/seven/style.css:1
error: core/themes/seven/style.css: patch does not apply
Jelle_S’s picture

Jelle_S’s picture

Status: Needs work » Needs review

status...

mgifford’s picture

mgifford’s picture

Issue tags: +a11ySprint

Seems like a worthwhile addition.

attiks’s picture

mgifford’s picture

Thanks @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?

mparker17’s picture

When 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.

webchick’s picture

Status: Needs review » Needs work

That sounds like a "needs work."

scott.gonzalez’s picture

When testing the patch in #133 on IE8, it just sits there and spins forever.

This 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.

Using Voiceover on Mac OS/X 10.7.5, there's no indication that matches are being suggested.

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).

deviantintegral’s picture

Did 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.

Minor IE9 visual artifact

mparker17’s picture

@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.

mparker17’s picture

Interesting. I tried testing with VoiceOver again, and it partially worked. Here's what I did:

  1. git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git
  2. Install Drupal with the Standard install profile.
  3. Disable the Overlay module (it has some other screenreader problems not related to this issue).
  4. Go to Administration -> Configuration -> Development -> Performance and clear caches.
  5. Go to Administration -> Structure -> Taxonomy, click the drop-down in the Operations column beside the Tags vocabulary and choose add terms.
  6. Add terms:
    • ASP
    • ActionScript
    • Ada
    • Basic
    • C
    • C++
    • Clojure
  7. Go to Administration -> Content, click Add content then Article.
  8. Use Control, Option ("VO" keys) and the arrow keys to navigate to the Tags field and type the letter "a".
  9. As a sighted user, note that 4 options are suggested. Note that VoiceOver doesn't report their presence.
  10. Use the arrow keys to navigate between the items in the popup. Note that it does read them aloud.
  11. Clear the field.
  12. As a sighted user, click somewhere else and then click back on the tags field.
  13. Enter the letter "a". Note it now says there are 4 items available, and using the arrow keys to navigate works normally.

Perhaps this is a bug in VoiceOver?

droplet’s picture

scott.gonzalez’s picture

@mparker17: That's interesting. Are you able to create a reduced test case outside of Drupal that shows that behavior?

mgifford’s picture

The last submitted patch, core-js-ui-autocomplete-675446.patch, failed testing.

andypost’s picture

patch needs re-roll

PS: is there any changes in discussed select2 implementation, commerce kickcstart now ships with Chosen

mgifford’s picture

Hadn'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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.73 KB

Just a re-roll.

mgifford’s picture

The last submitted patch, core-js-ui-autocomplete-675446-151.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.71 KB

Reroll.

mgifford’s picture

Interdiff between @Jelle_S's patch in #133 & mine in the last post. Includes rejected text.

mgifford’s picture

The last submitted patch, core-js-ui-autocomplete-675446-154.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.74 KB

re-roll. I think core/misc/autocomplete.js will need some work.

mgifford’s picture

The last submitted patch, core-js-ui-autocomplete-675446-158.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.66 KB

reroll. Also, there seemed to be a redundant set of comments in the last code, so removed one of these:

/**
 * Performs a cached and delayed search.
 */
shanly’s picture

hansyg’s picture

The 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

tomyouds’s picture

Patch to get jquery ui autocomplete hitting the correct source.

deviantintegral’s picture

Status: Needs review » Needs work

The patch in #164 is rather broken; it looks to have ASCII colour codes embedded. @tomyouds, could you re-reoll?

tomyouds’s picture

Re-rolled to fix my broken patch.

jmolivas’s picture

Status: Needs work » Needs review

The last submitted patch, core-js-ui-autocomplete-675446-166.patch, failed testing.

hansyg’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
42.83 KB
522 bytes
2.51 KB

I've attached some interdiff's but this works fine in my testing with FireFox, Chrome & Safari (on the Mac). It's now working fine.

Autocomplete working

nod_’s picture

Status: Reviewed & tested by the community » Needs work

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:

var ajax = Drupal.ajax;
var url = this.element.data('autocompletePath');
ajax.success = sourceCallbackHandler;

And that means we're overrding .success for all ajax requests. need to find the specific instance of Drupal.ajax with Drupal.ajax[inputID].

There should be a space between function name and ( in the declarations function autocompleteSplitValues(value) { should be function 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.

mgifford’s picture

The last submitted patch, core-js-ui-autocomplete-675446-166.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
18.71 KB

Patch rerolled.

The last submitted patch, core-js-ui-autocomplete-675446-174.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

The last submitted patch, core-js-ui-autocomplete-675446-174.patch, failed testing.

mgifford’s picture

Chunk 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.

Pancho’s picture

Status: Needs work » Needs review

Sending the patch to the testbot.

Status: Needs review » Needs work

The last submitted patch, core-js-ui-autocomplete-675446-178.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
18.2 KB

Reroll without rtl files.

eigentor’s picture

Just 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.

eigentor’s picture

Tried to test the patch, does not apply anymore as it seems. Which Drupal 8 version do I need? http://screencast.com/t/Ok3b8LwV5d

mgifford’s picture

mgifford’s picture

Issue tags: +TwinCities

I'm hoping that we can re-roll this patch for Core. Would be great if it were in D8.

rteijeiro’s picture

Re-rolled

Status: Needs review » Needs work

The last submitted patch, core-js-ui-autocomplete-675446-186.patch, failed testing.

rteijeiro’s picture

It 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...

rteijeiro’s picture

Status: Needs review » Needs work

The last submitted patch, core-js-ui-autocomplete-675446-186.patch, failed testing.

mgifford’s picture

I don't know why the unit tests are reporting that error.

longwave’s picture

    $result = $this->xpath('//input[@id = "edit-name-autocomplete"]');
    $this->assertEqual((string) $result[0]['value'], url('user/autocomplete'));
    $this->assertEqual(count($result), 1, 'Ensure that the user does have access to the autocompletion');

The 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?

longwave’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
21.32 KB

I updated the tests, and fixed form_process_autocomplete to use the prepared $path instead of calling url() again.

The last submitted patch, 675446-core-js-ui-autocomplete-193.patch, failed testing.

larowlan’s picture

jibran’s picture

Issue tags: +Needs screenshots

Wow it is green. Can we have new screen shot?

klonos’s picture

Screenshots reveal that the busy throbber from the sprite is visible when the autocomplete is not busy:

before the patch:

jQuery_UI_autocomplete-no_patch.png

with the patch from #193:

jQuery_UI_autocomplete-patch_applied.png

mcrittenden’s picture

Let's fix a typo while we're at it.

$this->assertEqual(count($result), 0, 'Ensure that the user did not had access to the autocompletion');

...should be this...

$this->assertEqual(count($result), 0, 'Ensure that the user does not have access to the autocompletion');

Updated patch attached.

jibran’s picture

Issue tags: +Needs manual testing

After #197 needs manual testing.

Status: Needs review » Needs work

The last submitted patch, 675446-core-js-ui-autocomplete-198.patch, failed testing.

droplet’s picture

What'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 :)

klonos’s picture

@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?

Jelle_S’s picture

The last submitted patch, 675446-core-js-ui-autocomplete-198.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
21.42 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 675446-core-js-ui-autocomplete-205.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
1003 bytes
22.4 KB

A new autocomplete test was added in the Views taxonomy filter, this probably needs a quick manual test as well.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
21.19 KB
30.54 KB

I 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.

Autocomplete with jQuery UI #1

Autocomplete with jQuery UI #1

nod_’s picture

some code style issues will rereoll today.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

That sounds like "needs work."

Mike, did you test this for accessibility as well? That was the major deterrent of this in the past.

longwave’s picture

If you can point to what/where the code style issues are I am happy to have a go at fixing them.

mcrittenden’s picture

Re: #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.

mgifford’s picture

@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.

nod_’s picture

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

nod_’s picture

Status: Needs work » Needs review
jibran’s picture

It is RTBC for me. Here is some screenshots.

Tags Suggestion
tags-suggestion.png
Tag Selection
tag-selection.png
Tags Searching
tags-searching.png
Tags Dropdown
tags-dropdown.png
Tags Dropdown Select
tags-dropdown-select.png

+++ b/core/misc/autocomplete.js
@@ -1,338 +1,201 @@
+    /*jshint validthis:true */

What is this? Is it temporary?

nod_’s picture

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.

amateescu’s picture

Status: Needs review » Needs work

I also tested this manually and, unfortunately, it breaks Entity reference in multiple ways :(

+++ b/core/misc/autocomplete.js
@@ -1,338 +1,201 @@
+  /**
+   * Transforms the data object into an array and update autocomplete results.
+   *
+   * @param {Object} data
+   */
+  function sourceCallbackHandler (data) {
+    // Drupal returns an object, we need an array.
+    var terms = [];
+    for (var i in data) {
+      if (data.hasOwnProperty(i)) {
+        terms.push(data[i]);
+      }
+    }
+    autocomplete.cache[term] = terms;

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

amateescu’s picture

FileSize
1.58 KB

This interdiff fixes the caching problem, working on the rest now.

amateescu’s picture

Status: Needs work » Needs review
FileSize
4.49 KB
25.24 KB

And this fixes return data handling. Interdiff is to #214.

nod_’s picture

Interdiff looks good to me. Thanks :)

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs accessibility review, -js-novice, -Needs JavaScript testing

And the rest of the patch looks good to me. Accessibility testing was done in #213.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, core-js-ui-autocomplete-675446-220.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.8 KB
35.04 KB

Of course, test expectations need to be updated as well.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Xano’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/misc/autocomplete.js
    @@ -1,338 +1,200 @@
    +(function ($, Drupal, drupalSettings) {
    ...
    +})(jQuery, Drupal, drupalSettings);
    

    Can't see any usage of drupalSettings is this necessary?

  2. +++ b/core/misc/autocomplete.js
    @@ -1,338 +1,200 @@
    +   * @param {Array} suggestions
    

    My validator says this in not an array but an object. Since it is called from sourceCallbackHandler and parameter passed in is an object.

  3. +++ b/core/misc/autocomplete.js
    @@ -1,338 +1,200 @@
     /**
      * Performs a cached and delayed search.
      */
    

    Is this comment really documenting what comes next?

  4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceAutocompleteTest.php
    @@ -7,6 +7,7 @@
    +use Drupal\Component\Utility\Json;
    

    This is not used

  5. +++ b/core/modules/system/css/system.module.css
    @@ -37,10 +18,10 @@
    +.js[dir="rtl"] input.form-autocomplete.ui-autocomplete-loading
       background-position: 0% -18px;
     }
    

    Missing opening {

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
34.85 KB

Applied changes in #227.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

All of #227 has been addressed.

droplet’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
21.88 KB

it always search the last char. see:
auto-error.gif

Alan D.’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated 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.

Alan D.’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

@droplet, #230: I consider that a bug too - not a feature. It should search in the middle. Did you file an issue for it?

Xano’s picture

Xano’s picture

webchick’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

No longer applies, sorry. :(

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
34.26 KB

Rerolled.

webchick’s picture

Title: Use jQuery UI Autocomplete » Change notice: Use jQuery UI Autocomplete
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Ok, 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.

Wim Leers’s picture

I found several problems:

  1. There appears to a major regression in the committed code: duplicates are suggested!

    Steps to reproduce:

    1. Make sure a free tagging field already has the "foo" and "bar" values. Assume these are the only two terms in the system.
    2. Now go back to the node and edit, type a comma and "b".
    3. Expected: nothing is suggested. Reality: "bar" is suggested again.

    This problem was in fact implicitly being demonstrated in #230.

  2. This introduced a JSHint error (the same hack as described in #217 should be used to fix that error).
  3. The Seven theme includes an override for jQuery UI's 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 own jquery.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:
    Seven
    Bartik
amateescu’s picture

I really fail to see the "utter crap" in Bartik.

Wim Leers’s picture

It 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.

Wim Leers’s picture

Priority: Major » Critical

The problem described in #238.1 is a critical regression. Updating priority.

catch’s picture

Priority: Critical » Major
Status: Needs work » Active

@Wim please open a new issue for the regressions, unless you think we should roll this back an continue here?

droplet’s picture

#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)

RainbowArray’s picture

Assigned: Unassigned » RainbowArray

I'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.

Damien Tournoud’s picture

Category: Task » Bug report
Priority: Major » Critical

This introduced two real regressions, that might be severe enough to warrant a revert:

  • The autocomplete doesn't support HTML content for the label anymore; this support is used for example in Search API Autocomplete (to display the results number) or in Entity Reference Views to provide rich preview of referenced entities;
  • Our autocomplete callbacks return HTML, but the new autocomplete expects plain-text, as a consequence HTML entities are going to be displayed the wrong way (a node titled Bonnie & Clyde will be rendered in the autocomplete as Bonnie &amp; Clyde).

Temporarily bumping to critical until we have a plan.

Damien Tournoud’s picture

Title: Change notice: Use jQuery UI Autocomplete » Regressions: Use jQuery UI Autocomplete
catch’s picture

I'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.

droplet’s picture

Quick 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.

Damien Tournoud’s picture

#248 sounds very reasonable.

Alan D.’s picture

http://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):

      $autocomplete.autocomplete(autocomplete.options).data( "ui-autocomplete" )._renderItem = function( ul, item ) {
        return $( "<li>" )
            .append( $( "<a>" ).html( item.label ) )
            .appendTo( ul );
};

Effectively changes .text() to .html()

[Edit]
See patch above which does this. Takes way too long to skim read these long threads ;)

scott.gonzalez’s picture

Changing `_renderItem()` alone isn't enough for proper HTML support. Please ping me in IRC (scott_gonzalez) to work through the best solution.

RainbowArray’s picture

Here 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/.

scott.gonzalez’s picture

The `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?

RainbowArray’s picture

Status: Active » Needs review
RainbowArray’s picture

Assigned: RainbowArray » Unassigned
droplet’s picture

@scott.gonzalez,

any updates from #251? Thanks

nod_’s picture

yeah he gave me some code to start, need to make the patch.

nod_’s picture

Status: Needs review » Needs work
RainbowArray’s picture

#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.

nod_’s picture

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.

xjm’s picture

Issue tags: +Missing change record

Tagging "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. ;)

droplet’s picture

I 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:

LABEL NAME

(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.

Alan D.’s picture

I 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.

droplet’s picture

UPDATES 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.

droplet’s picture

Issue summary: View changes
FileSize
1.3 KB
13.64 KB
12.22 KB

Reroll patch in #248

droplet’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Just 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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

The screenshots in #262 alone are very convincing. Code is what I expected to see.

catch’s picture

Status: Reviewed & tested by the community » Fixed

This is where we really miss js testing. Patch looks fine though.

Committed/pushed to 8.x, thanks!

nod_’s picture

#2187779: Add a 'html' option to the autocomplete widget issue summary needs work but created to track it down.

Wim Leers’s picture

Status: Fixed » Active

In 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.

catch’s picture

I'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.

droplet’s picture

These 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.

Wim Leers’s picture

#2186643 is a regression AFAICT.

amateescu’s picture

@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.

Wim Leers’s picture

nod_ 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.

nod_’s picture

Status: Active » Fixed

This is not worth an open critical. The two real regressions have patches and they're pretty close.

xjm’s picture

Title: Regressions: Use jQuery UI Autocomplete » Change record: Use jQuery UI Autocomplete
Category: Bug report » Task
Priority: Critical » Major
Status: Fixed » Active

I think the change record is still outstanding? At least, there's nothing for it in the sidebar.

cosmicdreams’s picture

Change notice started: [#2202701]

https://drupal.org/node/2202701

jessebeach’s picture

Status: Active » Fixed

Thanks cosmicdreams! I've updated the D8 description a little more. I think this is ready to go!

jessebeach’s picture

Title: Change record: Use jQuery UI Autocomplete » Use jQuery UI Autocomplete
Category: Task » Bug report
Priority: Major » Critical
Issue tags: -Needs change record, -Missing change record

Resetting to #278.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.