Resolve "The reply shortcut can add any text of the page to the "comment" text area"

app/assets/javascripts/behaviors/shortcuts/shortcuts_issuable.js

@@ -4,6 +4,7 @@ import _ from 'underscore';
 import Sidebar from '../../right_sidebar';
 import Shortcuts from './shortcuts';
 import { CopyAsGFM } from '../markdown/copy_as_gfm';
+import { getSelectedFragment } from '~/lib/utils/common_utils';
 
 export default class ShortcutsIssuable extends Shortcuts {
   constructor(isMergeRequest) {
@@ -24,17 +25,43 @@ export default class ShortcutsIssuable extends Shortcuts {
 
   static replyWithSelectedText() {
     const $replyField = $('.js-main-target-form .js-vue-comment-form');
-    const documentFragment = window.gl.utils.getSelectedFragment();
 
-    if (!$replyField.length) {
+    if (!$replyField.length || $replyField.is(':hidden') /* Other tab selected in MR */) {
       return false;
     }
 
+    const documentFragment = getSelectedFragment(document.querySelector('.issuable-details'));
+
     if (!documentFragment) {
       $replyField.focus();
       return false;
     }
 
+    // Sanity check: Make sure the selected text comes from a discussion : it can either contain a message...
+    let foundMessage = !!documentFragment.querySelector('.md, .wiki');
+
+    // ... Or come from a message
+    if (!foundMessage) {
+      if (documentFragment.originalNodes) {
+        documentFragment.originalNodes.forEach(e => {
+          let node = e;
+          do {
+            // Text nodes don't define the `matches` method
+            if (node.matches && node.matches('.md, .wiki')) {
+              foundMessage = true;
+            }
+            node = node.parentNode;
+          } while (node && !foundMessage);
+        });
+      }
+
+      // If there is no message, just select the reply field
+      if (!foundMessage) {
+        $replyField.focus();
+        return false;
+      }
+    }
+
     const el = CopyAsGFM.transformGFMSelection(documentFragment.cloneNode(true));
     const selected = CopyAsGFM.nodeToGFM(el);
 

app/assets/javascripts/lib/utils/common_utils.js

@@ -226,7 +226,17 @@ export const getParameterByName = (name, urlToParse) => {
   return decodeURIComponent(results[2].replace(/\+/g, ' '));
 };
 
-const handleSelectedRange = range => {
+const handleSelectedRange = (range, restrictToNode) => {
+  // Make sure this range is within the restricting container
+  if (restrictToNode && !range.intersectsNode(restrictToNode)) return null;
+
+  // If only a part of the range is within the wanted container, we need to restrict the range to it
+  if (restrictToNode && !restrictToNode.contains(range.commonAncestorContainer)) {
+    if (!restrictToNode.contains(range.startContainer)) range.setStart(restrictToNode, 0);
+    if (!restrictToNode.contains(range.endContainer))
+      range.setEnd(restrictToNode, restrictToNode.childNodes.length);
+  }
+
   const container = range.commonAncestorContainer;
   // add context to fragment if needed
   if (container.tagName === 'OL') {
@@ -237,14 +247,22 @@ const handleSelectedRange = range => {
   return range.cloneContents();
 };
 
-export const getSelectedFragment = () => {
+export const getSelectedFragment = restrictToNode => {
   const selection = window.getSelection();
   if (selection.rangeCount === 0) return null;
+  // Most usages of the selection only want text from a part of the page (e.g. discussion)
+  if (restrictToNode && !selection.containsNode(restrictToNode, true)) return null;
+
   const documentFragment = document.createDocumentFragment();
+  documentFragment.originalNodes = [];
 
   for (let i = 0; i < selection.rangeCount; i += 1) {
     const range = selection.getRangeAt(i);
-    documentFragment.appendChild(handleSelectedRange(range));
+    const handledRange = handleSelectedRange(range, restrictToNode);
+    if (handledRange) {
+      documentFragment.appendChild(handledRange);
+      documentFragment.originalNodes.push(range.commonAncestorContainer);
+    }
   }
   if (documentFragment.textContent.length === 0) return null;
 

changelogs/unreleased/54032-reply-shortcut-only-discussion-text.yml

+---
+title: Make reply shortcut only quote selected discussion text
+merge_request: 23096
+author: Thomas Pathier
+type: fix

spec/javascripts/behaviors/shortcuts/shortcuts_issuable_spec.js

+/* eslint-disable
+  no-underscore-dangle
+*/
+
 import $ from 'jquery';
 import initCopyAsGFM from '~/behaviors/markdown/copy_as_gfm';
 import ShortcutsIssuable from '~/behaviors/shortcuts/shortcuts_issuable';
@@ -27,13 +31,17 @@ describe('ShortcutsIssuable', function() {
 
   describe('replyWithSelectedText', () => {
     // Stub window.gl.utils.getSelectedFragment to return a node with the provided HTML.
-    const stubSelection = html => {
-      window.gl.utils.getSelectedFragment = () => {
+    const stubSelection = (html, invalidNode) => {
+      ShortcutsIssuable.__Rewire__('getSelectedFragment', () => {
+        const documentFragment = document.createDocumentFragment();
         const node = document.createElement('div');
+
         node.innerHTML = html;
+        if (!invalidNode) node.className = 'md';
 
-        return node;
-      };
+        documentFragment.appendChild(node);
+        return documentFragment;
+      });
     };
     describe('with empty selection', () => {
       it('does not return an error', () => {
@@ -105,5 +113,133 @@ describe('ShortcutsIssuable', function() {
         );
       });
     });
+
+    describe('with an invalid selection', () => {
+      beforeEach(() => {
+        stubSelection('<p>Selected text.</p>', true);
+      });
+
+      it('does not add anything to the input', () => {
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect($(FORM_SELECTOR).val()).toBe('');
+      });
+
+      it('triggers `focus`', () => {
+        const spy = spyOn(document.querySelector(FORM_SELECTOR), 'focus');
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(spy).toHaveBeenCalled();
+      });
+    });
+
+    describe('with a semi-valid selection', () => {
+      beforeEach(() => {
+        stubSelection('<div class="md">Selected text.</div><p>Invalid selected text.</p>', true);
+      });
+
+      it('only adds the valid part to the input', () => {
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect($(FORM_SELECTOR).val()).toBe('> Selected text.\n\n');
+      });
+
+      it('triggers `focus`', () => {
+        const spy = spyOn(document.querySelector(FORM_SELECTOR), 'focus');
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(spy).toHaveBeenCalled();
+      });
+
+      it('triggers `input`', () => {
+        let triggered = false;
+        $(FORM_SELECTOR).on('input', () => {
+          triggered = true;
+        });
+
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(triggered).toBe(true);
+      });
+    });
+
+    describe('with a selection in a valid block', () => {
+      beforeEach(() => {
+        ShortcutsIssuable.__Rewire__('getSelectedFragment', () => {
+          const documentFragment = document.createDocumentFragment();
+          const node = document.createElement('div');
+          const originalNode = document.createElement('body');
+          originalNode.innerHTML = `<div class="issue">
+            <div class="otherElem">Text...</div>
+            <div class="md"><p><em>Selected text.</em></p></div>
+          </div>`;
+          documentFragment.originalNodes = [originalNode.querySelector('em')];
+
+          node.innerHTML = '<em>Selected text.</em>';
+
+          documentFragment.appendChild(node);
+
+          return documentFragment;
+        });
+      });
+
+      it('adds the quoted selection to the input', () => {
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect($(FORM_SELECTOR).val()).toBe('> _Selected text._\n\n');
+      });
+
+      it('triggers `focus`', () => {
+        const spy = spyOn(document.querySelector(FORM_SELECTOR), 'focus');
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(spy).toHaveBeenCalled();
+      });
+
+      it('triggers `input`', () => {
+        let triggered = false;
+        $(FORM_SELECTOR).on('input', () => {
+          triggered = true;
+        });
+
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(triggered).toBe(true);
+      });
+    });
+
+    describe('with a selection in an invalid block', () => {
+      beforeEach(() => {
+        ShortcutsIssuable.__Rewire__('getSelectedFragment', () => {
+          const documentFragment = document.createDocumentFragment();
+          const node = document.createElement('div');
+          const originalNode = document.createElement('body');
+          originalNode.innerHTML = `<div class="issue">
+            <div class="otherElem"><div><b>Selected text.</b></div></div>
+            <div class="md"><p><em>Valid text</em></p></div>
+          </div>`;
+          documentFragment.originalNodes = [originalNode.querySelector('b')];
+
+          node.innerHTML = '<b>Selected text.</b>';
+
+          documentFragment.appendChild(node);
+
+          return documentFragment;
+        });
+      });
+
+      it('does not add anything to the input', () => {
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect($(FORM_SELECTOR).val()).toBe('');
+      });
+
+      it('triggers `focus`', () => {
+        const spy = spyOn(document.querySelector(FORM_SELECTOR), 'focus');
+        ShortcutsIssuable.replyWithSelectedText(true);
+
+        expect(spy).toHaveBeenCalled();
+      });
+    });
   });
 });