From 1ba850e5947176b7e2fee0407e22b1f87045aa86 Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Mon, 28 Nov 2016 15:16:21 -0800 Subject: [PATCH] Fix comment copy logic The addition of formatting in comments broke a variety of things having to do with the copying logic. This change updates the logic and tests to reflect the new DOM. This issue arose because of a lack of integration tests for copying and selection. That test is coming in a descendant change. Bug: Issue 4969 Change-Id: I4e1994ab07947506c77b07877a46a9369d666d50 --- .../behaviors/keyboard-shortcut-behavior.html | 11 +++++++- .../gr-diff-selection/gr-diff-selection.js | 27 ++++++++++--------- .../gr-diff-selection_test.html | 23 ++++++++-------- .../diff/gr-diff-view/gr-diff-view.js | 1 + .../gr-ranged-comment-layer.js | 2 +- .../gr-selection-action-box.js | 1 + 6 files changed, 39 insertions(+), 26 deletions(-) diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html index ba7d3de1c7..f2c6ab81c1 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html @@ -20,9 +20,18 @@ limitations under the License. (function(window) { 'use strict'; + var getKeyboardEvent = function(e) { + return Polymer.dom(e.detail ? e.detail.keyboardEvent : e).event; + }; + var KeyboardShortcutBehaviorImpl = { + modifierPressed: function(e) { + e = getKeyboardEvent(e); + return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey; + }, + shouldSuppressKeyboardShortcut: function(e) { - e = Polymer.dom(e.detail ? e.detail.keyboardEvent : e); + e = getKeyboardEvent(e); if (e.path[0].tagName === 'INPUT' || e.path[0].tagName === 'TEXTAREA') { return true; } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js index 24887e02c7..6ba3ff83e4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js @@ -59,7 +59,7 @@ return; } var commentSelected = - e.target.parentNode.classList.contains('gr-diff-comment'); + this._elementDescendedFromClass(e.target, 'gr-diff-comment'); var side = this.diffBuilder.getSideByLineEl(lineEl); var targetClasses = []; targetClasses.push(side === 'left' ? @@ -215,15 +215,6 @@ _getCommentLines: function(sel, side) { var range = sel.getRangeAt(0); var content = []; - // Fall back to default copy behavior if the selection lies within one - // comment body. - if (range.startContainer === range.endContainer) { - return; - } - if (this._elementDescendedFromClass(range.commonAncestorContainer, - 'message')) { - return; - } // Query the diffElement for comments. var messages = this.diffBuilder.diffElement.querySelectorAll( '.side-by-side [data-side="' + side + @@ -233,15 +224,25 @@ var el = messages[i]; // Check if the comment element exists inside the selection. if (sel.containsNode(el, true)) { - content.push(el.textContent); + // Padded elements require newlines for accurate spacing. + if (el.parentElement.id === 'container' || + el.parentElement.nodeName === 'BLOCKQUOTE') { + if (content.length && content[content.length - 1] !== '') { + content.push(''); + } + } + + if (!el.children.length) { + content.push(el.textContent); + } } } - // Deal with offsets. - content[0] = content[0].substring(range.startOffset); + if (range.endOffset) { content[content.length - 1] = content[content.length - 1].substring(0, range.endOffset); } + content[0] = content[0].substring(range.startOffset); return content.join('\n'); }, }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html index d6a6298525..a5c26e198a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html @@ -34,8 +34,8 @@ limitations under the License.
ba ba
-
- This is a comment +
+ This is a comment
@@ -55,8 +55,8 @@ limitations under the License.
more more more
-
- This is a comment on the right +
+ This is a comment on the right
@@ -68,8 +68,8 @@ limitations under the License.
ga ga
-
- This is a different comment +
+ This is a different comment
@@ -213,13 +213,13 @@ limitations under the License. element.classList.remove('selected-right'); var selection = window.getSelection(); + selection.removeAllRanges(); var range = document.createRange(); range.setStart(element.querySelector('div.contentText').firstChild, 3); range.setEnd( element.querySelectorAll('div.contentText')[4].firstChild, 2); selection.addRange(range); assert.equal(element._getSelectedText('left'), 'ba\nzin\nga'); - selection.removeAllRanges(); }); test('copies comments', function() { @@ -227,14 +227,15 @@ limitations under the License. element.classList.add('selected-comment'); element.classList.remove('selected-right'); var selection = window.getSelection(); + selection.removeAllRanges(); var range = document.createRange(); - range.setStart(element.querySelector('.message *').firstChild, 3); + range.setStart( + element.querySelector('.gr-formatted-text *').firstChild, 3); range.setEnd( - element.querySelectorAll('.message *')[2].firstChild, 16); + element.querySelectorAll('.gr-formatted-text *')[2].firstChild, 16); selection.addRange(range); assert.equal('s is a comment\nThis is a differ', element._getSelectedText('left', true)); - selection.removeAllRanges(); }); test('defers to default behavior for textarea', function() { @@ -257,6 +258,7 @@ limitations under the License. element.classList.remove('selected-left'); var selection = window.getSelection(); + selection.removeAllRanges(); var range = document.createRange(); range.setStart( element.querySelectorAll('div.contentText')[1].firstChild, 4); @@ -264,7 +266,6 @@ limitations under the License. element.querySelectorAll('div.contentText')[1].firstChild, 10); selection.addRange(range); assert.equal(element._getSelectedText('right'), ' other'); - selection.removeAllRanges(); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index 1d63b47123..f8f646a4e8 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -241,6 +241,7 @@ _handleCKey: function(e) { if (this.shouldSuppressKeyboardShortcut(e)) { return; } if (this.$.diff.isRangeSelected()) { return; } + if (this.modifierPressed(e)) { return; } e.preventDefault(); var line = this.$.cursor.getTargetLineElement(); diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js index 7496e5951b..90c37cdf25 100644 --- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js +++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js @@ -32,7 +32,7 @@ _commentMap: { type: Object, value: function() { return {left: [], right: []}; }, - } + }, }, observers: [ diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js index c6100737e5..3046534826 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js @@ -80,6 +80,7 @@ _handleCKey: function(e) { if (this.shouldSuppressKeyboardShortcut(e)) { return; } + if (this.modifierPressed(e)) { return; } e.preventDefault(); this._fireCreateComment();