This is probably the wrong issue queue -- sorry -- don't know where this belongs.

I'm on drupal.org commenting on an issue. I want to change one of the tags from the existing "Needs Documentation" to "Needs Update Documentation". It's in the middle of a list of tags the issue has been tagged with. Let's say the list is API Change, Needs Documentation, UI Freeze.

I go to where Needs Documentation is and start typing Update in the middle of that tag.

I get a type-ahead drop-down box, but it contains suggestions for the UI Freeze tag, not the tag I am actually editing.

So it should either (a) not offer suggestions if I'm not typing at the end of the list, or (b) realize which tag I am on in the list and offer appropriate suggestions.

Files: 
CommentFileSizeAuthor
#48 autocomplete_cursor-992020-48.patch9.3 KBdags
FAILED: [[SimpleTest]]: [MySQL] 36,611 pass(es), 5 fail(s), and 3 exception(s).
[ View ]
#48 interdiff.txt4.6 KBdags
#46 autocomplete_cursor-992020-46.patch9.07 KBdags
FAILED: [[SimpleTest]]: [MySQL] 36,834 pass(es), 30 fail(s), and 33 exception(s).
[ View ]
#43 autocomplete_cursor-992020-43.patch9.08 KBmuka
FAILED: [[SimpleTest]]: [MySQL] 36,756 pass(es), 31 fail(s), and 33 exception(s).
[ View ]
#41 autocomplete_cursor-992020-35.patch8.68 KBmuka
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete_cursor-992020-35_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 autocomplete_cursor-992020-34.patch6.94 KBmuka
FAILED: [[SimpleTest]]: [MySQL] 36,578 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#34 vocabulary.png12.56 KBtheamoeba
#34 terms.png13.72 KBtheamoeba
#34 term_ref_field.png12.91 KBtheamoeba
#34 auto_suggest.png11.05 KBtheamoeba
#34 tagging_selection.png9.76 KBtheamoeba
#32 autocomplete_cursor-992020-32.patch9.08 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,982 pass(es).
[ View ]
#29 autocomplete_cursor-992020-28.patch8.73 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es).
[ View ]
#29 interdiff-24-28.txt4.91 KBxjm
#24 autocomplete_cursor-992020-24.patch8.6 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,853 pass(es).
[ View ]
#24 interdiff.txt492 bytesxjm
#23 autocomplete_caret_position-992020-23.patch8.52 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]
#19 autocomplete_caret_position-992020-19.patch7.73 KBstBorchert
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete_caret_position-992020-19.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#17 autocomplete_caret_position-992020-17.patch5.26 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 29,891 pass(es).
[ View ]
#12 autocomplete_caret_position-992020_another_try.patch5.11 KBrfay
FAILED: [[SimpleTest]]: [MySQL] 29,899 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
#6 autocomplete_caret_position-992020-6.patch5.11 KBstBorchert
FAILED: [[SimpleTest]]: [MySQL] 29,926 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
#2 D7-taxonomy-autocomplete.jpg18.9 KBmr.baileys

Comments

drumm’s picture

Project:Drupal.org infrastructure» Comment alter taxonomy
Version:» 6.x-1.x-dev
Component:Other» User interface
mr.baileys’s picture

Title:Issue tagging type-ahead wrong if in middle of list» Taxonomy autocomplete does not respect cursor position.
Project:Comment alter taxonomy» Drupal core
Version:6.x-1.x-dev» 7.x-dev
Component:User interface» ajax system
StatusFileSize
new18.9 KB

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)

D7-taxonomy-autocomplete.jpg

jhodgdon’s picture

Priority:Normal» Major
Issue tags:+D7UX usability

Thanks for investigating. This looks like something we should fix in D7, and kind of a major usability issue? Even though it was also broken in D6, it still seems major to me.

rfay’s picture

subscribe.

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

chx’s picture

Version:7.x-dev» 8.x-dev
Priority:Major» Normal
stBorchert’s picture

Status:Active» Needs review
StatusFileSize
new5.11 KB
FAILED: [[SimpleTest]]: [MySQL] 29,926 pass(es), 0 fail(s), and 4 exception(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, autocomplete_caret_position-992020-6.patch, failed testing.

jhodgdon’s picture

Strange test glitch: failed to retrieve [autocomplete_caret_position-992020-6.patch] from [].

jhodgdon’s picture

Status:Needs work» Needs review
Issue tags:-D7UX usability

Status:Needs review» Needs work
Issue tags:+D7UX usability

The last submitted patch, autocomplete_caret_position-992020-6.patch, failed testing.

boombatower’s picture

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.

rfay’s picture

Status:Needs work» Needs review
StatusFileSize
new5.11 KB
FAILED: [[SimpleTest]]: [MySQL] 29,899 pass(es), 0 fail(s), and 4 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, autocomplete_caret_position-992020_another_try.patch, failed testing.

stBorchert’s picture

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

stBorchert’s picture

Status:Needs work» Needs review

Next try with PIFR ...

Status:Needs review» Needs work

The last submitted patch, autocomplete_caret_position-992020_another_try.patch, failed testing.

stBorchert’s picture

Status:Needs work» Needs review
StatusFileSize
new5.26 KB
PASSED: [[SimpleTest]]: [MySQL] 29,891 pass(es).
[ View ]

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

chx’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

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.

stBorchert’s picture

Status:Needs work» Needs review
StatusFileSize
new7.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete_caret_position-992020-19.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

First try with one simple test case.

stBorchert’s picture

stBorchert’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +D7UX usability, +needs backport to D7

The last submitted patch, autocomplete_caret_position-992020-19.patch, failed testing.

stBorchert’s picture

Status:Needs work» Needs review
StatusFileSize
new8.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]

