From 613b49c64e2400501cc46c2128005dd046940333 Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Wed, 24 Aug 2016 14:44:21 -0700
Subject: [PATCH] Fix copy/paste in diff view

The addition of syntax highlighting silently broke copy/paste
functionality due to the addition of another layer of div nesting.

Related to this bug are some issues with correct text selection in
unified diff view, so this patch addresses them as well.

Bug: Issue 4317
Change-Id: Iac7379de4131ab4e44905a54218d42fcfe67ce62
---
 .../diff/gr-diff-builder/gr-diff-builder.js   |  8 +++-
 .../gr-diff-selection/gr-diff-selection.html  | 12 +++---
 .../gr-diff-selection/gr-diff-selection.js    | 13 ++++---
 .../gr-diff-selection_test.html               | 37 +++++++++++++------
 .../app/elements/diff/gr-diff/gr-diff.html    |  7 ----
 5 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 670885a21f..6844416cb2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -340,6 +340,9 @@
         side,
         this._comments.meta.projectConfig);
     threadEl.comments = comments;
+    if (opt_side) {
+      threadEl.setAttribute('data-side', opt_side);
+    }
     return threadEl;
   };
 
@@ -363,11 +366,14 @@
 
   GrDiffBuilder.prototype._createTextEl = function(line, opt_side) {
     var td = this._createElement('td');
+    var text = line.text;
     if (line.type !== GrDiffLine.Type.BLANK) {
       td.classList.add('content');
+      if (!text) {
+        text = '\xa0';
+      }
     }
     td.classList.add(line.type);
-    var text = line.text;
     var html = util.escapeHTML(text);
     html = this._addTabWrappers(html, this._prefs.tab_size);
 
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.html b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.html
index 09cab0b2f0..bc551299bc 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.html
@@ -18,17 +18,19 @@ limitations under the License.
 <dom-module id="gr-diff-selection">
   <template>
     <style>
-      .contentWrapper ::content .content {
+      .contentWrapper ::content .content,
+      .contentWrapper ::content .contextControl {
         -webkit-user-select: none;
         -moz-user-select: none;
         -ms-user-select: none;
         user-select: none;
       }
 
-      :host.selected-right .contentWrapper ::content .right + .content,
-      :host.selected-left .contentWrapper ::content .left + .content,
-      :host.selected-right .contentWrapper ::content .unified .right ~ .content,
-      :host.selected-left .contentWrapper ::content .unified .left ~ .content {
+      :host.selected-right .contentWrapper ::content .side-by-side .right + .content,
+      :host.selected-left .contentWrapper ::content .side-by-side .left + .content,
+      :host.selected-right .contentWrapper ::content .unified .right.lineNum ~ .content,
+      :host.selected-left .contentWrapper ::content .unified .left.lineNum ~ .content:not(.both),
+      :host .contentWrapper ::content .unified .gr-diff-comment .message {
         -webkit-user-select: text;
         -moz-user-select: text;
         -ms-user-select: text;
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 7d0b7ea6d6..80e758ec7f 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
@@ -56,7 +56,8 @@
     },
 
     _handleCopy: function(e) {
-      if (!e.target.classList.contains('content')) {
+      if (!e.target.classList.contains('contentText') &&
+          !e.target.classList.contains('gr-syntax')) {
         return;
       }
       var lineEl = this.diffBuilder.getLineElByChild(e.target);
@@ -69,17 +70,17 @@
       e.preventDefault();
     },
 
-    _getSelectedText: function(opt_side) {
+    _getSelectedText: function(side) {
       var sel = window.getSelection();
       if (sel.rangeCount != 1) {
         return; // No multi-select support yet.
       }
       var range = sel.getRangeAt(0);
       var fragment = range.cloneContents();
-      var selector = '.content,td.content:nth-of-type(1)';
-      if (opt_side) {
-        selector = '.' + opt_side + ' + ' + selector;
-      }
+      var selector = '.contentText';
+      selector += '[data-side="' + side + '"]';
+      selector += ':not(:empty)';
+
       var contentEls = Polymer.dom(fragment).querySelectorAll(selector);
       if (contentEls.length === 0) {
         return fragment.textContent;
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 f99e373357..4472ebd425 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
@@ -30,21 +30,33 @@ limitations under the License.
       <table>
         <tr>
           <td class="lineNum left">1</td>
-          <td class="content">ba ba</td>
+          <td class="content">
+            <div class="contentText" data-side="left">ba ba</div>
+          </td>
           <td class="lineNum right">1</td>
-          <td class="content">some other text</td>
+          <td class="content">
+            <div class="contentText" data-side="right">some other text</div>
+          </td>
         </tr>
         <tr>
           <td class="lineNum left">2</td>
-          <td class="content">zin</td>
+          <td class="content">
+            <div class="contentText" data-side="left">zin</div>
+          </td>
           <td class="lineNum right">2</td>
-          <td class="content">more more more</td>
+          <td class="content">
+            <div class="contentText" data-side="right">more more more</div>
+          </td>
         </tr>
         <tr>
           <td class="lineNum left">2</td>
-          <td class="content">ga ga</td>
+          <td class="content">
+            <div class="contentText" data-side="left">ga ga</div>
+          </td>
           <td class="lineNum right">3</td>
-          <td class="other">some other text</td>
+          <td class="other">
+            <div class="contentText" data-side="right">some other text</div>
+          </td>
         </tr>
       </table>
     </gr-diff-selection>
@@ -105,36 +117,37 @@ limitations under the License.
     test('asks for text for right side Elements', function() {
       element._cachedDiffBuilder.getSideByLineEl.returns('left');
       sinon.stub(element, '_getSelectedText');
-      emulateCopyOn(element.querySelector('td.content'));
+      emulateCopyOn(element.querySelector('div.contentText'));
       assert.deepEqual(['left'], element._getSelectedText.lastCall.args);
     });
 
     test('reacts to copy for content Elements', function() {
       sinon.stub(element, '_getSelectedText');
-      emulateCopyOn(element.querySelector('td.content'));
+      emulateCopyOn(element.querySelector('div.contentText'));
       assert.isTrue(element._getSelectedText.called);
     });
 
     test('copy event is prevented for content Elements', function() {
       sinon.stub(element, '_getSelectedText');
-      var event = emulateCopyOn(element.querySelector('td.content'));
+      var event = emulateCopyOn(element.querySelector('div.contentText'));
       assert.isTrue(event.preventDefault.called);
     });
 
     test('inserts text into clipboard on copy', function() {
       sinon.stub(element, '_getSelectedText').returns('the text');
-      var event = emulateCopyOn(element.querySelector('td.content'));
+      var event = emulateCopyOn(element.querySelector('div.contentText'));
       assert.deepEqual(
           ['Text', 'the text'], event.clipboardData.setData.lastCall.args);
     });
 
     test('copies content correctly', function() {
       element.classList.add('selected-left');
+      element.classList.remove('selected-right');
       var selection = window.getSelection();
       var range = document.createRange();
-      range.setStart(element.querySelector('td.content').firstChild, 3);
+      range.setStart(element.querySelector('div.contentText').firstChild, 3);
       range.setEnd(
-          element.querySelectorAll('td.content')[4].firstChild, 2);
+          element.querySelectorAll('div.contentText')[4].firstChild, 2);
       selection.addRange(range);
       assert.equal('ba\nzin\nga\n', element._getSelectedText('left'));
     });
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 2431fb00c0..36a18fc955 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -91,13 +91,6 @@ limitations under the License.
         vertical-align: top;
         white-space: pre;
       }
-      .contentText:empty:before {
-        /**
-         * Insert glyph to prevent empty diff content from collapsing.
-         * "\200B" is a 'ZERO WIDTH SPACE' (U+200B)
-         */
-        content: "\200B";
-      }
       .contextLineNum:before,
       .lineNum:before {
         display: inline-block;