Merge "Fix highlight on multi-line 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 665e4a6..b82d4b8 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
@@ -23,6 +23,7 @@
import {htmlTemplate} from './gr-diff-highlight_html.js';
import {GrAnnotation} from './gr-annotation.js';
import {GrRangeNormalizer} from './gr-range-normalizer.js';
+import {strToClassName} from '../../../utils/dom-util.js';
/**
* @extends PolymerElement
@@ -125,20 +126,27 @@
// 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) {
+ let curNode = threadEl.assignedSlot;
+ while (curNode) {
+ if (curNode.nodeName === 'TABLE') break;
+ curNode = curNode.parentElement;
+ }
+ if (curNode && curNode.querySelectorAll) {
if (highlightRange) {
- const rangeNode = currentLine.querySelector('.range');
- if (rangeNode) {
+ const rangeNodes = curNode
+ .querySelectorAll(`.range.${strToClassName(threadEl.rootId)}`);
+ rangeNodes.forEach(rangeNode => {
rangeNode.classList.add('rangeHighlight');
rangeNode.classList.remove('range');
- }
+ });
} else {
- const rangeNode = currentLine.querySelector('.rangeHighlight');
- if (rangeNode) {
+ const rangeNodes = curNode.querySelectorAll(
+ `.rangeHighlight.${strToClassName(threadEl.rootId)}`
+ );
+ rangeNodes.forEach(rangeNode => {
rangeNode.classList.remove('rangeHighlight');
rangeNode.classList.add('range');
- }
+ });
}
}
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 252b9bc..4d96d53 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -352,7 +352,7 @@
function commentRangeFromThreadEl(threadEl) {
const side = threadEl.getAttribute('comment-side');
const range = JSON.parse(threadEl.getAttribute('range'));
- return {side, range, hovering: false};
+ return {side, range, hovering: false, rootId: threadEl.rootId};
}
const addedCommentRanges = addedThreadEls
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 c774f1a..231e4b5 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
@@ -20,6 +20,7 @@
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {htmlTemplate} from './gr-ranged-comment-layer_html.js';
import {GrDiffLine} from '../gr-diff/gr-diff-line.js';
+import {strToClassName} from '../../../utils/dom-util.js';
// Polymer 1 adds # before array's key, while Polymer 2 doesn't
const HOVER_PATH_PATTERN = /^(commentRanges\.#?\d+)\.hovering$/;
@@ -92,7 +93,8 @@
for (const range of ranges) {
GrAnnotation.annotateElement(el, range.start,
range.end - range.start,
- range.hovering ? HOVER_HIGHLIGHT : RANGE_HIGHLIGHT);
+ (range.hovering ? HOVER_HIGHLIGHT : RANGE_HIGHLIGHT) +
+ ` ${strToClassName(range.rootId)}`);
}
}
@@ -136,11 +138,11 @@
// If the entire set of comments was changed.
if (record.path === 'commentRanges') {
this._rangesMap = {left: {}, right: {}};
- for (const {side, range, hovering} of record.value) {
+ for (const {side, range, rootId, hovering} of record.value) {
this._updateRangesMap({
side, range, hovering,
operation: (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
+ forLine.push({start, end, hovering, rootId});
}});
}
}
@@ -150,7 +152,7 @@
if (match) {
// The #number indicates the key of that item in the array
// not the index, especially in polymer 1.
- const {side, range, hovering} = this.get(match[1]);
+ const {side, range, hovering, rootId} = this.get(match[1]);
this._updateRangesMap({
side, range, hovering, skipLayerUpdate: true,
@@ -158,6 +160,7 @@
const index = forLine.findIndex(lineRange =>
lineRange.start === start && lineRange.end === end);
forLine[index].hovering = hovering;
+ forLine[index].rootId = rootId;
}});
}
@@ -165,21 +168,22 @@
if (record.path === 'commentRanges.splices') {
for (const indexSplice of record.value.indexSplices) {
const removed = indexSplice.removed;
- for (const {side, range, hovering} of removed) {
+ for (const {side, range, hovering, rootId} of removed) {
this._updateRangesMap({
side, range, hovering, operation: (forLine, start, end) => {
const index = forLine.findIndex(lineRange =>
- lineRange.start === start && lineRange.end === end);
+ lineRange.start === start && lineRange.end === end &&
+ rootId === lineRange.rootId);
forLine.splice(index, 1);
}});
}
const added = indexSplice.object.slice(
indexSplice.index, indexSplice.index + indexSplice.addedCount);
- for (const {side, range, hovering} of added) {
+ for (const {side, range, hovering, rootId} of added) {
this._updateRangesMap({
side, range, hovering,
operation: (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
+ forLine.push({start, end, hovering, rootId});
}});
}
}
diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
index 2ce0afa..5f32677 100644
--- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
@@ -36,6 +36,7 @@
start_character: 6,
start_line: 36,
},
+ rootId: 'a',
},
{
side: 'right',
@@ -45,6 +46,7 @@
start_character: 10,
start_line: 10,
},
+ rootId: 'b',
},
{
side: 'right',
@@ -54,6 +56,7 @@
start_character: 5,
start_line: 100,
},
+ rootId: 'c',
},
{
side: 'right',
@@ -63,6 +66,7 @@
start_character: 32,
start_line: 55,
},
+ rootId: 'd',
},
];
@@ -106,7 +110,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_a');
});
test('type=Remove has-comment hovering', () => {
@@ -124,7 +128,9 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff rangeHighlight');
+ assert.equal(
+ lastCall.args[3], 'style-scope gr-diff rangeHighlight generated_a'
+ );
});
test('type=Both has-comment', () => {
@@ -141,7 +147,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_a');
});
test('type=Both has-comment off side', () => {
@@ -169,7 +175,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_b');
});
});
diff --git a/polygerrit-ui/app/utils/dom-util.js b/polygerrit-ui/app/utils/dom-util.js
index a9f080f..e26bf74 100644
--- a/polygerrit-ui/app/utils/dom-util.js
+++ b/polygerrit-ui/app/utils/dom-util.js
@@ -175,4 +175,18 @@
element = element.parentElement;
}
return isDescendant;
+}
+
+/**
+ * Convert any string into a valid class name.
+ *
+ * For class names, naming rules:
+ * Must begin with a letter A-Z or a-z
+ * Can be followed by: letters (A-Za-z), digits (0-9), hyphens ("-"), and underscores ("_")
+ *
+ * @param {string} str
+ * @param {string} prefix
+ */
+export function strToClassName(str = '', prefix = 'generated_') {
+ return `${prefix}${str.replace(/[^a-zA-Z0-9-_]/g, '_')}`;
}
\ No newline at end of file
diff --git a/polygerrit-ui/app/utils/dom-util_test.js b/polygerrit-ui/app/utils/dom-util_test.js
index c317578..4306cd2 100644
--- a/polygerrit-ui/app/utils/dom-util_test.js
+++ b/polygerrit-ui/app/utils/dom-util_test.js
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import '../test/common-test-setup-karma.js';
-import {getComputedStyleValue, querySelector, querySelectorAll, descendedFromClass, getEventPath} from './dom-util.js';
+import {strToClassName, getComputedStyleValue, querySelector, querySelectorAll, descendedFromClass, getEventPath} from './dom-util.js';
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
@@ -136,4 +136,16 @@
querySelector(testEl, '.b')));
});
});
+
+ suite('strToClassName', () => {
+ test('basic tests', () => {
+ assert.equal(strToClassName(''), 'generated_');
+ assert.equal(strToClassName('11'), 'generated_11');
+ assert.equal(strToClassName('0.123'), 'generated_0_123');
+ assert.equal(strToClassName('0.123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0>123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0<123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0+1+23', 'prefix_'), 'prefix_0_1_23');
+ });
+ });
});
\ No newline at end of file
