Commit bbfdfb78 authored by Phil Hughes's avatar Phil Hughes

Merge branch '54032-restrict-reply-shortcut-text' into 'master'

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

Closes #54032

See merge request gitlab-org/gitlab-ce!23096
parents ac6673dd 7ed62abf
...@@ -4,6 +4,7 @@ import _ from 'underscore'; ...@@ -4,6 +4,7 @@ import _ from 'underscore';
import Sidebar from '../../right_sidebar'; import Sidebar from '../../right_sidebar';
import Shortcuts from './shortcuts'; import Shortcuts from './shortcuts';
import { CopyAsGFM } from '../markdown/copy_as_gfm'; import { CopyAsGFM } from '../markdown/copy_as_gfm';
import { getSelectedFragment } from '~/lib/utils/common_utils';
export default class ShortcutsIssuable extends Shortcuts { export default class ShortcutsIssuable extends Shortcuts {
constructor(isMergeRequest) { constructor(isMergeRequest) {
...@@ -24,17 +25,43 @@ export default class ShortcutsIssuable extends Shortcuts { ...@@ -24,17 +25,43 @@ export default class ShortcutsIssuable extends Shortcuts {
static replyWithSelectedText() { static replyWithSelectedText() {
const $replyField = $('.js-main-target-form .js-vue-comment-form'); 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; return false;
} }
const documentFragment = getSelectedFragment(document.querySelector('.issuable-details'));
if (!documentFragment) { if (!documentFragment) {
$replyField.focus(); $replyField.focus();
return false; 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 el = CopyAsGFM.transformGFMSelection(documentFragment.cloneNode(true));
const selected = CopyAsGFM.nodeToGFM(el); const selected = CopyAsGFM.nodeToGFM(el);
......
...@@ -226,7 +226,17 @@ export const getParameterByName = (name, urlToParse) => { ...@@ -226,7 +226,17 @@ export const getParameterByName = (name, urlToParse) => {
return decodeURIComponent(results[2].replace(/\+/g, ' ')); 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; const container = range.commonAncestorContainer;
// add context to fragment if needed // add context to fragment if needed
if (container.tagName === 'OL') { if (container.tagName === 'OL') {
...@@ -237,14 +247,22 @@ const handleSelectedRange = range => { ...@@ -237,14 +247,22 @@ const handleSelectedRange = range => {
return range.cloneContents(); return range.cloneContents();
}; };
export const getSelectedFragment = () => { export const getSelectedFragment = restrictToNode => {
const selection = window.getSelection(); const selection = window.getSelection();
if (selection.rangeCount === 0) return null; 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(); const documentFragment = document.createDocumentFragment();
documentFragment.originalNodes = [];
for (let i = 0; i < selection.rangeCount; i += 1) { for (let i = 0; i < selection.rangeCount; i += 1) {
const range = selection.getRangeAt(i); 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; if (documentFragment.textContent.length === 0) return null;
......
---
title: Make reply shortcut only quote selected discussion text
merge_request: 23096
author: Thomas Pathier
type: fix
/* eslint-disable
no-underscore-dangle
*/
import $ from 'jquery'; import $ from 'jquery';
import initCopyAsGFM from '~/behaviors/markdown/copy_as_gfm'; import initCopyAsGFM from '~/behaviors/markdown/copy_as_gfm';
import ShortcutsIssuable from '~/behaviors/shortcuts/shortcuts_issuable'; import ShortcutsIssuable from '~/behaviors/shortcuts/shortcuts_issuable';
...@@ -27,13 +31,17 @@ describe('ShortcutsIssuable', function() { ...@@ -27,13 +31,17 @@ describe('ShortcutsIssuable', function() {
describe('replyWithSelectedText', () => { describe('replyWithSelectedText', () => {
// Stub window.gl.utils.getSelectedFragment to return a node with the provided HTML. // Stub window.gl.utils.getSelectedFragment to return a node with the provided HTML.
const stubSelection = html => { const stubSelection = (html, invalidNode) => {
window.gl.utils.getSelectedFragment = () => { ShortcutsIssuable.__Rewire__('getSelectedFragment', () => {
const documentFragment = document.createDocumentFragment();
const node = document.createElement('div'); const node = document.createElement('div');
node.innerHTML = html; node.innerHTML = html;
if (!invalidNode) node.className = 'md';
return node; documentFragment.appendChild(node);
}; return documentFragment;
});
}; };
describe('with empty selection', () => { describe('with empty selection', () => {
it('does not return an error', () => { it('does not return an error', () => {
...@@ -105,5 +113,133 @@ describe('ShortcutsIssuable', function() { ...@@ -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();
});
});
}); });
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment