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

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;
       }

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)) {
+          // 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');
     },
   });

polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html

@@ -34,8 +34,8 @@ limitations under the License.
             <div class="contentText" data-side="left">ba ba</div>
             <div data-side="left">
               <div class="gr-diff-comment-thread">
-                <div class="message">
+                <div class="gr-formatted-text message">
-                  <span>This is a comment</span>
+                  <span class="gr-linked-text">This is a comment</span>
                 </div>
               </div>
             </div>
@@ -55,8 +55,8 @@ limitations under the License.
             <div class="contentText" data-side="right">more more more</div>
             <div data-side="right">
               <div class="gr-diff-comment-thread">
-                <div class="message">
+                <div class="gr-formatted-text message">
-                  <span>This is a comment on the right</span>
+                  <span class="gr-linked-text">This is a comment on the right</span>
                 </div>
               </div>
             </div>
@@ -68,8 +68,8 @@ limitations under the License.
             <div class="contentText" data-side="left">ga ga</div>
             <div data-side="left">
               <div class="gr-diff-comment-thread">
-                <div class="message">
+                <div class="gr-formatted-text message">
-                  <span>This is a different comment</span>
+                  <span class="gr-linked-text">This is a different comment</span>
                 </div>
               </div>
             </div>
@@ -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();
     });
   });
 </script>

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();

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: [

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();