Handle tabs for range comments

Changes:
- wrapping in HL now adds cssClass instead of discarding previous ones.
- getLength accounts for tab tags correctly.
- generic splitNode, potentially should be moved into util.
- more tests.

Feature: Issue 3915
Change-Id: Id8a646a5de4fd702aa112678c039df9ff8dd8c0b

polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js

@@ -327,7 +327,7 @@
     },
 
     /**
-     * Traverse diff content from right to left, call callback for each node.
+     * Traverse Element from right to left, call callback for each node.
      * Stops if callback returns true.
      *
      * @param {!Node} startNode
@@ -338,7 +338,9 @@
       var travelLeft = opt_flags && opt_flags.left;
       var node = startNode;
       while (node) {
-        if (node instanceof Element && node.tagName !== 'HL') {
+        if (node instanceof Element &&
+            node.tagName !== 'HL' &&
+            node.tagName !== 'SPAN') {
           break;
         }
         var nextNode = travelLeft ? node.previousSibling : node.nextSibling;
@@ -360,7 +362,6 @@
         node = node.firstChild;
         var length = 0;
         while (node) {
-          // Only measure Text nodes and <hl>
           if (node instanceof Text || node.tagName == 'HL') {
             length += this._getLength(node);
           }
@@ -380,10 +381,16 @@
      * @return {!Element} Wrapped node.
      */
     _wrapInHighlight: function(node, cssClass) {
-      var hl = document.createElement('hl');
+      var hl;
-      hl.className = cssClass;
+      if (node.tagName === 'HL') {
-      Polymer.dom(node.parentElement).replaceChild(hl, node);
+        hl = node;
-      hl.appendChild(node);
+        hl.classList.add(cssClass);
+      } else {
+        hl = document.createElement('hl');
+        hl.className = cssClass;
+        Polymer.dom(node.parentElement).replaceChild(hl, node);
+        hl.appendChild(node);
+      }
       return hl;
     },
 
@@ -394,7 +401,7 @@
      * @param {number} offset
      * @return {!Text} Trailing Text Node.
      */
