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

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, L would no longer like posts, but L L would
  2. Fixes a related issue, where doing the above would also override custom keybinds provided by plugins (e.g. L 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
diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js
index 7d05dd7..ccd790e 100644
--- a/app/assets/javascripts/discourse/app/components/bookmark.js
+++ b/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"),
diff --git a/app/assets/javascripts/discourse/app/components/d-editor.js b/app/assets/javascripts/discourse/app/components/d-editor.js
index 83d9e24..a0c9d2d 100644
--- a/app/assets/javascripts/discourse/app/components/d-editor.js
+++ b/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()) {
diff --git a/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js b/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js
index d766575..cb3ad39 100644
--- a/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js
+++ b/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";
diff --git a/app/assets/javascripts/discourse/app/components/site-header.js b/app/assets/javascripts/discourse/app/components/site-header.js
index 487fa1c..eac5366 100644
--- a/app/assets/javascripts/discourse/app/components/site-header.js
+++ b/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();
 

[... diff too long, it was truncated ...]

GitHub sha: f0b2e77a

This commit appears in #12477 which was approved by pmusaraj. It was merged by CvX.