From befaf39aca3af22756a988134245bf1a88a7481d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 9 Apr 2020 10:30:26 +1000 Subject: [PATCH] DEV: Refactor and test plugin addKeyboardShortcut (#9381) Refactor plugin-api `addKeyboardShortcut` to point to `KeyboardShortcuts`. * Do not add shortcuts to the default object directly. * Create an addShortcut function in keyboard-shortcuts to add shortcuts safely and call to bindKey to be able to use opts. * Refactor controllers/bookmark.js to use new addShortcut func and emove unnecessary addBindings. * No longer export keyboard shortcut bindings, rename to DEFAULT_BINDINGS and remove export, these do not need to be accessed by anything else. --- .../discourse/controllers/bookmark.js | 17 +++--- .../discourse/lib/keyboard-shortcuts.js | 46 +++++++++------- .../javascripts/discourse/lib/plugin-api.js | 12 ++--- .../plugin-keyboard-shortcut-test.js | 54 +++++++++++++++++++ .../components/keyboard-shortcuts-test.js | 6 +-- 5 files changed, 99 insertions(+), 36 deletions(-) create mode 100644 test/javascripts/acceptance/plugin-keyboard-shortcut-test.js diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js b/app/assets/javascripts/discourse/controllers/bookmark.js index 140b6f85974..22218715b3e 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js +++ b/app/assets/javascripts/discourse/controllers/bookmark.js @@ -113,20 +113,23 @@ export default Controller.extend(ModalFunctionality, { bindKeyboardShortcuts() { KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE); - KeyboardShortcuts.addBindings(BOOKMARK_BINDINGS, binding => { - if (binding.args) { - return this.send(binding.handler, ...binding.args); - } - this.send(binding.handler); + Object.keys(BOOKMARK_BINDINGS).forEach(shortcut => { + KeyboardShortcuts.addShortcut(shortcut, () => { + let binding = BOOKMARK_BINDINGS[shortcut]; + if (binding.args) { + return this.send(binding.handler, ...binding.args); + } + this.send(binding.handler); + }); }); }, unbindKeyboardShortcuts() { - KeyboardShortcuts.unbind(BOOKMARK_BINDINGS, this.mouseTrap); + KeyboardShortcuts.unbind(BOOKMARK_BINDINGS); }, restoreGlobalShortcuts() { - KeyboardShortcuts.unpause(...GLOBAL_SHORTCUTS_TO_PAUSE); + KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE); }, // we always want to save the bookmark unless the user specifically diff --git a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js index 37fe28618c6..db3400cabda 100644 --- a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js +++ b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js @@ -6,7 +6,7 @@ import { ajax } from "discourse/lib/ajax"; import { throttle } from "@ember/runloop"; import { INPUT_DELAY } from "discourse-common/config/environment"; -export let bindings = { +const DEFAULT_BINDINGS = { "!": { postAction: "showFlags" }, "#": { handler: "goToPost", anonymous: true }, "/": { handler: "toggleSearch", anonymous: true }, @@ -96,18 +96,21 @@ export default { // Disable the shortcut if private messages are disabled if (!siteSettings.enable_personal_messages) { - delete bindings["g m"]; + delete DEFAULT_BINDINGS["g m"]; } }, bindEvents() { - Object.keys(bindings).forEach(key => { + Object.keys(DEFAULT_BINDINGS).forEach(key => { this.bindKey(key); }); }, - bindKey(key) { - const binding = bindings[key]; + bindKey(key, binding = null) { + if (!binding) { + binding = DEFAULT_BINDINGS[key]; + } + if (!binding.anonymous && !this.currentUser) { return; } @@ -135,23 +138,28 @@ export default { }, // restore global shortcuts that you have paused - unpause(...combinations) { + unpause(combinations) { combinations.forEach(combo => this.bindKey(combo)); }, - // add bindings to the key trapper, if none is specified then - // the shortcuts will be bound globally. - addBindings(newBindings, callback) { - Object.keys(newBindings).forEach(key => { - let binding = newBindings[key]; - this.keyTrapper.bind(key, event => { - // usually the caller that is adding the binding - // will want to decide what to do with it when the - // event is fired - callback(binding, event); - event.stopPropagation(); - }); - }); + /** + * addShortcut(shortcut, callback, opts) + * + * Used to bind a keyboard shortcut, which will fire the provided + * callback when pressed. Valid options are: + * + * - global - makes the shortcut work anywhere, including when an input is focused + * - anonymous - makes the shortcut work even if a user is not logged in + * - path - a specific path to limit the shortcut to .e.g /latest + * - postAction - binds the shortcut to fire the specified post action when a + * post is selected + **/ + addShortcut(shortcut, callback, opts = {}) { + // we trim but leave whitespace between characters, as shortcuts + // like `z z` are valid for Mousetrap + shortcut = shortcut.trim(); + let newBinding = Object.assign({ handler: callback }, opts); + this.bindKey(shortcut, newBinding); }, // unbinds all the shortcuts in a key binding object e.g. diff --git a/app/assets/javascripts/discourse/lib/plugin-api.js b/app/assets/javascripts/discourse/lib/plugin-api.js index b0594f7b8db..6df49e453d2 100644 --- a/app/assets/javascripts/discourse/lib/plugin-api.js +++ b/app/assets/javascripts/discourse/lib/plugin-api.js @@ -1,4 +1,3 @@ -/*global Mousetrap:true*/ import deprecated from "discourse-common/lib/deprecated"; import { iconNode } from "discourse-common/lib/icon-library"; import { addDecorator } from "discourse/widgets/post-cooked"; @@ -52,7 +51,7 @@ import { addCategorySortCriteria } from "discourse/components/edit-category-sett import { queryRegistry } from "discourse/widgets/widget"; import Composer from "discourse/models/composer"; import { on } from "@ember/object/evented"; -import KeyboardShortcuts, { bindings } from "discourse/lib/keyboard-shortcuts"; +import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts"; // If you add any methods to the API ensure you bump up this number const PLUGIN_API_VERSION = "0.8.40"; @@ -230,12 +229,11 @@ class PluginApi { } } + /** + * See KeyboardShortcuts.addShortcut documentation. + **/ addKeyboardShortcut(shortcut, callback, opts = {}) { - shortcut = shortcut.trim().replace(/\s/g, ""); // Strip all whitespace - let newBinding = {}; - newBinding[shortcut] = Object.assign({ handler: callback }, opts); - Object.assign(bindings, newBinding); - KeyboardShortcuts.bindEvents(Mousetrap, this.container); + KeyboardShortcuts.addShortcut(shortcut, callback, opts); } /** diff --git a/test/javascripts/acceptance/plugin-keyboard-shortcut-test.js b/test/javascripts/acceptance/plugin-keyboard-shortcut-test.js new file mode 100644 index 00000000000..3541bb26b07 --- /dev/null +++ b/test/javascripts/acceptance/plugin-keyboard-shortcut-test.js @@ -0,0 +1,54 @@ +import { acceptance } from "helpers/qunit-helpers"; +import { withPluginApi } from "discourse/lib/plugin-api"; +import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts"; +import KeyboardShortcutInitializer from "discourse/initializers/keyboard-shortcuts"; + +function initKeyboardShortcuts() { + // this is here because initializers/keyboard-shortcuts is not + // firing for this acceptance test, and it needs to be fired before + // more keyboard shortcuts can be added + KeyboardShortcutInitializer.initialize(Discourse.__container__); +} + +acceptance("Plugin Keyboard Shortcuts - Logged In", { + loggedIn: true +}); + +test("a plugin can add a keyboard shortcut", async assert => { + initKeyboardShortcuts(); + withPluginApi("0.8.38", api => { + api.addKeyboardShortcut("]", () => { + $("#qunit-fixture").html( + "
Test adding plugin shortcut
" + ); + }); + }); + + await visit("/t/this-is-a-test-topic/9"); + await keyEvent(document, "keypress", "]".charCodeAt(0)); + assert.equal( + $("#added-element").length, + 1, + "the keyboard shortcut callback fires successfully" + ); +}); + +acceptance("Plugin Keyboard Shortcuts - Anonymous", { + loggedIn: false +}); + +test("a plugin can add a keyboard shortcut with an option", async assert => { + var spy = sandbox.spy(KeyboardShortcuts, "_bindToPath"); + initKeyboardShortcuts(); + withPluginApi("0.8.38", api => { + api.addKeyboardShortcut("]", () => {}, { + anonymous: true, + path: "test-path" + }); + }); + + assert.ok( + spy.calledWith("test-path", "]"), + "bindToPath is called due to options provided" + ); +}); diff --git a/test/javascripts/components/keyboard-shortcuts-test.js b/test/javascripts/components/keyboard-shortcuts-test.js index d7f60a85c04..025bec5ecf0 100644 --- a/test/javascripts/components/keyboard-shortcuts-test.js +++ b/test/javascripts/components/keyboard-shortcuts-test.js @@ -74,7 +74,7 @@ Object.keys(pathBindings).forEach(path => { var testName = binding + " goes to " + path; test(testName, function(assert) { - KeyboardShortcuts.bindEvents(testMouseTrap, Discourse.__container__); + KeyboardShortcuts.bindEvents(); testMouseTrap.trigger(binding); assert.ok(DiscourseURL.routeTo.calledWith(path)); @@ -89,7 +89,7 @@ Object.keys(clickBindings).forEach(selector => { var testName = binding + " clicks on " + selector; test(testName, function(assert) { - KeyboardShortcuts.bindEvents(testMouseTrap, Discourse.__container__); + KeyboardShortcuts.bindEvents(); $(selector).on("click", function() { assert.ok(true, selector + " was clicked"); }); @@ -109,7 +109,7 @@ Object.keys(functionBindings).forEach(func => { sandbox.stub(KeyboardShortcuts, func, function() { assert.ok(true, func + " is called when " + binding + " is triggered"); }); - KeyboardShortcuts.bindEvents(testMouseTrap, Discourse.__container__); + KeyboardShortcuts.bindEvents(); testMouseTrap.trigger(binding); });