Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] QuickOpen file search initial selection is unpredictable #4430

Open
core-ai-bot opened this issue Aug 29, 2021 · 13 comments
Open

[CLOSED] QuickOpen file search initial selection is unpredictable #4430

core-ai-bot opened this issue Aug 29, 2021 · 13 comments

Comments

@core-ai-bot
Copy link
Member

Issue by iwehrman
Friday Aug 16, 2013 at 16:26 GMT
Originally opened as adobe/brackets#4791


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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Aug 16, 2013 at 17:19 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by gruehle
Monday Aug 19, 2013 at 18:57 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by iwehrman
Thursday Oct 17, 2013 at 19:38 GMT


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. 😡 🔥 💻 🔥

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Oct 17, 2013 at 20:06 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Oct 17, 2013 at 20:06 GMT


Let's nominate it for next sprint.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Oct 24, 2013 at 00:19 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Nov 01, 2013 at 20:59 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Nov 01, 2013 at 21:14 GMT


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.]

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Nov 01, 2013 at 21:19 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Nov 01, 2013 at 21:28 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Nov 06, 2013 at 01:49 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Thursday Nov 07, 2013 at 22:18 GMT


FBNC back to@iwehrman

@core-ai-bot
Copy link
Member Author

Comment by iwehrman
Friday Nov 08, 2013 at 18:06 GMT


Lovin' it! 🍟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant