opendev/gerrit
Code Issues Proposed changes

Reorganized DOM for diff content in PolyGerrit

Browse Source 
Formerly, diff content elements mixed text with comment threads. For
example, a diff content node with an intraline highlight, a ranged
comment, and a gr-diff-comment-thread may have been organized as below:

    TD.content
      ╠ #text
      ╠ HL (intraline difference)
      ║ ╚ #text
      ╠ #text
      ╠ HL.range (ranged comment highlight)
      ║ ╚ #text
      ╠ #text
      ╚ GR-DIFF-COMMENT-THREAD
        ╠ GR-DIFF-COMMENT
        ╚ ...

Note that the comment thread was inserted at the same level as the text
of the diff line.

With this change, the text is separated from the comment thread by
introducing a DIV to contain the text with class `contentText` as
sibling to comment threads.

    TD.content
      ╠ DIV.contentText
      ║ ╠ #text
      ║ ╠ HL
      ║ ║ ╚ #text
      ║ ╠ #text
      ║ ╠ HL.range
      ║ ║ ╚ #text
      ║ ╚ #text
      ╚ GR-DIFF-COMMENT-THREAD
        ╠ GR-DIFF-COMMENT
        ╚ ...

Modifies the `getContentByLine` method of gr-diff-builder to return the
`DIV.contentText` element rather than the `TD.content` element which is
its parent. In most uses of this function, the text is what is needed
rather than the TD or comment thread, but in other cases, they can be
easily DOM traversed.

Change-Id: I0eded34afd3d22963252efc7eabfee290ae21a9c
This commit is contained in:
Wyatt Allen
2016-07-01 09:30:44 -07:00
parent f9379c1b7b
commit 025e65d7d5
5 changed files with 109 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
View File

