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.

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

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.

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);
   }
 
   /**

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(
+        "<div id='added-element'>Test adding plugin shortcut</div>"
+      );
+    });
+  });
+
+  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"
+  );
+});

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);
   });