Problem with multipleSeparator == ', ' #26

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

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

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 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 join this conversation on GitHub. Already have an account? Sign in to comment