opendev/gerrit
Code Issues Proposed changes

Fix triple click select and copy when other diff side is empty

Browse Source 
Triple click in side-by-side mode with other side empty results in DOM
selection ending on the empty div line number tag. Normalizing such
selection results in null element for the end container.

This is handled on ad-hoc basis by validating where DOM selection landed
and is handled properly.

Previous fix (https://gerrit-review.googlesource.com/c/gerrit/+/193470)
was only addressing the case when the other diff side is populated.

Bug: Issue 8431
Change-Id: I17964db98c217d30dd266e01d0a03a11ed9c42b9
This commit is contained in:
viktard
2018-10-17 14:42:50 -07:00
parent 700db764ae
commit 05e0f20600
3 changed files with 35 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -171,13 +171,19 @@
}
const start = range.start;
const end = range.end;
// Happens when triple click in side-by-side mode with other side empty.
const endsAtOtherEmptySide = !end &&
domRange.endOffset === 0 &&
domRange.endContainer.nodeName === 'TD' &&
(domRange.endContainer.classList.contains('left') ||
domRange.endContainer.classList.contains('right'));
const endsAtBeginningOfNextLine = end &&
start.column === 0 &&
end.column === 0 &&
end.line === start.line + 1;
const content = domRange.cloneContents().querySelector('.contentText');
const lineLength = content && this._getLength(content) || 0;
if (lineLength && endsAtBeginningOfNextLine) {
if (lineLength && (endsAtBeginningOfNextLine || endsAtOtherEmptySide)) {
// Move the selection to the end of the previous line.
range.end = {
node: start.node,

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

@@ -123,7 +123,7 @@ 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="165"></td>
<td class="content both"><div class="contentText">in physicis, quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
<td class="content both"><div class="contentText"></div></td>
<td class="right lineNum" data-value="147"></td>
<td class="content both"><div class="contentText">in physicis, <hl><span class="tab-indicator" style="tab-size:8;"> </span></hl> quibus maxime gloriatur, primum totus est alienus. Democritea dicit</div></td>
</tr>
@@ -589,6 +589,21 @@ limitations under the License.
});
assert.equal(getActionSide(), 'right');
});
test('_fixTripleClickSelection empty line', () => {
const startContent = stubContent(146, 'right');
const endContent = stubContent(165, 'left');
emulateSelection(startContent.firstChild, 0,
endContent.parentElement.previousElementSibling, 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
startLine: 146,
startChar: 0,
endLine: 146,
endChar: 84,
});
assert.equal(getActionSide(), 'right');
});
});
});
</script>

15
polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js
View File

@@ -179,10 +179,19 @@
const startLineEl =
this.diffBuilder.getLineElByChild(range.startContainer);
const endLineEl = this.diffBuilder.getLineElByChild(range.endContainer);
// Happens when triple click in side-by-side mode with other side empty.
const endsAtOtherEmptySide = !endLineEl &&
range.endOffset === 0 &&
range.endContainer.nodeName === 'TD' &&
(range.endContainer.classList.contains('left') ||
range.endContainer.classList.contains('right'));
const startLineNum = parseInt(startLineEl.getAttribute('data-value'), 10);
const endLineNum = endLineEl === null ?
undefined :
parseInt(endLineEl.getAttribute('data-value'), 10);
let endLineNum;
if (endsAtOtherEmptySide) {
endLineNum = startLineNum + 1;
} else if (endLineEl) {
endLineNum = parseInt(endLineEl.getAttribute('data-value'), 10);
}
return this._getRangeFromDiff(startLineNum, range.startOffset, endLineNum,
range.endOffset, side);