Fix selection on diff with range comments
Due to the way we annotating the lines, the hovering will
recreate the line which will clear the selection.
The fix is for hovering changes, skip the annotation process,
and do the style changing in the mouse move listeners.
Since the gr-ranged-comment-layer has no access or knowledge
to the thread or the line, I decided to do the style change
in the gr-diff-highlight where we are listening the mouse events.
Bug: Issue 12934
Change-Id: Ia3186a5034ad26b01d8c7f071508ac7ad2dd12d6
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);
});
