FIX: Isolate modal and global key-binds (#12477)

This change makes is so that when a time-picking modal (e.g.  "Add bookmark" modal) is visible, **all** global key bindings are paused.

1. Fixes an issue where opening and closing a time-picking modal would break global single-key keybinds, so for example, <kbd>L</kbd> would no longer like posts, but <kbd>L</kbd> <kbd>L</kbd> would
2. Fixes a related issue, where doing the above would also override custom keybinds provided by plugins (e.g. <kbd>L</kbd> shortcut that discourse-reactions uses)

Included:

* DEV: Reset Mousetraps instead of unbinding
* FIX: Make unbind use unbind
* DEV: Don't check for keyTrapper twice
* DEV: Use an instance of Mousetrap
* DEV: Remove an invalid `for` attribute (`set_reminder` doesn't exist)
* DEV: Add ability to pause all KeyboardShortcuts
* FIX: Pause all keybinds when in a time-picking modal
* DEV: Move bookmark keybind resets to willDestroyElement
* DEV: Fix shortcuts-related tests

app/assets/javascripts/discourse/app/components/bookmark.js

@@ -7,14 +7,13 @@ import {
   startOfDay,
   tomorrow,
 } from "discourse/lib/time-utils";
-
 import { AUTO_DELETE_PREFERENCES } from "discourse/models/bookmark";
 import Component from "@ember/component";
 import I18n from "I18n";
 import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
+import Mousetrap from "mousetrap";
 import { Promise } from "rsvp";
 import { TIME_SHORTCUT_TYPES } from "discourse/lib/time-shortcut";
-
 import { action } from "@ember/object";
 import { ajax } from "discourse/lib/ajax";
 import bootbox from "bootbox";
@@ -24,11 +23,6 @@ import { and, notEmpty } from "@ember/object/computed";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { later } from "@ember/runloop";
 
-// global shortcuts that interfere with these modal shortcuts, they are rebound when the
-// modal is closed
-//
-// d deletePost
-const GLOBAL_SHORTCUTS_TO_PAUSE = ["d"];
 const BOOKMARK_BINDINGS = {
   enter: { handler: "saveAndClose" },
   "d d": { handler: "delete" },
@@ -127,26 +121,18 @@ export default Component.extend({
   },
 
   _bindKeyboardShortcuts() {
-    KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE);
+    KeyboardShortcuts.pause();
+
+    this._mousetrap = new Mousetrap();
     Object.keys(BOOKMARK_BINDINGS).forEach((shortcut) => {
-      KeyboardShortcuts.addShortcut(shortcut, () => {
+      this._mousetrap.bind(shortcut, () => {
         let binding = BOOKMARK_BINDINGS[shortcut];
-        if (binding.args) {
-          return this.send(binding.handler, ...binding.args);
-        }
         this.send(binding.handler);
+        return false;
       });
     });
   },
 
-  _unbindKeyboardShortcuts() {
-    KeyboardShortcuts.unbind(BOOKMARK_BINDINGS);
-  },
-
-  _restoreGlobalShortcuts() {
-    KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);
-  },
-
   _loadPostLocalDates() {
     let postEl = document.querySelector(
       `[data-post-id="${this.model.postId}"]`
@@ -270,9 +256,6 @@ export default Component.extend({
     this._closeWithoutSaving =
       this._closeWithoutSaving || initiatedByCloseButton;
 
-    this._unbindKeyboardShortcuts();
-    this._restoreGlobalShortcuts();
-
     if (!this._closeWithoutSaving && !this._savingBookmarkManually) {
       this._saveBookmark().catch((e) => this._handleSaveError(e));
     }
@@ -281,6 +264,12 @@ export default Component.extend({
     }
   },
 
+  willDestroyElement() {
+    this._super(...arguments);
+    this._mousetrap.reset();
+    KeyboardShortcuts.unpause();
+  },
+
   showExistingReminderAt: notEmpty("model.reminderAt"),
   showDelete: notEmpty("model.id"),
   userHasTimezoneSet: notEmpty("userTimezone"),

app/assets/javascripts/discourse/app/components/d-editor.js

@@ -274,12 +274,14 @@ export default Component.extend({
 
     scheduleOnce("afterRender", this, this._readyNow);
 
-    const mouseTrap = Mousetrap(this.element.querySelector(".d-editor-input"));
+    this._mouseTrap = new Mousetrap(
+      this.element.querySelector(".d-editor-input")
+    );
     const shortcuts = this.get("toolbar.shortcuts");
 
     Object.keys(shortcuts).forEach((sc) => {
       const button = shortcuts[sc];
-      mouseTrap.bind(sc, () => {
+      this._mouseTrap.bind(sc, () => {
         button.action(button);
         return false;
       });
@@ -317,7 +319,6 @@ export default Component.extend({
       this.appEvents.on("composer:insert-text", this, "_insertText");
       this.appEvents.on("composer:replace-text", this, "_replaceText");
     }
-    this._mouseTrap = mouseTrap;
 
     if (isTesting()) {
       this.element.addEventListener("paste", this.paste.bind(this));
@@ -340,10 +341,7 @@ export default Component.extend({
       this.appEvents.off("composer:replace-text", this, "_replaceText");
     }
 
-    const mouseTrap = this._mouseTrap;
-    Object.keys(this.get("toolbar.shortcuts")).forEach((sc) =>
-      mouseTrap.unbind(sc)
-    );
+    this._mouseTrap.reset();
     $(this.element.querySelector(".d-editor-preview")).off("click.preview");
 
     if (isTesting()) {

app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js

@@ -8,13 +8,15 @@ import {
   PUBLISH_TO_CATEGORY_STATUS_TYPE,
 } from "discourse/controllers/edit-topic-timer";
 import { FORMAT } from "select-kit/components/future-date-input-selector";
-import discourseComputed, { on } from "discourse-common/utils/decorators";
+import discourseComputed from "discourse-common/utils/decorators";
 import { equal, or, readOnly } from "@ember/object/computed";
 import I18n from "I18n";
 import { action } from "@ember/object";
 import Component from "@ember/component";
 import { isEmpty } from "@ember/utils";
 import { now, startOfDay, thisWeekend } from "discourse/lib/time-utils";
+import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
+import Mousetrap from "mousetrap";
 
 export default Component.extend({
   statusType: readOnly("topicTimer.status_type"),
@@ -37,18 +39,31 @@ export default Component.extend({
   ),
   duration: null,
 
-  @on("init")
-  preloadDuration() {
+  init() {
+    this._super(...arguments);
+
+    KeyboardShortcuts.pause();
+    this._mousetrap = new Mousetrap();
+
+    this.set("duration", this.initialDuration);
+  },
+
+  get initialDuration() {
     if (!this.useDuration || !this.topicTimer.duration_minutes) {
-      return;
-    }
-    if (this.durationType === "days") {
-      this.set("duration", this.topicTimer.duration_minutes / 60 / 24);
+      return null;
+    } else if (this.durationType === "days") {
+      return this.topicTimer.duration_minutes / 60 / 24;
     } else {
-      this.set("duration", this.topicTimer.duration_minutes / 60);
+      return this.topicTimer.duration_minutes / 60;
     }
   },
 
+  willDestroyElement() {
+    this._super(...arguments);
+    this._mousetrap.reset();
+    KeyboardShortcuts.unpause();
+  },
+
   @discourseComputed("autoDeleteReplies")
   durationType(autoDeleteReplies) {
     return autoDeleteReplies ? "days" : "hours";

app/assets/javascripts/discourse/app/components/site-header.js

@@ -251,8 +251,8 @@ const SiteHeaderComponent = MountWidget.extend(
       }
 
       const header = document.querySelector("header.d-header");
-      const mousetrap = new Mousetrap(header);
-      mousetrap.bind(["right", "left"], (e) => {
+      this._mousetrap = new Mousetrap(header);
+      this._mousetrap.bind(["right", "left"], (e) => {
         const activeTab = document.querySelector(".glyphs .menu-link.active");
 
         if (activeTab) {
@@ -267,8 +267,6 @@ const SiteHeaderComponent = MountWidget.extend(
           });
         }
       });
-
-      this.set("_mousetrap", mousetrap);
     },
 
     _cleanDom() {
@@ -290,7 +288,7 @@ const SiteHeaderComponent = MountWidget.extend(
       cancel(this._scheduledRemoveAnimate);
       window.cancelAnimationFrame(this._scheduledMovingAnimation);
 
-      this._mousetrap.unbind(["right", "left"]);
+      this._mousetrap.reset();
 
       document.removeEventListener("click", this._dismissFirstNotification);
     },

app/assets/javascripts/discourse/app/components/time-shortcut-picker.js

@@ -16,18 +16,9 @@ import discourseComputed, {
 
 import Component from "@ember/component";
 import I18n from "I18n";
-import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
 import { action } from "@ember/object";
 import { and, equal } from "@ember/object/computed";
 
-// global shortcuts that interfere with these modal shortcuts, they are rebound when the
-// component is destroyed
-//
-// c createTopic
-// r replyToPost
-// l toggle like
-// t replyAsNewTopic
-const GLOBAL_SHORTCUTS_TO_PAUSE = ["c", "r", "l", "t"];
 const BINDINGS = {
   "l t": {
     handler: "selectShortcut",
@@ -113,10 +104,9 @@ export default Component.extend({
     }
   },
 
-  @on("willDestroyElement")
-  _resetKeyboardShortcuts() {
-    KeyboardShortcuts.unbind(BINDINGS);
-    KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);
+  willDestroyElement() {
+    this._super(...arguments);
+    this.mousetrap.unbind(Object.keys(BINDINGS));
   },
 
   parsePrefilledDatetime() {
@@ -157,14 +147,11 @@ export default Component.extend({
   },
 
   _bindKeyboardShortcuts() {
-    KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE);
     Object.keys(BINDINGS).forEach((shortcut) => {
-      KeyboardShortcuts.addShortcut(shortcut, () => {
+      this.mousetrap.bind(shortcut, () => {
         let binding = BINDINGS[shortcut];
-        if (binding.args) {
-          return this.send(binding.handler, ...binding.args);
-        }
-        this.send(binding.handler);
+        this.send(binding.handler, ...binding.args);
+        return false;
       });
     });
   },

app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js

@@ -100,7 +100,7 @@ function preventKeyboardEvent(event) {
 
 export default {
   init(keyTrapper, container) {
-    this.keyTrapper = keyTrapper;
+    this.keyTrapper = new keyTrapper();
     this.container = container;
     this._stopCallback();
 
@@ -168,6 +168,12 @@ export default {
     if (this.isTornDown()) {
       return;
     }
+
+    if (!combinations) {
+      this.keyTrapper.paused = true;
+      return;
+    }
+
     combinations.forEach((combo) => this.keyTrapper.unbind(combo));
   },
 
@@ -176,10 +182,12 @@ export default {
     if (this.isTornDown()) {
       return;
     }
-    // if the keytrapper has already been torn down this will error
-    if (this.keyTrapper == null) {
+
+    if (!combinations) {
+      this.keyTrapper.paused = false;
       return;
     }
+
     combinations.forEach((combo) => this.bindKey(combo));
   },
 
@@ -210,7 +218,7 @@ export default {
   //   'c': createTopic
   // }
   unbind(combinations) {
-    this.pause(Object.keys(combinations));
+    Object.keys(combinations).forEach((combo) => this.keyTrapper.unbind(combo));
   },
 
   toggleBookmark(event) {
@@ -758,20 +766,21 @@ export default {
   },
 
   _stopCallback() {
-    const oldStopCallback = this.keyTrapper.prototype.stopCallback;
+    const prototype = Object.getPrototypeOf(this.keyTrapper);
+    const oldStopCallback = prototype.stopCallback;
+
+    prototype.stopCallback = function (e, element, combo, sequence) {
+      if (this.paused) {
+        return true;
+      }
 
-    this.keyTrapper.prototype.stopCallback = function (
-      e,
-      element,
-      combo,
-      sequence
-    ) {
       if (
         (combo === "ctrl+f" || combo === "command+f") &&
         element.id === "search-term"
       ) {
         return false;
       }
+
       return oldStopCallback.call(this, e, element, combo, sequence);
     };
   },

app/assets/javascripts/discourse/app/templates/components/bookmark.hbs

@@ -30,12 +30,18 @@
   {{/if}}
 
   <div class="control-group">
-    <label class="control-label" for="set_reminder">
+    <label class="control-label">
       {{i18n "post.bookmarks.set_reminder"}}
     </label>
 
     {{#if userHasTimezoneSet}}
-      {{time-shortcut-picker prefilledDatetime=prefilledDatetime onTimeSelected=(action "onTimeSelected") customOptions=customTimeShortcutOptions additionalOptionsToShow=additionalTimeShortcutOptions}}
+      {{time-shortcut-picker
+        prefilledDatetime=prefilledDatetime
+        onTimeSelected=(action "onTimeSelected")
+        customOptions=customTimeShortcutOptions
+        additionalOptionsToShow=additionalTimeShortcutOptions
+        mousetrap=_mousetrap
+      }}
     {{else}}
       <div class="alert alert-info">{{html-safe (i18n "bookmarks.no_timezone" basePath=(base-path))}}</div>
     {{/if}}

app/assets/javascripts/discourse/app/templates/components/edit-topic-timer-form.hbs

@@ -19,7 +19,13 @@
   {{/if}}
   {{#if showFutureDateInput}}
     <label class="control-label">{{i18n "topic.topic_status_update.when"}}</label>
-    {{time-shortcut-picker prefilledDatetime=topicTimer.execute_at onTimeSelected=onTimeSelected customOptions=customTimeShortcutOptions hiddenOptions=hiddenTimeShortcutOptions}}
+    {{time-shortcut-picker
+      prefilledDatetime=topicTimer.execute_at
+      onTimeSelected=onTimeSelected
+      customOptions=customTimeShortcutOptions
+      hiddenOptions=hiddenTimeShortcutOptions
+      mousetrap=_mousetrap
+    }}
   {{/if}}
   {{#if useDuration}}
     <div class="controls">
@@ -45,4 +51,3 @@
     </div>
   {{/if}}
 </form>
-

app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js

@@ -4,12 +4,11 @@ import {
   loggedInUser,
   queryAll,
 } from "discourse/tests/helpers/qunit-helpers";
-import { click, fillIn, getApplication, visit } from "@ember/test-helpers";
+import { click, fillIn, visit } from "@ember/test-helpers";
 import I18n from "I18n";
 import selectKit from "discourse/tests/helpers/select-kit-helper";
 import { test } from "qunit";
 import topicFixtures from "discourse/tests/fixtures/topic";
-import KeyboardShortcutInitializer from "discourse/initializers/keyboard-shortcuts";
 
 async function openBookmarkModal(postNumber = 1) {
   if (exists(`#post_${postNumber} button.show-more-actions`)) {
@@ -27,7 +26,6 @@ acceptance("Bookmarking", function (needs) {
   let steps = [];
 
   needs.hooks.beforeEach(function () {
-    KeyboardShortcutInitializer.initialize(getApplication());
     steps = [];
   });
 

app/assets/javascripts/discourse/tests/acceptance/plugin-keyboard-shortcut-test.js

@@ -1,5 +1,4 @@
-import { getApplication, triggerKeyEvent, visit } from "@ember/test-helpers";
-import KeyboardShortcutInitializer from "discourse/initializers/keyboard-shortcuts";
+import { triggerKeyEvent, visit } from "@ember/test-helpers";
 import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
 import { acceptance } from "discourse/tests/helpers/qunit-helpers";
 import sinon from "sinon";
@@ -8,10 +7,11 @@ import { withPluginApi } from "discourse/lib/plugin-api";
 
 acceptance("Plugin Keyboard Shortcuts - Logged In", function (needs) {
   needs.user();
-  needs.hooks.beforeEach(function () {
-    KeyboardShortcutInitializer.initialize(getApplication());
-  });
+
   test("a plugin can add a keyboard shortcut", async function (assert) {
+    // Initialize the app (required in the legacy testing env)
+    await visit("/");
+
     withPluginApi("0.8.38", (api) => {
       api.addKeyboardShortcut("]", () => {
         $("#qunit-fixture").html(
@@ -30,11 +30,11 @@ acceptance("Plugin Keyboard Shortcuts - Logged In", function (needs) {
   });
 });
 
-acceptance("Plugin Keyboard Shortcuts - Anonymous", function (needs) {
-  needs.hooks.beforeEach(function () {
-    KeyboardShortcutInitializer.initialize(getApplication());
-  });
+acceptance("Plugin Keyboard Shortcuts - Anonymous", function () {
   test("a plugin can add a keyboard shortcut with an option", async function (assert) {
+    // Initialize the app (required in the legacy testing env)
+    await visit("/");
+
     let spy = sinon.spy(KeyboardShortcuts, "_bindToPath");
     withPluginApi("0.8.38", (api) => {
       api.addKeyboardShortcut("]", () => {}, {

app/assets/javascripts/discourse/tests/integration/components/bookmark-test.js

@@ -6,8 +6,6 @@ import {
   fakeTime,
   query,
 } from "discourse/tests/helpers/qunit-helpers";
-import KeyboardShortcutInitializer from "discourse/initializers/keyboard-shortcuts";
-import { getApplication } from "@ember/test-helpers";
 import sinon from "sinon";
 
 let clock = null;
@@ -23,7 +21,6 @@ discourseModule("Integration | Component | bookmark", function (hooks) {
     '{{bookmark model=model afterSave=afterSave afterDelete=afterDelete onCloseWithoutSaving=onCloseWithoutSaving registerOnCloseHandler=(action "registerOnCloseHandler") closeModal=(action "closeModal")}}';
 
   hooks.beforeEach(function () {
-    KeyboardShortcutInitializer.initialize(getApplication());
     this.actions.registerOnCloseHandler = () => {};
     this.actions.closeModal = () => {};
     this.setProperties({