FIX: Improves bookmark shortcut reliability and other minor issues (#9547)

This commit reworks slightly the `toggleBookmark` and `toggleBookmarkTopic` functions.

- Pressing [f] (toggleBookmarkTopic)
  - a topic list item is selected, we attempt to toggle the related topic
  - a post is selected, we bookmark the current topic
  - nothing is selected, if there's a currentTopic we bookmark it

- Pressing [b] (toggleBookmark)
  - a post is selected, we bookmark it
  - a topic list item is selected, we attempt to toggle the related topic
  - nothing is selected, if there's a currentTopic we bookmark it

Note this, commit also reduces jquery usage, a bug where the [f] shortcut was propagated to the modal input, and fixes bug when bookmarking a topic list item on the front page and the firstPost couldn't be found.

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

@@ -83,6 +83,11 @@ const DEFAULT_BINDINGS = {
 
 const animationDuration = 100;
 
+function preventKeyboardEvent(event) {
+  event.preventDefault();
+  event.stopPropagation();
+}
+
 export default {
   init(keyTrapper, container) {
     this.keyTrapper = keyTrapper;
@@ -175,20 +180,39 @@ export default {
   },
 
   toggleBookmark(event) {
-    event.preventDefault();
+    const selectedPost = this._getSelectedPost();
-    event.stopPropagation();
+    if (selectedPost) {
+      preventKeyboardEvent(event);
+      this.sendToSelectedPost("toggleBookmark", selectedPost);
+      return;
+    }
 
-    this.sendToSelectedPost("toggleBookmark");
+    const selectedTopicListItem = this._getSelectedTopicListItem();
-    this.sendToTopicListItemView("toggleBookmark");
+    if (selectedTopicListItem) {
+      preventKeyboardEvent(event);
+      this.sendToTopicListItemView("toggleBookmark", selectedTopicListItem);
+      return;
+    }
+
+    this._bookmarkCurrentTopic(event);
   },
 
-  toggleBookmarkTopic() {
+  toggleBookmarkTopic(event) {
+    const selectedTopicListItem = this._getSelectedTopicListItem();
+    if (selectedTopicListItem) {
+      preventKeyboardEvent(event);
+      this.sendToTopicListItemView("toggleBookmark", selectedTopicListItem);
+      return;
+    }
+
+    this._bookmarkCurrentTopic(event);
+  },
+
+  _bookmarkCurrentTopic(event) {
     const topic = this.currentTopic();
-    // BIG hack, need a cleaner way
+    if (topic && document.querySelectorAll(".posts-wrapper").length) {
-    if (topic && $(".posts-wrapper").length > 0) {
+      preventKeyboardEvent(event);
       this.container.lookup("controller:topic").send("toggleBookmark");
-    } else {
-      this.sendToTopicListItemView("toggleBookmark");
     }
   },
 
@@ -380,8 +404,8 @@ export default {
     });
   },
 
-  sendToTopicListItemView(action) {
+  sendToTopicListItemView(action, elem) {
-    const elem = $("tr.selected.topic-list-item.ember-view")[0];
+    elem = elem || document.querySelector("tr.selected.topic-list-item");
     if (elem) {
       const registry = this.container.lookup("-view-registry:main");
       if (registry) {
@@ -401,15 +425,18 @@ export default {
     }
   },
 
-  sendToSelectedPost(action) {
+  sendToSelectedPost(action, elem) {
-    const container = this.container;
     // TODO: We should keep track of the post without a CSS class
-    let selectedPostId = parseInt(
+    const selectedPost =
-      $(".topic-post.selected article.boxed").data("post-id"),
+      elem || document.querySelector(".topic-post.selected article.boxed");
-      10
+
-    );
+    let selectedPostId;
+    if (selectedPost) {
+      selectedPostId = parseInt(selectedPost.dataset.postId, 10);
+    }
+
     if (selectedPostId) {
-      const topicController = container.lookup("controller:topic");
+      const topicController = this.container.lookup("controller:topic");
       const post = topicController
         .get("model.postStream.posts")
         .findBy("id", selectedPostId);
@@ -418,7 +445,7 @@ export default {
 
         let actionMethod = topicController.actions[action];
         if (!actionMethod) {
-          const topicRoute = container.lookup("route:topic");
+          const topicRoute = this.container.lookup("route:topic");
           actionMethod = topicRoute.actions[action];
         }
 
@@ -696,6 +723,14 @@ export default {
     this.container.lookup("controller:topic").send("replyToPost");
   },
 
+  _getSelectedPost() {
+    return document.querySelector(".topic-post.selected article[data-post-id]");
+  },
+
+  _getSelectedTopicListItem() {
+    return document.querySelector("tr.selected.topic-list-item");
+  },
+
   deferTopic() {
     this.container.lookup("controller:topic").send("deferTopic");
   },

app/assets/javascripts/discourse/app/models/topic.js

@@ -410,19 +410,22 @@ const Topic = RestModel.extend({
     const postStream = this.postStream;
     let firstPost = postStream.get("posts.firstObject");
 
-    if (firstPost.post_number === 1) {
+    if (firstPost && firstPost.post_number === 1) {
       return Promise.resolve(firstPost);
     }
 
     const postId = postStream.findPostIdForPostNumber(1);
+    if (postId) {
+      // try loading from identity map first
+      firstPost = postStream.findLoadedPost(postId);
+      if (firstPost) {
+        return Promise.resolve(firstPost);
+      }
 
-    // try loading from identity map first
+      return this.postStream.loadPost(postId);
-    firstPost = postStream.findLoadedPost(postId);
+    } else {
-    if (firstPost) {
+      return this.postStream.loadPostByPostNumber(1);
-      return Promise.resolve(firstPost);
     }
-
-    return this.postStream.loadPost(postId);
   },
 
   toggleBookmark() {