Merge "Fix selection on diff with range comments"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
index 229ae43..665e4a6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -119,6 +119,30 @@
return null;
}
+ _toggleRangeElHighlight(threadEl, highlightRange = false) {
+ // We don't want to re-create the line just for highlighting the range which
+ // is creating annoying bugs: @see Issue 12934
+ // As gr-ranged-comment-layer now does not notify the layer re-render and
+ // lack of access to the thread or the lineEl from the ranged-comment-layer,
+ // need to update range class for styles here.
+ const currentLine = threadEl.assignedSlot.parentElement.previousSibling;
+ if (currentLine && currentLine.querySelector) {
+ if (highlightRange) {
+ const rangeNode = currentLine.querySelector('.range');
+ if (rangeNode) {
+ rangeNode.classList.add('rangeHighlight');
+ rangeNode.classList.remove('range');
+ }
+ } else {
+ const rangeNode = currentLine.querySelector('.rangeHighlight');
+ if (rangeNode) {
+ rangeNode.classList.remove('rangeHighlight');
+ rangeNode.classList.add('range');
+ }
+ }
+ }
+ }
+
_handleCommentThreadMouseenter(e) {
const threadEl = this._getThreadEl(e);
const index = this._indexForThreadEl(threadEl);
@@ -126,6 +150,8 @@
if (index !== undefined) {
this.set(['commentRanges', index, 'hovering'], true);
}
+
+ this._toggleRangeElHighlight(threadEl, /* highlightRange= */ true);
}
_handleCommentThreadMouseleave(e) {
@@ -135,6 +161,8 @@
if (index !== undefined) {
this.set(['commentRanges', index, 'hovering'], false);
}
+
+ this._toggleRangeElHighlight(threadEl, /* highlightRange= */ false);
}
_indexForThreadEl(threadEl) {
diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
index 70aa1e7..9c9997c 100644
--- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
+++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
@@ -133,10 +133,11 @@
if (record.path === 'commentRanges') {
this._rangesMap = {left: {}, right: {}};
for (const {side, range, hovering} of record.value) {
- this._updateRangesMap(
- side, range, hovering, (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
- });
+ this._updateRangesMap({
+ side, range, hovering,
+ operation: (forLine, start, end, hovering) => {
+ forLine.push({start, end, hovering});
+ }});
}
}
@@ -147,12 +148,13 @@
// not the index, especially in polymer 1.
const {side, range, hovering} = this.get(match[1]);
- this._updateRangesMap(
- side, range, hovering, (forLine, start, end, hovering) => {
- const index = forLine.findIndex(lineRange =>
- lineRange.start === start && lineRange.end === end);
- forLine[index].hovering = hovering;
- });
+ this._updateRangesMap({
+ side, range, hovering, skipLayerUpdate: true,
+ operation: (forLine, start, end, hovering) => {
+ const index = forLine.findIndex(lineRange =>
+ lineRange.start === start && lineRange.end === end);
+ forLine[index].hovering = hovering;
+ }});
}
// If comments were spliced in or out.
@@ -160,26 +162,40 @@
for (const indexSplice of record.value.indexSplices) {
const removed = indexSplice.removed;
for (const {side, range, hovering} of removed) {
- this._updateRangesMap(
- side, range, hovering, (forLine, start, end) => {
- const index = forLine.findIndex(lineRange =>
- lineRange.start === start && lineRange.end === end);
- forLine.splice(index, 1);
- });
+ this._updateRangesMap({
+ side, range, hovering, operation: (forLine, start, end) => {
+ const index = forLine.findIndex(lineRange =>
+ lineRange.start === start && lineRange.end === end);
+ forLine.splice(index, 1);
+ }});
}
const added = indexSplice.object.slice(
indexSplice.index, indexSplice.index + indexSplice.addedCount);
for (const {side, range, hovering} of added) {
- this._updateRangesMap(
- side, range, hovering, (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
- });
+ this._updateRangesMap({
+ side, range, hovering,
+ operation: (forLine, start, end, hovering) => {
+ forLine.push({start, end, hovering});
+ }});
}
}
}
}
- _updateRangesMap(side, range, hovering, operation) {
+ /**
+ * @param {!Object} options
+ * @property {!string} options.side
+ * @property {boolean} options.hovering
+ * @property {boolean} options.skipLayerUpdate
+ * @property {!Function} options.operation
+ * @property {!{
+ * start_character: number,
+ * start_line: number,
+ * end_line: number,
+ * end_character: number}} options.range
+ */
+ _updateRangesMap(options) {
+ const {side, range, hovering, operation, skipLayerUpdate} = options;
const forSide = this._rangesMap[side] || (this._rangesMap[side] = {});
for (let line = range.start_line; line <= range.end_line; line++) {
const forLine = forSide[line] || (forSide[line] = []);
@@ -187,7 +203,9 @@
const end = line === range.end_line ? range.end_character : -1;
operation(forLine, start, end, hovering);
}
- this._notifyUpdateRange(range.start_line, range.end_line, side);
+ if (!skipLayerUpdate) {
+ this._notifyUpdateRange(range.start_line, range.end_line, side);
+ }
}
_getRangesForLine(line, side) {
diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html
index ff1f4a7..37d1707 100644
--- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html
@@ -214,11 +214,8 @@
element.set(['commentRanges', 1, 'hovering'], true);
- assert.isTrue(notifyStub.called);
- const lastCall = notifyStub.lastCall;
- assert.equal(lastCall.args[0], 10);
- assert.equal(lastCall.args[1], 12);
- assert.equal(lastCall.args[2], 'right');
+ // notify will be skipped for hovering
+ assert.isFalse(notifyStub.called);
assert.isTrue(updateRangesMapSpy.called);
});