Updated patch to latest head.

xjm’s picture

StatusFileSize
new492 bytes
new8.6 KB
PASSED: [[SimpleTest]]: [MySQL] 33,853 pass(es).
[ View ]

Rerolled for core/ with a slight doxygen correction.

jhodgdon’s picture

I read this patch over with regards to documentation.

I would like to suggest renaming the "caret position" variables and arguments to "cursor position". I've never heard of it being called a "caret"? This also applies to some in-code comments.

Also, this docblock is not up to standards at all:

/**
  * Helper function for autocompletion
  */
-function taxonomy_autocomplete($field_name, $tags_typed = '') {
+function taxonomy_autocomplete($field_name, $tags_typed = '', $caret_position = 0) {

http://drupal.org/node/1354#functions

Other docblocks could use some @param/@return docs, but this one is particularly bad.

xjm’s picture

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

xjm’s picture

Assigned:Unassigned» xjm

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

jhodgdon’s picture

Regarding the docblocks, several functions have arguments that are being changed in this patch. Their docblocks should be updated, and the new parameters should be documented. I realize that the former docblocks were garbage, but that is not (I think) a good reason not to document the functions now, especially since the new parameters may not be obvious what they are. They should be documented now.

xjm’s picture

Issue tags:-Needs tests+Novice, +Needs manual testing
StatusFileSize
new4.91 KB
new8.73 KB
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es).
[ View ]

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.

xjm’s picture

Status:Needs review» Needs work

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

xjm’s picture

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new9.08 KB
PASSED: [[SimpleTest]]: [MySQL] 33,982 pass(es).
[ View ]

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

xjm’s picture

Assigned:xjm» Unassigned
theamoeba’s picture

StatusFileSize
new9.76 KB
new11.05 KB
new12.91 KB
new13.72 KB
new12.56 KB

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

I created a taxonomy vocabulary called Testing.
Vocabulary

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

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

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.

Auto SuggestAppending to the end

xjm’s picture

Status:Needs review» Needs work

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.

xjm’s picture

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.

muka’s picture

Assigned:Unassigned» muka
StatusFileSize
new3.2 KB
new7.09 KB
new3.45 KB
new6.94 KB
FAILED: [[SimpleTest]]: [MySQL] 36,578 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

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.

xjm’s picture

Setting NR for the bot. Thanks @muka!

xjm’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, autocomplete_cursor-992020-34.patch, failed testing.

muka’s picture

Status:Needs work» Needs review
StatusFileSize
new8.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch autocomplete_cursor-992020-35_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reviewed to add tests too

Status:Needs review» Needs work

The last submitted patch, autocomplete_cursor-992020-35.patch, failed testing.

muka’s picture

Status:Needs work» Needs review
StatusFileSize
new9.08 KB
FAILED: [[SimpleTest]]: [MySQL] 36,756 pass(es), 31 fail(s), and 33 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, autocomplete_cursor-992020-43.patch, failed testing.

dags’s picture

Assigned:muka» dags
dags’s picture

Status:Needs work» Needs review
StatusFileSize
new9.07 KB
FAILED: [[SimpleTest]]: [MySQL] 36,834 pass(es), 30 fail(s), and 33 exception(s).
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, autocomplete_cursor-992020-46.patch, failed testing.

dags’s picture

Assigned:dags» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.6 KB
new9.3 KB
FAILED: [[SimpleTest]]: [MySQL] 36,611 pass(es), 5 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, autocomplete_cursor-992020-48.patch, failed testing.

tim.plunkett’s picture

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

bas.hr’s picture

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

nod_’s picture

Status:Needs work» Postponed

Yep, that would make sense.

jhodgdon’s picture

Status:Postponed» Needs work

It might make sense for Drupal 8.x, but it doesn't make sense for Drupal 7.x, where this is a long-standing and fairly annoying bug that will need to be fixed in the existing code. Can we consider not postponing it after all?

nod_’s picture

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

droplet’s picture

the backend parser is borken as well. It may affect testbot result. #1329742: Taxonomy autocomplete widget's input validation silently discards invalid input

tkoleary’s picture

Issue summary:View changes

Please follow progress on: #2346973: Replace jQuery UI autocomplete with Select2

Changing to Select2 may render this issue obsolete