Skip to content
This repository was archived by the owner on Mar 16, 2018. It is now read-only.

Problem with multipleSeparator == ', '#26

Open
alexis wants to merge 1 commit intoagarzola:masterfrom
alexis:master
Open

Problem with multipleSeparator == ', '#26
alexis wants to merge 1 commit intoagarzola:masterfrom
alexis:master

Conversation

@alexis
Copy link

@alexis alexis commented Nov 4, 2011

Hi, Alfonso!

I found a little bug, when the plugin used with the following settings: {multiple: true, multipleSeparator: ', '}
Basically, it doesn't play well with redundant spaces. Steps to reproduce:

  1. save the following html into the root directory of the project:
<html>
  <head>
    <script type='text/javascript' src='lib/jQuery-1.4.4.min.js'></script>
    <script type='text/javascript' src='jquery.autocomplete.js'></script>
    <script type="text/javascript">
      $().ready(function() {
        $('#ac').autocomplete(['Option', 'Another Option'], {multiple: true, multipleSeparator: ', '});
      });
    </script>
    <link rel="stylesheet" type="text/css" href="jquery.autocomplete.css">
  </head>

  <body>
  <form>
    <input id='ac'>
  </form>
  <body>
</html>
  1. Type "Opt", choose "Option"
  2. Type space several times
  3. Type "Ano", choose "Another Option"

Expected result: "Option, Another Option"
Actual result: "Option, ano"

@alexis
Copy link
Author

alexis commented Nov 4, 2011

(sorry, I didn't updated minified and packed versions of the plugin - just wasn't sure which tools with what options to use)

@agarzola
Copy link
Owner

agarzola commented Nov 4, 2011

Hello, Alexis!

I appreciate your help in finding and squashing this bug. I tried your solution and see that bypassing trimWords does in fact correct the problem. It took me a while to figure out why, though. It turns out that the conditional statement on line 234 is checking the current position of the cursor against the script’s character count to determine where to put the new value in the words map.

var cursorAt = $(input).selection().start;
var wordAt, progress = 0;
$.each(words, function(i, word) {
    progress += word.length;
    if (cursorAt <= progress) {
        wordAt = i;
        return false;
    }
    progress += seperator;
});
words[wordAt] = v;

Since trimWords shaved some extra spaces off, the current cursor position is higher than the script’s current character count (it’s not counting all of those extra spaces) and so the test is false, so wordAt never gets its value. For some reason, this results in an undefined wordAt variable (which I can’t figure out, since, at the very least, it should have a 0 value, as specified in line 231). So, the new value gets stored in an undefined key within the words map. Eliminating that test altogether also corrects the problem, without needing to bypass trimWords altogether):

var wordAt, progress = 0;
$.each(words, function(i, word) {
    progress += word.length;
    wordAt = i;
    progress += seperator;
});
words[wordAt] = v;

I like this option better, as it addresses the real culprit: the fact that the test does not take into account the trimming we’ve done. On the other hand, I can’t figure out why the original author is using this method as opposed to something simpler, such as:

words[words.length] = v;

Seems to me like that would do the trick, without needing to count characters and check them against current cursor position to determine position within words to place the value in. I’ll look closer at this as soon as I get some time and make sure that this solution doesn’t interfere with some other use case.

Thanks again, Alexis, for bringing this to my attention!

@alexis
Copy link
Author

alexis commented Nov 4, 2011

Alfonso, thanks for the quick response!

I think I understand why author didn't use a simpler approach, like words[words.length] = v...
I suppose the $.each cycle with the cursor position check is meant to make it possible to
autocomplete any element of the list in the inbox, even if it is not the latest element in the list.

For example, when the list is "Option**[>>cursor is here<<]**, Another Option, ", you should be
able to hit backspace and to receive the autocompletion dropdown, then you choose "Option"
and the list should be again "Option, Another Option", and not "Optio, Another Option, Option"

Does this make sense?

Btw, when I tried this in browser, I found that the plugin (with or without my fix) doesn't process the described
usercase very well - it produces "Option, Another Option, , " (note redundant coma), which I suppose is better than
"Optio, Another Option, Option", but still needs to be fixed. I can work on it if you want.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexis @agarzola