-    _splitText: function(node, offset) {
+    _splitTextNode: function(node, offset) {
       if (node.textContent.match(REGEX_ASTRAL_SYMBOL)) {
         // DOM Api for splitText() is broken for Unicode:
         // https://mathiasbynens.be/notes/javascript-unicode
@@ -412,11 +419,42 @@
       }
     },
 
+    /**
+     * Split Node at offset.
+     * If Node is Element, it's cloned and the node at offset is split too.
+     *
+     * @param {!Node} node
+     * @param {number} offset
+     * @return {!Node} Trailing Node.
+     */
+    _splitNode: function(element, offset) {
+      if (element instanceof Text) {
+        return this._splitTextNode(element, offset);
+      }
+      var tail = element.cloneNode(false);
+      element.parentElement.insertBefore(tail, element.nextSibling);
+      // Skip nodes before offset.
+      var node = element.firstChild;
+      while (node &&
+          this._getLength(node) <= offset ||
+          this._getLength(node) === 0) {
+        offset -= this._getLength(node);
+        node = node.nextSibling;
+      }
+      if (this._getLength(node) > offset) {
+        tail.appendChild(this._splitNode(node, offset));
+      }
+      while (node.nextSibling) {
+        tail.appendChild(node.nextSibling);
+      }
+      return tail;
+    },
+
     /**
      * Split Text Node and wrap it in hl with cssClass.
      * Wraps trailing part after split, tailing one if opt_firstPart is true.
      *
-     * @param {!Text} node
+     * @param {!Node} node
      * @param {number} offset
      * @param {string} cssClass
      * @param {boolean=} opt_firstPart
@@ -426,10 +464,10 @@
         return this._wrapInHighlight(node, cssClass);
       } else {
         if (opt_firstPart) {
-          this._splitText(node, offset);
+          this._splitNode(node, offset);
           // Node points to first part of the Text, second one is sibling.
         } else {
-          node = this._splitText(node, offset);
+          node = this._splitNode(node, offset);
         }
         return this._wrapInHighlight(node, cssClass);
       }
@@ -467,29 +505,21 @@
       if (startNode instanceof Text) {
         startNode =
             this._splitAndWrapInHighlight(startNode, startOffset, cssClass);
-        startContent.insertBefore(startNode, startNode.nextSibling);
         // Edge case: single line, text node wraps the highlight.
         if (isOneLine && this._getLength(startNode) > length) {
-          var extra = this._splitText(startNode.firstChild, length);
+          var extra = this._splitTextNode(startNode.firstChild, length);
           startContent.insertBefore(extra, startNode.nextSibling);
           startContent.normalize();
         }
       } else if (startNode.tagName == 'HL') {
         if (!startNode.classList.contains(cssClass)) {
-          var hl = startNode;
-          startNode = this._splitAndWrapInHighlight(
-              startNode.firstChild, startOffset, cssClass);
-          startContent.insertBefore(startNode, hl.nextSibling);
           // Edge case: single line, <hl> wraps the highlight.
-          if (isOneLine && this._getLength(startNode) > length) {
+          // Should leave wrapping HL's content after the highlight.
-            var trailingHl = hl.cloneNode(false);
+          if (isOneLine && startOffset + length < this._getLength(startNode)) {
-            trailingHl.appendChild(
+            this._splitNode(startNode, startOffset + length);
-                this._splitText(startNode.firstChild, length));
-            startContent.insertBefore(trailingHl, startNode.nextSibling);
-          }
-          if (hl.textContent.length === 0) {
-            hl.remove();
           }
+          startNode =
+              this._splitAndWrapInHighlight(startNode, startOffset, cssClass);
         }
       } else {
         startNode = null;
@@ -531,8 +561,7 @@
           // Split text inside HL.
           var hl = endNode;
           endNode = this._splitAndWrapInHighlight(
-              endNode.firstChild, endOffset, cssClass, true);
+              endNode, endOffset, cssClass, true);
-          endContent.insertBefore(endNode, hl);
           if (hl.textContent.length === 0) {
             hl.remove();
           }
@@ -562,19 +591,32 @@
 
       // Grow starting highlight until endNode or end of line.
       if (startNode && startNode != endNode) {
-        this._traverseContentSiblings(startNode.nextSibling, function(node) {
+        var growStartHl = function(node) {
-          startNode.textContent += node.textContent;
+          if (node instanceof Text || node.tagName === 'SPAN') {
-          node.remove();
+            startNode.appendChild(node);
+          } else if (node.tagName === 'HL') {
+            this._traverseContentSiblings(node.firstChild, growStartHl);
+            node.remove();
+          }
           return node == endNode;
-        });
+        }.bind(this);
+        this._traverseContentSiblings(startNode.nextSibling, growStartHl);
+        startNode.normalize();
       }
 
       if (!isOneLine && endNode) {
+        var growEndHl = function(node) {
+          if (node instanceof Text || node.tagName === 'SPAN') {
+            endNode.insertBefore(node, endNode.firstChild);
+          } else if (node.tagName === 'HL') {
+            this._traverseContentSiblings(node.firstChild, growEndHl);
+            node.remove();
+          }
+        }.bind(this);
         // Prepend text up to line start to the ending highlight.
-        this._traverseContentSiblings(endNode.previousSibling, function(node) {
+        this._traverseContentSiblings(
-          endNode.textContent = node.textContent + endNode.textContent;
+          endNode.previousSibling, growEndHl, {left: true});
-          node.remove();
+        endNode.normalize();
-        }, {left: true});
       }
     },
 

polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html

@@ -42,21 +42,19 @@ limitations under the License.
           <tr class="diff-row side-by-side" left-type="remove" right-type="add">
             <td class="left lineNum" data-value="140">140</td>
             <!-- Next tag is formatted to eliminate zero-length text nodes. -->
-            <td class="content remove lightHighlight">na💢ti <hl class="foo">te, inquit</hl>, sumus <hl class="bar">aliquando</hl> otiosum, <hl>certe</hl> a udiam, <hl>quid</hl> sit, quod <hl>Epicurum</hl><gr-diff-comment-thread>
+            <td class="content remove lightHighlight">na💢ti <hl class="foo">te, inquit</hl>, sumus <hl class="bar">aliquando</hl> otiosum, <hl>certe</hl> a <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl>udiam, <hl>quid</hl> sit, <span class="tab withIndicator" style="tab-size:8;"></span>quod <hl>Epicurum</hl><gr-diff-comment-thread>
                 [Yet another random diff thread content here]
               </gr-diff-comment-thread></td>
             <td class="right lineNum" data-value="120">120</td>
-            <td class="content add lightHighlight">nacti ,
+            <!-- Next tag is formatted to eliminate zero-length text nodes. -->
-              <hl>,</hl>
+            <td class="content add lightHighlight">nacti , <hl>,</hl> sumus <hl><span class="tab withIndicator" style="tab-size:8;"></span></hl> otiosum,  <span class="tab withIndicator" style="tab-size:8;"></span> audiam,  sit, quod</td>
-              sumus  otiosum,  audiam,  sit, quod
-            </td>
           </tr>
         </tbody>
 
         <tbody class="section both">
           <tr class="diff-row side-by-side" left-type="both" right-type="both">
             <td class="left lineNum" data-value="141"></td>
-            <td class="content both darkHighlight">nam et complectitur verbis, quod vult, et dicit plane, quod intellegam;</td>
+            <td class="content both darkHighlight">nam et<hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl>complectitur<span class="tab withIndicator" style="tab-size:8;">	</span>verbis, quod vult, et dicit plane, quod intellegam;</td>
             <td class="right lineNum" data-value="130"></td>
             <td class="content both darkHighlight">nam et complectitur verbis, quod vult, et dicit plane, quod intellegam;</td>
           </tr>
@@ -97,7 +95,7 @@ limitations under the License.
             <td class="left lineNum" data-value="165"></td>
             <td class="content both darkHighlight">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</td>
             <td class="right lineNum" data-value="147"></td>
-            <td class="content both darkHighlight">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</td>
+            <td class="content both darkHighlight">in physicis, <hl><span class="tab withIndicator" style="tab-size:8;">	</span></hl> quibus maxime gloriatur, primum totus est alienus. Democritea dicit</td>
           </tr>
         </tbody>
 
@@ -222,7 +220,6 @@ limitations under the License.
         });
       });
 
-
       test('renders lines in comment range on comment discard', function(done) {
         element.fire('comment-discard', {
           comment: {
@@ -314,10 +311,10 @@ limitations under the License.
       var diff = element.querySelector('#diffTable');
       var startContent =
           diff.querySelector('.left.lineNum[data-value="138"] ~ .content');
-      var endContent =
-          diff.querySelector('.left.lineNum[data-value="141"] ~ .content');
       var betweenContent =
           diff.querySelector('.left.lineNum[data-value="140"] ~ .content');
+      var endContent =
+          diff.querySelector('.left.lineNum[data-value="141"] ~ .content');
       var commentThread =
           diff.querySelector('gr-diff-comment-thread');
       var builder = {
@@ -333,7 +330,7 @@ limitations under the License.
           startContent);
       builder.getContentByLine.withArgs(141, 'left').returns(
           endContent);
-      element._applyRangedHighlight('some', 138, 4, 141, 8, 'left');
+      element._applyRangedHighlight('some', 138, 4, 141, 28, 'left');
       assert.instanceOf(startContent.childNodes[0], Text);
       assert.equal(startContent.childNodes[0].textContent, '[14]');
       assert.instanceOf(startContent.childNodes[1], Element);
@@ -342,14 +339,6 @@ limitations under the License.
       assert.equal(startContent.childNodes[1].tagName, 'HL');
       assert.equal(startContent.childNodes[1].className, 'some');
 
-      assert.instanceOf(endContent.childNodes[0], Element);
-      assert.equal(endContent.childNodes[0].textContent, 'nam et c');
-      assert.equal(endContent.childNodes[0].tagName, 'HL');
-      assert.equal(endContent.childNodes[0].className, 'some');
-      assert.instanceOf(endContent.childNodes[1], Text);
-      assert.equal(endContent.childNodes[1].textContent,
-          'omplectitur verbis, quod vult, et dicit plane, quod intellegam;');
-
       assert.instanceOf(betweenContent.firstChild, Element);
       assert.equal(betweenContent.firstChild.tagName, 'HL');
       assert.equal(betweenContent.firstChild.className, 'some');
@@ -364,6 +353,22 @@ limitations under the License.
 
       assert.strictEqual(betweenContent.querySelector('gr-diff-comment-thread'),
           commentThread, 'Comment threads should be preserved.');
+
+      assert.instanceOf(endContent.childNodes[0], Element);
+      assert.equal(endContent.childNodes[0].textContent,
+          'nam et\tcomplectitur\tverbis, ');
+      assert.equal(endContent.childNodes[0].tagName, 'HL');
+      assert.equal(endContent.childNodes[0].className, 'some');
+      assert.instanceOf(endContent.childNodes[1], Text);
+      assert.equal(endContent.childNodes[1].textContent,
+          'quod vult, et dicit plane, quod intellegam;');
+      var endHl = endContent.querySelector('hl.some');
+      assert.equal(endHl.childNodes.length, 5);
+      var tabs = endHl.querySelectorAll('span.tab');
+      assert.equal(tabs.length, 2);
+      assert.equal(tabs[0].previousSibling.textContent, 'nam et');
+      assert.equal(tabs[1].previousSibling.textContent, 'complectitur');
+      assert.equal(tabs[1].nextSibling.textContent, 'verbis, ');
     });
 
     suite('single line ranges', function() {
@@ -394,10 +399,18 @@ limitations under the License.
         assert.equal(content.firstChild.tagName, 'HL');
         assert.equal(content.firstChild.className, 'some');
         assert.equal(content.childNodes.length, 2);
-        assert.equal(content.firstChild.childNodes.length, 1);
+        assert.equal(content.firstChild.childNodes.length, 5);
         assert.equal(content.firstChild.textContent,
             'na💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
             'quid sit, quod Epicurum');
+        var tabs = content.querySelectorAll('span.tab');
+        assert.equal(tabs.length, 2);
+        assert.strictEqual(tabs[1].previousSibling, tabs[0].nextSibling);
+        assert.equal(tabs[0].previousSibling.textContent,
+            'na💢ti te, inquit, sumus aliquando otiosum, certe a ');
+        assert.equal(tabs[1].previousSibling.textContent,
+            'udiam, quid sit, ');
+        assert.equal(tabs[1].nextSibling.textContent, 'quod Epicurum');
       });
 
       test('merging multiple other hls', function() {
@@ -406,7 +419,8 @@ limitations under the License.
         assert.equal(content.childNodes.length, 4);
         var hl = content.querySelector('hl.some');
         assert.strictEqual(content.firstChild, hl.previousSibling);
-        assert.equal(hl.childNodes.length, 1);
+        assert.equal(hl.childNodes.length, 5);
+        assert.equal(content.querySelectorAll('span.tab').length, 2);
         assert.equal(hl.textContent,
             'a💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
             'quid sit, quod Epicuru');
@@ -435,7 +449,7 @@ limitations under the License.
         //  After: na💢ti <hl class="foo">te, in</hl><hl class="some">quit, ...
         element._applyRangedHighlight('some', 140, 12, 140, 21, 'left');
         var hl = content.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">quit, sum</hl>');
+        assert.equal(hl.textContent, 'quit, sum');
         assert.equal(
             hl.previousSibling.outerHTML, '<hl class="foo">te, in</hl>');
       });
@@ -445,7 +459,7 @@ limitations under the License.
         //  After: <hl class="foo">t</hl><hl="some">e, i</hl><hl class="foo">n..
         element._applyRangedHighlight('some', 140, 7, 140, 12, 'left');
         var hl = content.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">e, in</hl>');
+        assert.equal(hl.textContent, 'e, in');
         assert.equal(hl.previousSibling.outerHTML, '<hl class="foo">t</hl>');
         assert.equal(hl.nextSibling.outerHTML, '<hl class="foo">quit</hl>');
       });
@@ -453,7 +467,7 @@ limitations under the License.
       test('hl starts and ends in different hls', function() {
         element._applyRangedHighlight('some', 140, 8, 140, 27, 'left');
         var hl = content.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">, inquit, sumus ali</hl>');
+        assert.equal(hl.textContent, ', inquit, sumus ali');
         assert.equal(hl.previousSibling.outerHTML, '<hl class="foo">te</hl>');
         assert.equal(hl.nextSibling.outerHTML, '<hl class="bar">quando</hl>');
       });
@@ -468,9 +482,7 @@ limitations under the License.
       test('hl starting and ending in boundaries', function() {
         element._applyRangedHighlight('some', 140, 6, 140, 33, 'left');
         var hl = content.querySelector('hl.some');
-        assert.equal(
+        assert.equal(hl.textContent, 'te, inquit, sumus aliquando');
-            hl.outerHTML, '<hl class="some">te, inquit, sumus aliquando</hl>');
-        assert.notOk(content.querySelector('.foo'));
         assert.notOk(content.querySelector('.bar'));
       });
 
@@ -482,7 +494,7 @@ limitations under the License.
         assert.equal(hl.outerHTML, '<hl class="some">a💢t</hl>');
       });
 
-      test('growing hl left including another hl', function() {
+      test('growing hl right including another hl', function() {
         element._applyRangedHighlight('some', 140, 1, 140, 4, 'left');
         element._applyRangedHighlight('some', 140, 3, 140, 10, 'left');
         assert.equal(content.querySelectorAll('hl.some').length, 1);
@@ -491,7 +503,7 @@ limitations under the License.
         assert.equal(hl.nextSibling.outerHTML, '<hl class="foo">inquit</hl>');
       });
 
-      test('growing hl right to start of line', function() {
+      test('growing hl left to start of line', function() {
         element._applyRangedHighlight('some', 140, 2, 140, 5, 'left');
         element._applyRangedHighlight('some', 140, 0, 140, 3, 'left');
         assert.equal(content.querySelectorAll('hl.some').length, 1);
@@ -499,6 +511,14 @@ limitations under the License.
         assert.equal(hl.outerHTML, '<hl class="some">na💢ti</hl>');
         assert.strictEqual(content.firstChild, hl);
       });
+
+      test('splitting hl containing a tab', function() {
+        element._applyRangedHighlight('some', 140, 63, 140, 72, 'left');
+        assert.equal(content.querySelector('hl.some').textContent, 'sit, quod');
+        element._applyRangedHighlight('another', 140, 66, 140, 81, 'left');
+        assert.equal(content.querySelector('hl.another').textContent,
+            ', quod Epicurum');
+      });
     });
 
     test('_applyAllHighlights', function() {
@@ -644,6 +664,21 @@ limitations under the License.
       });
 
       test('multiline', function() {
+        var startContent = stubContent(119, 'right');
+        var endContent = stubContent(120, 'right');
+        emulateSelection(
+            startContent.firstChild, 10, endContent.lastChild, 7);
+        assert.isTrue(element.isRangeSelected());
+        assert.deepEqual(getActionRange(), {
+          startLine: 119,
+          startChar: 10,
+          endLine: 120,
+          endChar: 34,
+        });
+        assert.equal(getActionSide(), 'right');
+      });
+
+      test('multiline grow end highlight over tabs', function() {
         var startContent = stubContent(119, 'right');
         var endContent = stubContent(120, 'right');
         emulateSelection(startContent.firstChild, 10, endContent.firstChild, 2);
@@ -693,7 +728,7 @@ limitations under the License.
 
       test('multiple hl', function() {
         var content = stubContent(140, 'left');
-        var hl = content.querySelectorAll('hl')[3];
+        var hl = content.querySelectorAll('hl')[4];
         emulateSelection(content.firstChild, 2, hl.firstChild, 2);
         assert.isTrue(element.isRangeSelected());
         assert.deepEqual(getActionRange(), {
@@ -766,7 +801,7 @@ limitations under the License.
       test('ends in context element', function() {
         var contextControl = diff.querySelector('.contextControl');
         var content = stubContent(141, 'left');
-        emulateSelection(content.firstChild, 7, contextControl, 0);
+        emulateSelection(content.firstChild, 2, contextControl, 0);
         // TODO (viktard): Select nearest line.
         assert.isFalse(element.isRangeSelected());
       });
@@ -785,6 +820,35 @@ limitations under the License.
         assert.equal(getActionSide(), 'right');
       });
 
+      test('ends at a tab', function() {
+        var content = stubContent(140, 'left');
+        emulateSelection(
+            content.firstChild, 1, content.querySelector('span'), 0);
+        assert.isTrue(element.isRangeSelected());
+        assert.deepEqual(getActionRange(), {
+          startLine: 140,
+          startChar: 1,
+          endLine: 140,
+          endChar: 51,
+        });
+        assert.equal(getActionSide(), 'left');
+      });
+
+      test('starts at a tab', function() {
+        var content = stubContent(140, 'left');
+        emulateSelection(
+            content.querySelectorAll('hl')[3], 0,
+            content.querySelectorAll('span')[1], 0);
+        assert.isTrue(element.isRangeSelected());
+        assert.deepEqual(getActionRange(), {
+          startLine: 140,
+          startChar: 51,
+          endLine: 140,
+          endChar: 68,
+        });
+        assert.equal(getActionSide(), 'left');
+      });
+
       // TODO (viktard): Selection starts in line number.
       // TODO (viktard): Empty lines in selection start.
       // TODO (viktard): Empty lines in selection end.