search

webscopeio / react-textarea-autocomplete

📝 React component implements configurable GitHub's like textarea autocomplete.
MIT License
451 stars 80 forks source link

Escape key is not propagated sometimes #135

Closed O4epegb closed 5 years ago

O4epegb commented 5 years ago

So, escape key propagation stops after autocomplete is shown first time.

Steps to reproduce: (for example here https://874-94480675-gh.circle-artifacts.com/0/example/index.html) 1) Press Escape. In console there will be log indicating that Escape was pressed 2) Type "@" to trigger autocomplete, then select value 3) Press Escape again, now there is no console log, because it is not propagating

It seems that every time you trigger autocomplete popup, one listener for Escape key is added (to prevent propagation), but after popup is closed listener is not removed for some reason. So if you trigger popup 10 times there will be 10 listeners still alive. I cannot figure out why, because this code seems to work and it actually trying to remove listeners after popup is closed. But it doesn't work for some reason.

O4epegb commented 5 years ago

I think I got it!

It might be because of this block inside listener code

    this.listeners[this.index] = {
      keyCode,
      fn
    };

    this.index += 1;

    return this.index;

So for example, when we add first listener the index starts with 0. We add it to hashmap of listeners, but then we increment index to 1 and return it (that a bug, we should return 0). So when we try to delete we actually deleting wrong listener every time.

O4epegb commented 5 years ago

It is actually quite strange pattern to use (indexes), maybe we should refactor it to more common observer implementation with just function references?

jukben commented 5 years ago

Oh, that makes sense. It should be return this.index++; or something if I'm correct. But definitely, I'm for refactoring it. Will you give it a shot?

Thanks for the issue!

This might be also some lead for the bug https://github.com/webscopeio/react-textarea-autocomplete/pull/126

O4epegb commented 5 years ago

Yep! I think I will do fast fix like return this.index++; and maybe refactor afterwards!

jukben commented 5 years ago

Yeah, the quick fix is an OK solution for now! Thanks for this. Once you test it out that it works for your use case, please create a PR. I will merge it ASAP. Thanks for letting me know! 💪