From 7ed62abf7edbdd987fa7a983e06398f4c515aa3e Mon Sep 17 00:00:00 2001 From: Thomas Pathier Date: Tue, 20 Nov 2018 18:18:29 +0000 Subject: Resolve "The reply shortcut can add any text of the page to the "comment" text area" --- .../behaviors/shortcuts/shortcuts_issuable.js | 31 ++++- app/assets/javascripts/lib/utils/common_utils.js | 24 +++- .../54032-reply-shortcut-only-discussion-text.yml | 5 + .../behaviors/shortcuts/shortcuts_issuable_spec.js | 144 ++++++++++++++++++++- 4 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/54032-reply-shortcut-only-discussion-text.yml diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_issuable.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_issuable.js index 5e48bf5a35c..5f86fc9e63d 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_issuable.js +++ b/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); diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index e14fff7a610..3186ae9c133 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/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; diff --git a/changelogs/unreleased/54032-reply-shortcut-only-discussion-text.yml b/changelogs/unreleased/54032-reply-shortcut-only-discussion-text.yml new file mode 100644 index 00000000000..5c1f6e74b39 --- /dev/null +++ b/changelogs/unreleased/54032-reply-shortcut-only-discussion-text.yml @@ -0,0 +1,5 @@ +--- +title: Make reply shortcut only quote selected discussion text +merge_request: 23096 +author: Thomas Pathier +type: fix diff --git a/spec/javascripts/behaviors/shortcuts/shortcuts_issuable_spec.js b/spec/javascripts/behaviors/shortcuts/shortcuts_issuable_spec.js index bc25549cbed..b709b937180 100644 --- a/spec/javascripts/behaviors/shortcuts/shortcuts_issuable_spec.js +++ b/spec/javascripts/behaviors/shortcuts/shortcuts_issuable_spec.js @@ -1,3 +1,7 @@ +/* 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('

Selected text.

', 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('
Selected text.

Invalid selected text.

', 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 = `
+
Text...
+

Selected text.

+
`; + documentFragment.originalNodes = [originalNode.querySelector('em')]; + + node.innerHTML = 'Selected text.'; + + 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 = `
+
Selected text.
+

Valid text

+
`; + documentFragment.originalNodes = [originalNode.querySelector('b')]; + + node.innerHTML = 'Selected text.'; + + 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(); + }); + }); }); }); -- cgit v1.2.1