QuickOpen file search initial selection is unpredictable

[iwehrman]

This is probably a known issue, but I couldn't find anything in the tracker.

The subject basically summarizes the issue: when you start typing in the QuickOpen file search dialog, the initially selected item is unpredictable: sometimes it's the first item, sometimes it's an item other than the first, and sometimes I don't see a selection at all. (If the latter case, pressing enter seems to open the first entry.)

It seems like the correct behavior is that the first item should always be selected by default when starting a new QuickOpen file search.

[njx]

One possible issue is that if the mouse happens to be over the list when it drops down, and you nudge it slightly, that's what gets selected first (because hover changes selection). That seems to happen to me fairly often. Personally, I don't think hovering should change the selection.

[gruehle]

Reviewed, low priority to @njx. Can we just remove the select on hover functionality?

[iwehrman]

I vote that this be bumped to medium priority. It doesn't just happen if your mouse is hovering over the menu. (Although that does happen quite a lot for me.) If you hover your mouse in the middle of the menu and accidentally open the wrong file as a result of this bug, and then move your mouse out of the way to execute QuickOpen again, then the selection remains, by default, in the middle of the menu, which causes the erroneous behavior a second time in a row. 😡 🔥 💻 🔥

[njx]

Works for me, I don't like it either :)

[njx]

Let's nominate it for next sprint.

[njx]

It looks like we would need to actually fix this in smart-autocomplete, as there's no option to distinguish between hover and key navigation. I would guess it's easy to add that, but there have been a number of other fixes in smart-autocomplete since we last updated it, so if we submit a pull request for this we'd need to get those changes as well. It actually looks like many (if not all) of those changes were actually submitted by folks associated with Brackets (e.g. for #2757), but we haven't pulled them yet because we were waiting to do a general cleanup of Quick Open: https://trello.com/card/quick-open-full-design-ux-polish/4f90a6d98f77505d7940ce88/834

Marking this as Needs Review to figure out if we want to try to do this as a one-off bug fix, or to actually submit a patch to smart-autocomplete and then do the work of merging it back and testing to make sure the other patches didn't break anything (in advance of doing the full backlog card).

[njx]

We decided to go ahead and try to update smart-autocomplete. However, I just pulled the latest upstream and it seems to break Quick Open, so some change that was made since our last update isn't innocuous.

/cc @dangoor @WebsiteDeveloper @SAplayer as an FYI since you guys have made patches to smart-autocomplete. I'll dig into it and see what seems to be broken.

[njx]

The issue appears to be the change in laktek/jQuery-Smart-Auto-Complete#26 from @WebsiteDeveloper that changes the keyup handler in smart-autocomplete to a keydown handler. It looks like this was intended to make repeat work for holding down arrow keys, but it breaks Quick Open completely. There appears to be a bunch of finicky logic in QuickOpen.js related to the timing of how we handle key events; I tried just changing that logic to listen to keydown instead of keyup, but it still didn't work.

Given where we are now, if we still want to fix this bug, my preference would be to change smart-autocomplete back to using keyup so we can keep the existing logic, and then add whatever other fixes we need on top of it. Longer term, we should consider fixing or rewriting the dropdown, as in #5703.

[Edited to remove some misleading stuff about that change breaking Brackets...it probably worked in Brackets at the time but we never merged it, and we added the keyup handling later.]

[njx]

Marking this needs review. I'll bring it up in our next standup to see if we should continue trying to move forward on this (either by re-patching it or forking), or wait until we get to the official Quick Open story and consider rewriting the control.

(I'd note that it doesn't really look like smart-autocomplete is being actively maintained anyway--pretty much all the patches in the last year have come from people associated with Brackets--so forking/rewriting doesn't seem like a bad alternative.)

[njx]

Looking into this a little further, it looks like our change to keyup happened after the keydown change was submitted to smart-autocomplete (but we hadn't merged that change back into Brackets), so that change would probably have worked in Brackets at the time but would have broken later. It's possible that some rejiggering of the Quick Open logic would work with the keydown change, but it definitely seems a little dangerous to do that as part of a bug fix without doing some testing, so it makes sense to combine it with the larger Quick Open revamp.

[njx]

We discussed this at the architecture meeting, and we think at this point it would be best in the short term to just monkey-patch smart autocomplete rather than try to submit fixes to the main repo, since we're probably going to replace it with something else eventually anyway. So, my plan is to remove the submodule and just copy the latest version of smart autocomplete into the Brackets repo, revert the keydown change to keyup (which should make it work in Brackets again with the same behavior that Brackets has currently), and then turn off the hover behavior directly.

@WebsiteDeveloper - if you'd like to restore the keydown change, we'd need to figure out how to make it work with the current Quick Open code. Feel free to investigate that if you'd like, but it doesn't seem like a high priority at the moment.

[redmunds]

FBNC back to @iwehrman

[iwehrman]

Lovin' it! 🍟