@@ -173,18 +173,19 @@
if (!line) {
return;
}
var content = this.diffBuilder.getContentByLineEl(lineEl);
if (!content) {
var contentText = this.diffBuilder.getContentByLineEl(lineEl);
if (!contentText) {
return;
}
if (!content.contains(node)) {
node = content;
var contentTd = contentText.parentElement;
if (!contentTd.contains(node)) {
node = contentText;
column = 0;
} else {
var thread = content.querySelector('gr-diff-comment-thread');
var thread = contentTd.querySelector('gr-diff-comment-thread');
if (thread && thread.contains(node)) {
column = this._getLength(content);
node = content;
column = this._getLength(contentText);
node = contentText;
} else {
column = this._convertOffsetToColumn(node, offset);
}
@@ -322,22 +323,15 @@
},
/**
* Get length of a node. Traverses diff content siblings if required.
* Get length of a node. If the node is a content node, then only give the
* length of its .contentText child.
*
* @param {!Node} node
* @return {number}
*/
_getLength: function(node) {
if (node instanceof Element && node.classList.contains('content')) {
node = node.firstChild;
var length = 0;
while (node) {
if (node instanceof Text || node.tagName == 'HL') {
length += this._getLength(node);
}
node = node.nextSibling;
}
return length;
return this._getLength(node.querySelector('.contentText'));
} else {
// DOM API for textContent.length is broken for Unicode:
// https://mathiasbynens.be/notes/javascript-unicode
@@ -446,8 +440,10 @@
/**
* Creates hl tag with cssClass for starting side of range highlight.
*
* @param {!Element} startContent Range start diff content aka td.content.
* @param {!Element} endContent Range end diff content aka td.content.
* @param {!Element} startContent Range start diff content
* aka div.contentText.
* @param {!Element} endContent Range end diff content
* aka div.contentText.
* @param {number} startOffset Range start within start content.
* @param {number} endOffset Range end within end content.
* @param {string} cssClass
@@ -501,8 +497,10 @@
/**
* Creates hl tag with cssClass for ending side of range highlight.
*
* @param {!Element} startContent Range start diff content aka td.content.
* @param {!Element} endContent Range end diff content aka td.content.
* @param {!Element} startContent Range start diff content
* aka div.contentText.
* @param {!Element} endContent Range end diff content
* aka div.contentText.
* @param {number} startOffset Range start within start content.
* @param {number} endOffset Range end within end content.
* @param {string} cssClass
@@ -547,8 +545,10 @@
/**
* Applies highlight to first and last lines in range.
*
* @param {!Element} startContent Range start diff content aka td.content.
* @param {!Element} endContent Range end diff content aka td.content.
* @param {!Element} startContent Range start diff content
* aka div.contentText.
* @param {!Element} endContent Range end diff content
* aka div.contentText.
* @param {number} startOffset Range start within start content.
* @param {number} endOffset Range end within end content.
* @param {string} cssClass

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

@@ -32,9 +32,9 @@ limitations under the License.
<tbody class="section both">
<tr class="diff-row side-by-side" left-type="both" right-type="both">
<td class="left lineNum" data-value="138">138</td>
<td class="content both darkHighlight">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</td>
<td class="content both darkHighlight"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
<td class="right lineNum" data-value="119">119</td>
<td class="content both darkHighlight">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</td>
<td class="content both darkHighlight"><div class="contentText">[14] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
</tr>
</tbody>
@@ -42,21 +42,21 @@ 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 <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>
<td class="content remove lightHighlight"><div class="contentText">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></div><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>
<!-- Next tag is formatted to eliminate zero-length text nodes. -->
<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>
<td class="content add lightHighlight"><div class="contentText">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</div></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<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="content both darkHighlight"><div class="contentText">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;</div></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>
<td class="content both darkHighlight"><div class="contentText">nam et complectitur verbis, quod vult, et dicit plane, quod intellegam;</div></td>
</tr>
</tbody>
@@ -86,16 +86,16 @@ limitations under the License.
<td class="left"></td>
<td class="blank darkHighlight"></td>
<td class="right lineNum" data-value="146"></td>
<td class="content add darkHighlight">[17] Quid igitur est? inquit; audire enim cupio, quid non probes. Principio, inquam,</td>
<td class="content add darkHighlight"><div class="contentText">[17] Quid igitur est? inquit; audire enim cupio, quid non probes. Principio, inquam,</div></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="165"></td>
<td class="content both darkHighlight">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</td>
<td class="content both darkHighlight"><div class="contentText">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
<td class="right lineNum" data-value="147"></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>
<td class="content both darkHighlight"><div class="contentText">in physicis, <hl><span class="tab withIndicator" style="tab-size:8;"> </span></hl> quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
</tr>
</tbody>
@@ -309,12 +309,12 @@ limitations under the License.
test('apply multiline highlight', function() {
var diff = element.querySelector('#diffTable');
var startContent =
diff.querySelector('.left.lineNum[data-value="138"] ~ .content');
var betweenContent =
diff.querySelector('.left.lineNum[data-value="140"] ~ .content');
var endContent =
diff.querySelector('.left.lineNum[data-value="141"] ~ .content');
var startContent = diff.querySelector(
'.left.lineNum[data-value="138"] ~ .content .contentText');
var betweenContent = diff.querySelector(
'.left.lineNum[data-value="140"] ~ .content .contentText');
var endContent = diff.querySelector(
'.left.lineNum[data-value="141"] ~ .content .contentText');
var commentThread =
diff.querySelector('gr-diff-comment-thread');
var builder = {
@@ -407,18 +407,19 @@ limitations under the License.
suite('single line ranges', function() {
var diff;
var content;
var contentText;
var commentThread;
var builder;
setup(function() {
diff = element.querySelector('#diffTable');
content =
diff.querySelector('.left.lineNum[data-value="140"] ~ .content');
var contentTd = diff.querySelector(
'.left.lineNum[data-value="140"] ~ .content');
contentText = contentTd.querySelector('.contentText');
commentThread = diff.querySelector('gr-diff-comment-thread');
builder = {
getCommentThreadByContentEl: sandbox.stub().returns(commentThread),
getContentByLine: sandbox.stub().returns(content),
getContentByLine: sandbox.stub().returns(contentText),
getContentsByLineRange: sandbox.stub().returns([]),
getLineElByChild: sandbox.stub().returns(
{getAttribute: sandbox.stub()}),
@@ -429,15 +430,15 @@ limitations under the License.
test('whole line range', function() {
element._applyRangedHighlight('some', 140, 0, 140, 81, 'left');
assert.instanceOf(content.firstChild, Element);
assert.equal(content.firstChild.tagName, 'HL');
assert.equal(content.firstChild.className, 'some');
assert.equal(content.childNodes.length, 2);
assert.equal(content.firstChild.childNodes.length, 5);
assert.equal(content.firstChild.textContent,
assert.equal(contentText.childNodes.length, 1);
assert.instanceOf(contentText.firstChild, Element);
assert.equal(contentText.firstChild.tagName, 'HL');
assert.equal(contentText.firstChild.className, 'some');
assert.equal(contentText.firstChild.childNodes.length, 5);
assert.equal(contentText.firstChild.textContent,
'na💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
'quid sit, quod Epicurum');
var tabs = content.querySelectorAll('span.tab');
var tabs = contentText.querySelectorAll('span.tab');
assert.equal(tabs.length, 2);
assert.strictEqual(tabs[1].previousSibling, tabs[0].nextSibling);
assert.equal(tabs[0].previousSibling.textContent,
@@ -449,12 +450,12 @@ limitations under the License.
test('merging multiple other hls', function() {
element._applyRangedHighlight('some', 140, 1, 140, 80, 'left');
assert.instanceOf(content.firstChild, Text);
assert.equal(content.childNodes.length, 4);
var hl = content.querySelector('hl.some');
assert.strictEqual(content.firstChild, hl.previousSibling);
assert.instanceOf(contentText.firstChild, Text);
assert.equal(contentText.childNodes.length, 3);
var hl = contentText.querySelector('hl.some');
assert.strictEqual(contentText.firstChild, hl.previousSibling);
assert.equal(hl.childNodes.length, 5);
assert.equal(content.querySelectorAll('span.tab').length, 2);
assert.equal(contentText.querySelectorAll('span.tab').length, 2);
assert.equal(hl.textContent,
'a💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
'quid sit, quod Epicuru');
@@ -464,7 +465,7 @@ limitations under the License.
// Before: na💢ti
// After: n<hl class="some">a💢t</hl>i
element._applyRangedHighlight('some', 140, 1, 140, 4, 'left');
var hl = content.querySelector('hl.some');
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">a💢t</hl>');
});
@@ -472,7 +473,7 @@ limitations under the License.
// Before: na💢ti <hl>te, inquit</hl>,
// After: na💢<hl class="some">ti te</hl><hl class="foo">, inquit</hl>,
element._applyRangedHighlight('some', 140, 3, 140, 8, 'left');
var hl = content.querySelector('hl.some');
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">ti te</hl>');
assert.equal(hl.nextSibling.outerHTML,
'<hl class="foo">, inquit</hl>');
@@ -482,7 +483,7 @@ limitations under the License.
// Before: na💢ti <hl>te, inquit</hl>, sumus
// 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');
var hl = contentText.querySelector('hl.some');
assert.equal(hl.textContent, 'quit, sum');
assert.equal(
hl.previousSibling.outerHTML, '<hl class="foo">te, in</hl>');
@@ -492,7 +493,7 @@ limitations under the License.
// Before: na💢ti <hl class="foo">te, inquit</hl>, sumus
// 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');
var hl = contentText.querySelector('hl.some');
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>');
@@ -500,7 +501,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');
var hl = contentText.querySelector('hl.some');
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>');
@@ -508,31 +509,31 @@ limitations under the License.
test('hl over different hl', function() {
element._applyRangedHighlight('some', 140, 2, 140, 21, 'left');
var hl = content.querySelector('hl.some');
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">💢ti te, inquit, sum</hl>');
assert.notOk(content.querySelector('.foo'));
assert.notOk(contentText.querySelector('.foo'));
});
test('hl starting and ending in boundaries', function() {
element._applyRangedHighlight('some', 140, 6, 140, 33, 'left');
var hl = content.querySelector('hl.some');
var hl = contentText.querySelector('hl.some');
assert.equal(hl.textContent, 'te, inquit, sumus aliquando');
assert.notOk(content.querySelector('.bar'));
assert.notOk(contentText.querySelector('.bar'));
});
test('overlapping hls', function() {
element._applyRangedHighlight('some', 140, 1, 140, 3, 'left');
element._applyRangedHighlight('some', 140, 2, 140, 4, 'left');
assert.equal(content.querySelectorAll('hl.some').length, 1);
var hl = content.querySelector('hl.some');
assert.equal(contentText.querySelectorAll('hl.some').length, 1);
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">a💢t</hl>');
});
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);
var hl = content.querySelector('hl.some');
assert.equal(contentText.querySelectorAll('hl.some').length, 1);
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">a💢ti te, </hl>');
assert.equal(hl.nextSibling.outerHTML, '<hl class="foo">inquit</hl>');
});
@@ -540,17 +541,18 @@ limitations under the License.
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);
var hl = content.querySelector('hl.some');
assert.equal(contentText.querySelectorAll('hl.some').length, 1);
var hl = contentText.querySelector('hl.some');
assert.equal(hl.outerHTML, '<hl class="some">na💢ti</hl>');
assert.strictEqual(content.firstChild, hl);
assert.strictEqual(contentText.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');
assert.equal(contentText.querySelector('hl.some').textContent,
'sit, quod');
element._applyRangedHighlight('another', 140, 66, 140, 81, 'left');
assert.equal(content.querySelector('hl.another').textContent,
assert.equal(contentText.querySelector('hl.another').textContent,
', quod Epicurum');
});
});
@@ -620,19 +622,21 @@ limitations under the License.
var contentStubs;
var stubContent = function(line, side, opt_child) {
var content = diff.querySelector(
var contentTd = diff.querySelector(
'.' + side + '.lineNum[data-value="' + line + '"] ~ .content');
var contentText = contentTd.querySelector('.contentText');
var lineEl = diff.querySelector(
'.' + side + '.lineNum[data-value="' + line + '"]');
contentStubs.push({
lineEl: lineEl,
content: content,
contentTd: contentTd,
contentText: contentText,
});
builder.getContentByLineEl.withArgs(lineEl).returns(content);
builder.getContentByLineEl.withArgs(lineEl).returns(contentText);
builder.getLineNumberByChild.withArgs(lineEl).returns(line);
builder.getContentByLine.withArgs(line, side).returns(content);
builder.getContentByLine.withArgs(line, side).returns(contentText);
builder.getSideByLineEl.withArgs(lineEl).returns(side);
return content;
return contentText;
};
var emulateSelection = function(
@@ -657,7 +661,7 @@ limitations under the License.
var getLineElByChild = function(node) {
var stubs = contentStubs.find(function(stub) {
return stub.content.contains(node);
return stub.contentTd.contains(node);
});
return stubs && stubs.lineEl;
};
@@ -775,9 +779,11 @@ limitations under the License.
});
test('starts outside of diff', function() {
var content = stubContent(140, 'left');
emulateSelection(content.previousElementSibling.firstChild, 2,
content.firstChild, 2);
var contentText = stubContent(140, 'left');
var contentTd = contentText.parentElement;
emulateSelection(contentTd.previousElementSibling.firstChild, 2,
contentText.firstChild, 2);
assert.isFalse(element.isRangeSelected());
});
@@ -797,7 +803,8 @@ limitations under the License.
test('starts in comment thread element', function() {
var startContent = stubContent(140, 'left');
var comment = startContent.querySelector('gr-diff-comment-thread');
var comment = startContent.parentElement.querySelector(
'gr-diff-comment-thread');
var endContent = stubContent(141, 'left');
emulateSelection(comment.firstChild, 2, endContent.firstChild, 4);
assert.isTrue(element.isRangeSelected());
@@ -812,7 +819,8 @@ limitations under the License.
test('ends in comment thread element', function() {
var content = stubContent(140, 'left');
var comment = content.querySelector('gr-diff-comment-thread');
var comment = content.parentElement.querySelector(
'gr-diff-comment-thread');
emulateSelection(content.firstChild, 4, comment.firstChild, 1);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {