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] REVIEW ONLY: Add ability to create multiple global key handlers #3829

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

Comments

@core-ai-bot
Copy link
Member

Issue by njx
Wednesday Jun 05, 2013 at 23:33 GMT
Originally opened as adobe/brackets#4110


This is an alternate fix for #3975. It creates a more general mechanism for dealing with multiple global capture handlers for keydown.

I think this is a cleaner fix than #4032, and it has the advantage that we can also use this same mechanism to simplify the nested-dialog key handling in Dialogs.js. However, it is somewhat more invasive (though I don't think it's necessarily more risky than #4032 if we can convince ourselves that the logic is correct).

The semantics of these new global keydown handlers is a bit tricky. The idea is that you can install multiple global keydown handlers, with the last-added one running first. Any of these handlers can return true in order to indicate that it handled the key. However, this does not have the same effect as the usual event stopPropagation()/preventDefault(); all it does is prevent the other global keydown handlers (including KeyBindingManager itself) from running. The desired intention is that this is the way modal/semi-modal key handling should work: only one modal should get to deal with the key, but it should be able to decide that the key should go through normal event handling (bypassing other modals further down the stack, including ordinary key bindings).

Also note that this introduces a subtle change in how keys are handled by code hints. It used to be that key events while the code hint list is up would come from CodeMirror, passed into CodeHintManager, and from there to the CodeHintList. Now, CodeHintList itself installs a global capture handler for the key events it cares about. I think this actually makes more sense, since the CodeHintList is basically a semi-modal UI, and should be trapping these key events directly, rather than relying on them coming from the editor.

I tested various cases to try to verify that this is working properly:

  • the nested dialogs in ExtensionManager
  • code hints without Emmet installed--tested that up/down arrows, Enter and Esc work as expected while code hints are up, and after they're dismissed nothing unusual happens
  • code hints with Emmet installed--tested that this actually fixed the original bug :)

njx included the following code: https://github.com/adobe/brackets/pull/4110/commits

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Jun 05, 2013 at 23:36 GMT


@iwehrman - you might be interested in weighing in on this since you wrote the most recent iteration of the Dialog key handling code and are also familiar with the code hints stuff.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Wednesday Jun 05, 2013 at 23:52 GMT


@njx The Emmet extension checks to see if Brackets is displaying code hints when deciding if it should execute command, which author considers a hack (see https://github.com/emmetio/brackets-emmet/blob/master/main.js#L84). This is being tracked in adobe/brackets#2455. Is that bug also fixed with this change?

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Jun 06, 2013 at 00:19 GMT


I didn't think of that explicitly, but I do think this fix might make possible to get rid of that hack, since we're now handling these key events in the code hint list during the capture phase and preventing KeyBindingManager from executing associated commands if we handle it. But even if he doesn't remove that hack, this fix will still work.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Jun 06, 2013 at 21:17 GMT


Fixes pushed, ready for re-review.

@core-ai-bot
Copy link
Member Author

Comment by gruehle
Thursday Jun 06, 2013 at 21:22 GMT


Looks good. Merging.

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