Typing quickly in Quick Open (Go to Definition mode) goes to wrong location on Enter key

[peterflynn]

1. Open a JS file (e.g.: jquery.smart-autocomplete.js)
2. Press Ctrl+T
3. Quickly type "keyin" and quickly hit Enter afterward

Result:
Cursor jumps to "_default" or some other function instead of the keyIn function.
Exceptions in console log.

Expected:
If you wait a beat before hitting Enter, it jumps to the correct location.
(Although sometimes the cursor location is still wrong until you hit Enter).

This repros more easily on slower computers that bog down a bit when the editor scroll position is jumping around while typing.

[peterflynn]

I think the problem is as follows:

• smart-autocomplete has an async wait between receiving a keyup event and processing it (i.e. filtering and formatting the results)
• QuickOpen.js responds to keyup events synchronously: pulling the top item from the autocomplete results for use with its plugins' focus/select handlers. It also closes the search bar synchronously on Enter keydown.
• So if processing keys is slow enough, we'll sync handle the Enter key while smart-autocomplete is still waiting for its timeout to async handle an earlier letter key. This has two consequences:
  • When we sync handle the Enter key, we grab the top item off a stale results list, hence jumping to the wrong location.
  • Smart-autocomplete will still process the earlier letter key async later, though the toolbar has already been torn down by the time. The Exception when selecting Quick Open result by clicking #1384 fix prevents smart-autocomplete from choking on this, but our _handleResultsFormatter() comes up with 'undefined' as the query text, crashing the plugin handler it calls. Hence the exception thrown.

[peterflynn]

The lesser bug in the "expected" case works similarly: we call the plugin's "focus" handler on the top list item sync after each keyup, but since smart-autocomplete processes keyup async, the "focus" handler is always getting called with stale results -- even if you type very slowly it will always be one keystroke behind.

So actually this "lesser" bug is hit 100% of the time -- you just don't notice it because you usually type enough chars to over-constrain the filtering/sorting, and the top result when you're done typing is usually the same as the top result if you back up one char.

[peterflynn]

It would be nice if we could just patch smart-autocomplete to not be so async, but it's pulled in as a submodule and we don't have our own fork of it... so it would be kind of a pain.

Alternatively we could have our code wait for 'resultsReady' after the keystroke is done being processed, but that's tricky since then we no longer have an event object e.g. to check if it was the Enter key. Ideally we could force smart-autocomplete to do a filter-render pass synchronously when we know we want its results immediately, but it has no API for that...

[peterflynn]

Here's another case of this problem, originally noted in #1627:
If Quick Open is invoked via the menu with prepopulated search text, it doesn't immediately jump to the result -- but it does if invoked via keyboard shortcut instead. This is because the twin keyups from the shortcut go to the Quick Open toolbar: each time QuickOpen tries to 'focus' the top result, and then smart-autocomplete updates the results list. At the first keyup, QuickOpen can't find any results, and then later smart-autocomplete populates them. At the second keyup QuickOpen tries again and this time finds a result to jump to.

[pthiess]

Reviewed - @peterflynn I make this a medium priority, but don't think we have to fix this in Sprint 15.

[njx]

Nominating for sprint 17--I run into this pretty often.

[njx]

BTW, it's very noticeable in a big file like codemirror.js -- that's a good stress test.

[njx]

Moving out of sprint 17. Probably wouldn't land today, seems risky, and is in an unofficial feature.

[njx]

Nominating for sprint 18 since it was high enough priority for us to want to pull into sprint 17.

[peterflynn]

Another oddity, from #2332: smart-autocomplete does some key handling synchronously, like we do (except on keyup, whereas we do it on keydown). Notably, it handles Enter synchronously. So seems like they'd encounter the same race condition bugs as us: user types a letter, which merely kicks off a timeout; user hits Enter too quickly, and it gets handled synchronously even though the timeout has yet to run, so Enter selects a stale result. Do they do anything to avoid this? Or is it mere luck that keyup is a bit later and thus a bit less likely to repro the bug?

[peterflynn]

To answer my own question above: yes, smart-autocomplete is just getting lucky because its handler comes slightly later. It too will crash if you lengthen the timeout just a little bit from the default value of zero (even if QuickOpen's key handling is turned off). It seems like smart-autocomplete is just really not designed to be used on non-permanent UI that might get destroyed.