From fba13c2374abd1c1c92111b5ce46f1fe7547e030 Mon Sep 17 00:00:00 2001 From: Narciso Jaramillo Date: Wed, 5 Jun 2013 16:25:22 -0700 Subject: [PATCH] Add ability to create multiple global key handlers that override KeyBindingManager. Refactor dialogs and code hints to use this. --- src/command/KeyBindingManager.js | 91 ++++++++++++++++++++++++--- src/editor/CodeHintList.js | 44 +++++++++++-- src/editor/CodeHintManager.js | 12 ---- src/widgets/Dialogs.js | 50 +++------------ test/spec/CodeHint-test.js | 2 +- test/spec/KeyBindingManager-test.js | 95 ++++++++++++++++++++++++++++- 6 files changed, 226 insertions(+), 68 deletions(-) diff --git a/src/command/KeyBindingManager.js b/src/command/KeyBindingManager.js index 99e84e55810..08ac2b3b232 100644 --- a/src/command/KeyBindingManager.js +++ b/src/command/KeyBindingManager.js @@ -24,6 +24,7 @@ /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50, regexp: true, boss: true */ /*global define, $, brackets, window */ +/*unittests: KeyBindingManager */ /** * Manages the mapping of keyboard inputs to commands. @@ -41,21 +42,32 @@ define(function (require, exports, module) { var KeyboardPrefs = JSON.parse(require("text!base-config/keyboard.json")); /** + * @private * Maps normalized shortcut descriptor to key binding info. * @type {!Object.} */ var _keyMap = {}; /** + * @private * Maps commandID to the list of shortcuts that are bound to it. * @type {!Object.>} */ var _commandMap = {}; /** + * @private * Allow clients to toggle key binding + * @type {boolean} */ var _enabled = true; + + /** + * @private + * Stack of registered global keydown handlers. + * @type {Array.} + */ + var _globalKeydownHandlers = []; /** * @private @@ -63,6 +75,7 @@ define(function (require, exports, module) { function _reset() { _keyMap = {}; _commandMap = {}; + _globalKeydownHandlers = []; } /** @@ -482,7 +495,7 @@ define(function (require, exports, module) { * @param {string} A key-description string. * @return {boolean} true if the key was processed, false otherwise */ - function handleKey(key) { + function _handleKey(key) { if (_enabled && _keyMap[key]) { // The execute() function returns a promise because some commands are async. // Generally, commands decide whether they can run or not synchronously, @@ -594,17 +607,74 @@ define(function (require, exports, module) { addBinding(commandId, defaults); } } + + /** + * Adds a global keydown handler that gets first crack at keydown events + * before standard keybindings do. This is intended for use by modal or + * semi-modal UI elements like dialogs or the code hint list that should + * execute before normal command bindings are run. + * + * The handler is passed one parameter, the original keyboard event. If the + * handler handles the event (or wants to block other global handlers from + * handling the event), it should return true. Note that this will *only* + * stop other global handlers and KeyBindingManager from handling the + * event; to prevent further event propagation, you will need to call + * stopPropagation(), stopImmediatePropagation(), and/or preventDefault() + * as usual. + * + * Multiple keydown handlers can be registered, and are executed in order, + * most-recently-added first. + * + * (We have to have a special API for this because (1) handlers are normally + * called in least-recently-added order, and we want most-recently-added; + * (2) native DOM events don't have a way for us to find out if + * stopImmediatePropagation()/stopPropagation() has been called on the + * event, so we have to have some other way for one of the handlers to + * indicate that it wants to block the other handlers from running.) + * + * @param {function(Event): boolean} handler The global handler to add. + */ + function addGlobalKeydownHandler(handler) { + _globalKeydownHandlers.push(handler); + } + + /** + * Removes a global keydown handler added by `addGlobalKeydownHandler`. + * Does not need to be the most recently added handler. + * + * @param {function(Event): boolean} handler The global handler to remove. + */ + function removeGlobalKeydownHandler(handler) { + var index = _globalKeydownHandlers.indexOf(handler); + if (index !== -1) { + _globalKeydownHandlers.splice(index, 1); + } + } + + /** + * Handles a given keydown event, checking global handlers first before + * deciding to handle it ourselves. + * @param {Event} The keydown event to handle. + */ + function _handleKeyEvent(event) { + var i, handled = false; + for (i = _globalKeydownHandlers.length - 1; i >= 0; i--) { + if (_globalKeydownHandlers[i](event)) { + handled = true; + break; + } + } + if (!handled && _handleKey(_translateKeyboardEvent(event))) { + event.stopPropagation(); + event.preventDefault(); + } + } AppInit.htmlReady(function () { // Install keydown event listener. window.document.body.addEventListener( "keydown", - function (event) { - if (handleKey(_translateKeyboardEvent(event))) { - event.stopPropagation(); - event.preventDefault(); - } - }, + _handleKeyEvent, true ); @@ -619,12 +689,13 @@ define(function (require, exports, module) { // Define public API exports.getKeymap = getKeymap; - exports.handleKey = handleKey; exports.setEnabled = setEnabled; exports.addBinding = addBinding; exports.removeBinding = removeBinding; exports.formatKeyDescriptor = formatKeyDescriptor; exports.getKeyBindings = getKeyBindings; + exports.addGlobalKeydownHandler = addGlobalKeydownHandler; + exports.removeGlobalKeydownHandler = removeGlobalKeydownHandler; /** * Use windows-specific bindings if no other are found (e.g. Linux). @@ -635,4 +706,8 @@ define(function (require, exports, module) { * not Linux. */ exports.useWindowsCompatibleBindings = false; + + // For unit testing only + exports._handleKey = _handleKey; + exports._handleKeyEvent = _handleKeyEvent; }); diff --git a/src/editor/CodeHintList.js b/src/editor/CodeHintList.js index 5ed0a313e84..707f3c2daf3 100644 --- a/src/editor/CodeHintList.js +++ b/src/editor/CodeHintList.js @@ -32,6 +32,7 @@ define(function (require, exports, module) { StringUtils = require("utils/StringUtils"), PopUpManager = require("widgets/PopUpManager"), ViewUtils = require("utils/ViewUtils"), + KeyBindingManager = require("command/KeyBindingManager"), KeyEvent = require("utils/KeyEvent"); var CodeHintListHTML = require("text!htmlContent/code-hint-list.html"); @@ -103,6 +104,8 @@ define(function (require, exports, module) { .append($("") .hide()) .append("
    "); + + this._handleKeydown = this._handleKeydown.bind(this); } /** @@ -265,12 +268,24 @@ define(function (require, exports, module) { return {left: posLeft, top: posTop, width: availableWidth}; }; + /** + * Check whether keyCode is one of the keys that we handle or not. + * + * @param {number} keyCode + */ + CodeHintList.prototype.isHandlingKeyCode = function (keyCode) { + return (keyCode === KeyEvent.DOM_VK_UP || keyCode === KeyEvent.DOM_VK_DOWN || + keyCode === KeyEvent.DOM_VK_PAGE_UP || keyCode === KeyEvent.DOM_VK_PAGE_DOWN || + keyCode === KeyEvent.DOM_VK_RETURN || keyCode === KeyEvent.DOM_VK_TAB); + + }; + /** * Convert keydown events into hint list navigation actions. * * @param {KeyBoardEvent} keyEvent */ - CodeHintList.prototype.handleKeyEvent = function (event) { + CodeHintList.prototype._handleKeydown = function (event) { var keyCode, self = this; @@ -326,10 +341,20 @@ define(function (require, exports, module) { } // (page) up, (page) down, enter and tab key are handled by the list - if (event.type === "keydown") { + if (event.type === "keydown" && this.isHandlingKeyCode(event.keyCode)) { keyCode = event.keyCode; - if (keyCode === KeyEvent.DOM_VK_UP) { + if (event.shiftKey && + (event.keyCode === KeyEvent.DOM_VK_UP || + event.keyCode === KeyEvent.DOM_VK_DOWN || + event.keyCode === KeyEvent.DOM_VK_PAGE_UP || + event.keyCode === KeyEvent.DOM_VK_PAGE_DOWN)) { + this.handleClose(); + + // Let the event bubble. + return false; + } + else if (keyCode === KeyEvent.DOM_VK_UP) { _rotateSelection.call(this, -1); } else if (keyCode === KeyEvent.DOM_VK_DOWN) { _rotateSelection.call(this, 1); @@ -342,12 +367,17 @@ define(function (require, exports, module) { // Trigger a click handler to commmit the selected item $(this.$hintMenu.find("li")[this.selectedIndex]).trigger("click"); } else { - // only prevent default handler when the list handles the event - return; + // Let the event bubble. + return false; } + event.stopImmediatePropagation(); event.preventDefault(); + return true; } + + // If we didn't handle it, let other global handlers handle it. + return false; }; /** @@ -387,6 +417,8 @@ define(function (require, exports, module) { this.opened = true; PopUpManager.addPopUp(this.$hintMenu, this.handleClose, true); + + KeyBindingManager.addGlobalKeydownHandler(this._handleKeydown); } }; @@ -416,6 +448,8 @@ define(function (require, exports, module) { PopUpManager.removePopUp(this.$hintMenu); this.$hintMenu.remove(); + + KeyBindingManager.removeGlobalKeydownHandler(this._handleKeydown); }; /** diff --git a/src/editor/CodeHintManager.js b/src/editor/CodeHintManager.js index f0aa393aa80..5b8e2e7cb91 100644 --- a/src/editor/CodeHintManager.js +++ b/src/editor/CodeHintManager.js @@ -458,18 +458,6 @@ define(function (require, exports, module) { function handleKeyEvent(editor, event) { keyDownEditor = editor; if (event.type === "keydown") { - if (_inSession(editor) && hintList.isOpen()) { - if (event.shiftKey && - (event.keyCode === KeyEvent.DOM_VK_UP || - event.keyCode === KeyEvent.DOM_VK_DOWN || - event.keyCode === KeyEvent.DOM_VK_PAGE_UP || - event.keyCode === KeyEvent.DOM_VK_PAGE_DOWN)) { - _endSession(); - } else { - // Pass event to the hint list, if it's open - hintList.handleKeyEvent(event); - } - } if (!(event.ctrlKey || event.altKey || event.metaKey) && (event.keyCode === KeyEvent.DOM_VK_ENTER || event.keyCode === KeyEvent.DOM_VK_RETURN || diff --git a/src/widgets/Dialogs.js b/src/widgets/Dialogs.js index b4e33e2e388..00101ae74da 100644 --- a/src/widgets/Dialogs.js +++ b/src/widgets/Dialogs.js @@ -58,14 +58,6 @@ define(function (require, exports, module) { DIALOG_BTN_CLASS_LEFT = "left"; - /** - * A stack of keydown event handler functions that corresponds to the - * current stack of modal dialogs. - * - * @type {Array.} - */ - var _keydownListeners = []; - function _dismissDialog(dlg, buttonId) { dlg.data("buttonId", buttonId); $(".clickable-link", dlg).off("click"); @@ -117,11 +109,13 @@ define(function (require, exports, module) { } else if (!($.contains(this.get(0), e.target)) || !inFormField) { // Stop the event if the target is not inside the dialog // or if the target is not a form element. - // TODO (issue #414): more robust handling of dialog scoped - // vs. global key bindings e.stopPropagation(); e.preventDefault(); } + + // Stop any other global handlers from processing the event (but + // allow it to continue bubbling if we haven't otherwise stopped it). + return true; }; @@ -200,7 +194,7 @@ define(function (require, exports, module) { }); var handleKeyDown = function (e) { - _handleKeyDown.call($dlg, e, autoDismiss); + return _handleKeyDown.call($dlg, e, autoDismiss); }; // Pipe dialog-closing notification back to client code @@ -220,24 +214,8 @@ define(function (require, exports, module) { // Remove the dialog instance from the DOM. $dlg.remove(); - // Remove keydown event handler - window.document.body.removeEventListener("keydown", handleKeyDown, true); - - // Remove this dialog's listener from the stack of listeners. (It - // might not be on the top of the stack if the dialogs are somehow - // dismissed out of order.) - _keydownListeners = _keydownListeners.filter(function (listener) { - return listener !== handleKeyDown; - }); - - // Restore the previous listener if there was one; otherwise return - // control to the KeyBindingManager. - if (_keydownListeners.length > 0) { - var previousListener = _keydownListeners[_keydownListeners.length - 1]; - window.document.body.addEventListener("keydown", previousListener, true); - } else { - KeyBindingManager.setEnabled(true); - } + // Remove our global keydown handler. + KeyBindingManager.removeGlobalKeydownHandler(handleKeyDown); }).one("shown", function () { // Set focus to the default button var primaryBtn = $dlg.find(".primary"); @@ -246,18 +224,8 @@ define(function (require, exports, module) { primaryBtn.focus(); } - // If this is the only dialog then also disable the KeyBindingManager; - // otherwise, remove and the previous dialog's listener before installing the new one. - if (_keydownListeners.length > 0) { - var previousListener = _keydownListeners[_keydownListeners.length - 1]; - window.document.body.removeEventListener("keydown", previousListener, true); - } else { - KeyBindingManager.setEnabled(false); - } - _keydownListeners.push(handleKeyDown); - - // Handle this dialog's keyboard events with the current listener - window.document.body.addEventListener("keydown", handleKeyDown, true); + // Push our global keydown handler onto the global stack of handlers. + KeyBindingManager.addGlobalKeydownHandler(handleKeyDown); }); // Click handler for buttons diff --git a/test/spec/CodeHint-test.js b/test/spec/CodeHint-test.js index a0d04e2d810..f9587a01eca 100644 --- a/test/spec/CodeHint-test.js +++ b/test/spec/CodeHint-test.js @@ -202,7 +202,7 @@ define(function (require, exports, module) { editor = EditorManager.getCurrentFullEditor(); expect(editor).toBeTruthy(); - CodeHintManager.handleKeyEvent(editor, e); + CodeHintManager._getCodeHintList()._handleKeydown(e); // doesn't matter what was inserted, but line should be different var newPos = editor.getCursorPos(); diff --git a/test/spec/KeyBindingManager-test.js b/test/spec/KeyBindingManager-test.js index 365948ff26e..24e12610e0d 100644 --- a/test/spec/KeyBindingManager-test.js +++ b/test/spec/KeyBindingManager-test.js @@ -24,6 +24,7 @@ /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ /*global define, describe, beforeEach, afterEach, it, xit, runs, waitsFor, expect, brackets, $ */ +/*unittests: KeyBindingManager */ define(function (require, exports, module) { 'use strict'; @@ -64,11 +65,13 @@ define(function (require, exports, module) { var platform = brackets.platform; beforeEach(function () { + CommandManager._testReset(); KeyBindingManager._reset(); brackets.platform = "test"; }); afterEach(function () { + CommandManager._testRestore(); brackets.platform = platform; }); @@ -387,11 +390,101 @@ define(function (require, exports, module) { KeyBindingManager.addBinding("test.foo", "Ctrl-A"); expect(fooCalled).toBe(false); - KeyBindingManager.handleKey("Ctrl-A"); + KeyBindingManager._handleKey("Ctrl-A"); expect(fooCalled).toBe(true); }); }); + describe("global handlers", function () { + var commandCalled, handler1Called, handler2Called, ctrlAEvent; + + function keydownHandler1(event) { + handler1Called = true; + return true; + } + + function keydownHandler2(event) { + handler2Called = true; + return true; + } + + function makeKeyEvent() { + // We don't create a real native event object here--just a fake + // object with enough info for the key translation to work + // properly--since our mock handlers don't actually look at it + // anyway. + return { + ctrlKey: true, + keyCode: "A".charCodeAt(0), + immediatePropagationStopped: false, + propagationStopped: false, + defaultPrevented: false, + stopImmediatePropagation: function () { + this.immediatePropagationStopped = true; + }, + stopPropagation: function () { + this.propagationStopped = true; + }, + preventDefault: function () { + this.defaultPrevented = true; + } + }; + } + + beforeEach(function () { + commandCalled = false; + handler1Called = false; + handler2Called = false; + ctrlAEvent = makeKeyEvent(); + CommandManager.register("FakeUnitTestCommand", "unittest.fakeCommand", function () { + commandCalled = true; + }); + KeyBindingManager.addBinding("unittest.fakeCommand", "Ctrl-A"); + }); + + it("should block command execution if a global handler is added that prevents it", function () { + KeyBindingManager.addGlobalKeydownHandler(keydownHandler1); + KeyBindingManager._handleKeyEvent(ctrlAEvent); + expect(handler1Called).toBe(true); + expect(commandCalled).toBe(false); + + // In this case, the event should not have been stopped, because our handler didn't stop it + // and KBM didn't handle it. + expect(ctrlAEvent.immediatePropagationStopped).toBe(false); + expect(ctrlAEvent.propagationStopped).toBe(false); + expect(ctrlAEvent.defaultPrevented).toBe(false); + }); + + it("should not block command execution if a global handler is added then removed", function () { + KeyBindingManager.addGlobalKeydownHandler(keydownHandler1); + KeyBindingManager.removeGlobalKeydownHandler(keydownHandler1); + KeyBindingManager._handleKeyEvent(ctrlAEvent); + expect(handler1Called).toBe(false); + expect(commandCalled).toBe(true); + + // In this case, the event should have been stopped (but not immediately) because + // KBM handled it. + expect(ctrlAEvent.immediatePropagationStopped).toBe(false); + expect(ctrlAEvent.propagationStopped).toBe(true); + expect(ctrlAEvent.defaultPrevented).toBe(true); + }); + + it("should call the most recently added handler first", function () { + KeyBindingManager.addGlobalKeydownHandler(keydownHandler1); + KeyBindingManager.addGlobalKeydownHandler(keydownHandler2); + KeyBindingManager._handleKeyEvent(ctrlAEvent); + expect(handler2Called).toBe(true); + expect(handler1Called).toBe(false); + expect(commandCalled).toBe(false); + + // In this case, the event should not have been stopped, because our handler didn't stop it + // and KBM didn't handle it. + expect(ctrlAEvent.immediatePropagationStopped).toBe(false); + expect(ctrlAEvent.propagationStopped).toBe(false); + expect(ctrlAEvent.defaultPrevented).toBe(false); + }); + }); + }); }); \ No newline at end of file