Taxonomy autocomplete does not respect cursor position.

Comment alter taxonomy just re-uses Taxonomy's forms and auto-complete logic, so this is not the correct queue. Seems that the same behavior occurs on standard D6 and D7 sites wherever a free-tagging vocabulary is used and without project_issue/comment_alter_taxonomy.

The taxonomy auto-complete seems to always auto-complete the last item in the list instead of respecting the cursor to see which term is being edited (see D7 screenshot below)

subscribe.

Actually, autocomplete has not had enough attention over time and there are a number of issues open for it.

Here is a first working patch. If you edit a tag in the list its text will be used to search matching terms.
See https://img.skitch.com/20110604-fx6du4sy8pxrjq5mc57texpy4q.png

Seems something on d.o didn't give PIFT the proper URL to send over to qa.d.o. Hmm.

Created #1178406: All tests failing: Relative patch file path used instead of absolute in patch testing.

Just to see if it's some unexpected thing about file naming, here's the same patch with a different name. This is the same as #6.

Seems like PIFR didn't use the full path to the file ("//" in the beginning of path instead of a single "/").
https://img.skitch.com/20110604-ddu6xgxfgnfw5a4691t75dsh4q.png

Next try with PIFR ...

Next try with minor fix to prevent errors in test case.

Thanks for fixing this rather pesky issue. While the JS cant be tested easily surely you could emulate the requests JS will make and verify results.

First try with one simple test case.

#19: autocomplete_caret_position-992020-19.patch queued for re-testing.

#19: autocomplete_caret_position-992020-19.patch queued for re-testing.

Updated patch to latest head.

Rerolled for core/ with a slight doxygen correction.

#25 I noticed that as well, but those lines aren't part of the patch change, so should be fixed in a separate issue. I am expecting taxonomy.module cleanup to take care of that.

I am also -1 on "caret".

Eh I'll just rename the variable myself. :)

The attached patch replaces the word "caret" with "cursor" and also polishes the inline comments a bit. I didn't touch the docblock for taxonomy_autocomplete() for the reasons stated in #26, but I made note of it in #1326644: Clean up API docs for taxonomy module.

The patch includes automated tests already. However, since this is JS functionality, we should have a few people test the functionality and report the results. Tagging as novice for manual review and testing.

#28 Oh! Now I understand what you mean. Okay, fixing that. Sorry. :)

Opened #1337124: Improve API documentation for taxonomy_autocomplete().

Now that #1337124: Improve API documentation for taxonomy_autocomplete() is in, here's a reroll that adds documentation for the additional parameter.

I have tested this patch on minimal-8.0-dev.

I created a taxonomy vocabulary called Testing.

I then populated it it with three terms: Alpha, Beta, Gamma.

I created a content type called Page and added the term reference field as an autocomplete term widget (tagging).

Before implementing the patch in #32 I could replicated this issue.
After implementing the patch and clearing the cache I saw the following behavior: the patch fixes the issue respecting the cursor position but when selecting a suggested term it appends it to the end which is confusing to the user.

Thanks @theamoeba. That's a perfect test writeup. :)

I think we should change the function so it also orders the terms as expected after autocomplete. As @theamoeba pointed out in IRC, it becomes a usability issue.

From IRC:

davereid: xjm: Hrm, adding cursor position as an URL path seems odd. That should probably be a query string?

See also:
http://docs.jquery.com/UI/Autocomplete
http://jqueryui.com/demos/autocomplete/
Per Dave Reid there's an issue to replace our autocomplete with that in D8.

Hi,
here a revision of the patch #32 by xjm. It miss the autocomplete tests in taxonomy.test as I didn't found the appropriate code.

Setting NR for the bot. Thanks @muka!

Reviewed to add tests too

Rerolled.

Reroll.

Moving test into subclass to reduce number of fails. I'm also taking this off of myself for someone else to work on fixing the test failures.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -122,12 +121,28 @@ function taxonomy_autocomplete($field_name, $tags_typed = '') {
+  $position_last = -1;
+  foreach ($tag_positions as $position) {
+    if ($cursor_position >= $position_last && $cursor_position < $position) {
+      // Tag found on cursor position.
+      $tag_current = $tags_typed_array[$position_last];

This will always fail the first time around, since -1 is never valid.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -122,12 +121,28 @@ function taxonomy_autocomplete($field_name, $tags_typed = '') {
-  if ($tag_last != '') {
+  if ($tag_current != '') {

With this change, $term_matches can now be empty, whereas before it wasn't.

Should we postpone this until #675446: Use jQuery UI Autocomplete is resolved?

Yep, that would make sense.

Oh right, D7 slipped out of my mind. Sorry about that.

the backend parser is borken as well. It may affect testbot result. #1329742: Autocomplete with tagging silently discards invalid input

Please follow progress on: #2346973: Improve usability, accessibility, and scalability of long select lists

Changing to Select2 may render this issue obsolete

Let's triage this a little more and see if

1. this is an issue on D8. If it is, fix there.
2. Then this will need to be backported to D7.

As of beta 10 I can no longer reproduce this.

Maybe it was solved by jquery update.

OK, I see now. I misunderstood the summary.

This issue would be solved in 8 by using Select2 if that issue lands #2346973: Improve usability, accessibility, and scalability of long select lists, which it could even in 8.1 since it does not impact backwards compatibility AFAICT.

IMO that's where we should put the effort and not waste time with a half-way fix to a problem for which there is a simple work around (delete the tag and start typing again at the end of the list).

Related issue #2186643: Autocomplete always searches the last tag (probably duplicate of this)