Add key unbinds to DiscussionKeyboardNavigatorfe-add-unbinds-to-discussion-keyboard-navigator

Also adds comment to explain why this works on the diff-tab
when it's only used in `notes-app`.

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30144#note_204066538

3 files changed, 35 insertions, 0 deletions

diff --git a/app/assets/javascripts/mr_notes/init_notes.js b/app/assets/javascripts/mr_notes/init_notes.js
index 8caac68e0d4..622db360d1f 100644
--- a/app/assets/javascripts/mr_notes/init_notes.js
+++ b/app/assets/javascripts/mr_notes/init_notes.js
@@ -59,6 +59,10 @@ export default () => {
     render(createElement) {
       const isDiffView = this.activeTab === 'diffs';
 
+      // NOTE: Even though `discussionKeyboardNavigator` is added to the `notes-app`,
+      // it adds a global key listener so it works on the diffs tab as well.
+      // If we create a single Vue app for all of the MR tabs, we should move this
+      // up the tree, to the root.
       return createElement(discussionKeyboardNavigator, { props: { isDiffView } }, [
         createElement('notes-app', {
           props: {
diff --git a/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue b/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue
index 5fc2b6ba04c..7fbfe8eebb2 100644
--- a/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue
+++ b/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue
@@ -25,6 +25,10 @@ export default {
     Mousetrap.bind('n', () => this.jumpToNextDiscussion());
     Mousetrap.bind('p', () => this.jumpToPreviousDiscussion());
   },
+  beforeDestroy() {
+    Mousetrap.unbind('n');
+    Mousetrap.unbind('p');
+  },
   methods: {
     ...mapActions(['expandDiscussion']),
     jumpToNextDiscussion() {
diff --git a/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js
index 6d50713999d..8881bedf3cc 100644
--- a/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js
+++ b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js
@@ -74,4 +74,31 @@ describe('notes/components/discussion_keyboard_navigator', () => {
       expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
     });
   });
+
+  describe('on destroy', () => {
+    beforeEach(() => {
+      jest.spyOn(Mousetrap, 'unbind');
+
+      createComponent();
+
+      wrapper.destroy();
+    });
+
+    it('unbinds keys', () => {
+      expect(Mousetrap.unbind).toHaveBeenCalledWith('n');
+      expect(Mousetrap.unbind).toHaveBeenCalledWith('p');
+    });
+
+    it('does not call jumpToNextDiscussion when pressing `n`', () => {
+      Mousetrap.trigger('n');
+
+      expect(wrapper.vm.jumpToDiscussion).not.toHaveBeenCalled();
+    });
+
+    it('does not call jumpToNextDiscussion when pressing `p`', () => {
+      Mousetrap.trigger('p');
+
+      expect(wrapper.vm.jumpToDiscussion).not.toHaveBeenCalled();
+    });
+  });
 });