Imperative named Slots

The idea is to instead of imperatively appending slotted comments to
threadGroupEls (which results in them being removed from the light DOM
of the gr-diff element and thus causing all kinds of tracking
difficulties), to slot them into a named slot in the appropriate line.

Change-Id: I17e99b236240baab581f8c266bed3d400a302408

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-binary.js

@@ -20,10 +20,8 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderBinary) { return; }
 
-  function GrDiffBuilderBinary(diff, commentThreadEls, prefs,
+  function GrDiffBuilderBinary(diff, prefs, outputEl) {
-      outputEl) {
+    GrDiffBuilder.call(this, diff, prefs, outputEl);
-    GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
-        outputEl);
   }
 
   GrDiffBuilderBinary.prototype = Object.create(GrDiffBuilder.prototype);

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js

@@ -22,10 +22,8 @@
 
   const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/;
 
-  function GrDiffBuilderImage(diff, commentThreadEls, prefs,
+  function GrDiffBuilderImage(diff, prefs, outputEl, baseImage, revisionImage) {
-      outputEl, baseImage, revisionImage) {
+    GrDiffBuilderSideBySide.call(this, diff, prefs, outputEl, []);
-    GrDiffBuilderSideBySide.call(this, diff, commentThreadEls,
-        prefs, outputEl, []);
     this._baseImage = baseImage;
     this._revisionImage = revisionImage;
   }

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js

@@ -20,10 +20,8 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderSideBySide) { return; }
 
-  function GrDiffBuilderSideBySide(diff, commentThreadEls,
+  function GrDiffBuilderSideBySide(diff, prefs, outputEl, layers) {
-      prefs, outputEl, layers) {
+    GrDiffBuilder.call(this, diff, prefs, outputEl, layers);
-    GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
-        outputEl, layers);
   }
   GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
@@ -99,10 +97,6 @@
       row.appendChild(action);
     } else {
       const textEl = this._createTextEl(line, side);
-      const threadGroupEl = this._commentThreadGroupForLine(line, side);
-      if (threadGroupEl) {
-        textEl.appendChild(threadGroupEl);
-      }
       row.appendChild(textEl);
     }
   };

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js

@@ -20,10 +20,8 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderUnified) { return; }
 
-  function GrDiffBuilderUnified(diff, commentThreadEls, prefs,
+  function GrDiffBuilderUnified(diff, prefs, outputEl, layers) {
-      outputEl, layers) {
+    GrDiffBuilder.call(this, diff, prefs, outputEl, layers);
-    GrDiffBuilder.call(this, diff, commentThreadEls, prefs,
-        outputEl, layers);
   }
   GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;
@@ -88,10 +86,6 @@
       row.appendChild(action);
     } else {
       const textEl = this._createTextEl(line);
-      const threadGroupEl = this._commentThreadGroupForLine(line);
-      if (threadGroupEl) {
-        textEl.appendChild(threadGroupEl);
-      }
       row.appendChild(textEl);
     }
     return row;

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html

@@ -121,10 +121,6 @@ limitations under the License.
           return this.queryEffectiveChildren('#diffTable');
         },
 
-        get _commentThreadElements() {
-          return this.queryAllEffectiveChildren('.comment-thread');
-        },
-
         observers: [
           '_groupsChanged(_groups.splices)',
         ],
@@ -292,20 +288,16 @@ limitations under the License.
 
           let builder = null;
           if (this.isImageDiff) {
-            builder = new GrDiffBuilderImage(diff,
+            builder = new GrDiffBuilderImage(diff, prefs, this.diffElement,
-              this._commentThreadElements, prefs, this.diffElement,
               this.baseImage, this.revisionImage);
           } else if (diff.binary) {
             // If the diff is binary, but not an image.
-            return new GrDiffBuilderBinary(diff,
+            return new GrDiffBuilderBinary(diff, prefs, this.diffElement);
-                this._commentThreadElements, prefs, this.diffElement);
           } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
-            builder = new GrDiffBuilderSideBySide(diff,
+            builder = new GrDiffBuilderSideBySide(diff, prefs, this.diffElement,
-                this._commentThreadElements, prefs, this.diffElement,
                 this._layers);
           } else if (this.viewMode === DiffViewMode.UNIFIED) {
-            builder = new GrDiffBuilderUnified(diff,
+            builder = new GrDiffBuilderUnified(diff, prefs, this.diffElement,
-                this._commentThreadElements, prefs, this.diffElement,
                 this._layers);
           }
           if (!builder) {

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js

@@ -20,60 +20,6 @@
   // Prevent redefinition.
   if (window.GrDiffBuilder) { return; }
 
-  /** @enum {string} */
-  Gerrit.DiffSide = {
-    LEFT: 'left',
-    RIGHT: 'right',
-    BOTH: 'both',
-  };
-
-  /**
-   * @param {!Array<!HTMLElement>} threadEls
-   * @param {!{beforeNumber: (number|string|undefined),
-   *           afterNumber: (number|string|undefined)}}
-   *     lineInfo
-   * @param {!Gerrit.DiffSide=} side The side (LEFT, RIGHT, BOTH) for
-   *     which to return the threads (default: BOTH).
-   * @return {!Array<!HTMLElement>} The thread elements matching the given
-   *     location.
-   */
-  Gerrit.filterThreadElsForLocation = function(
-      threadEls, lineInfo, side = Gerrit.DiffSide.BOTH) {
-    function matchesLeftLine(threadEl) {
-      return threadEl.getAttribute('comment-side') ==
-          Gerrit.DiffSide.LEFT &&
-          threadEl.getAttribute('line-num') == lineInfo.beforeNumber;
-    }
-    function matchesRightLine(threadEl) {
-      return threadEl.getAttribute('comment-side') ==
-          Gerrit.DiffSide.RIGHT &&
-          threadEl.getAttribute('line-num') == lineInfo.afterNumber;
-    }
-    function matchesFileComment(threadEl) {
-      return (side === Gerrit.DiffSide.BOTH ||
-              threadEl.getAttribute('comment-side') == side) &&
-            // line/range comments have 1-based line set, if line is falsy it's
-            // a file comment
-            !threadEl.getAttribute('line-num');
-    }
-
-    // Select the appropriate matchers for the desired side and line
-    // If side is BOTH, we want both the left and right matcher.
-    const matchers = [];
-    if (side !== Gerrit.DiffSide.RIGHT) {
-      matchers.push(matchesLeftLine);
-    }
-    if (side !== Gerrit.DiffSide.LEFT) {
-      matchers.push(matchesRightLine);
-    }
-    if (lineInfo.afterNumber === 'FILE' ||
-        lineInfo.beforeNumber === 'FILE') {
-      matchers.push(matchesFileComment);
-    }
-    return threadEls.filter(threadEl =>
-        matchers.some(matcher => matcher(threadEl)));
-  };
-
   /**
    * In JS, unicode code points above 0xFFFF occupy two elements of a string.
    * For example '𐀏'.length is 2. An occurence of such a code point is called a
@@ -96,8 +42,7 @@
    */
   const REGEX_TAB_OR_SURROGATE_PAIR = /\t|[\uD800-\uDBFF][\uDC00-\uDFFF]/;
 
-  function GrDiffBuilder(diff, commentThreadEls, prefs,
+  function GrDiffBuilder(diff, prefs, outputEl, layers) {
-      outputEl, layers) {
     this._diff = diff;
     this._prefs = prefs;
     this._outputEl = outputEl;
@@ -119,8 +64,6 @@
         layer.addListener(this._handleLayerUpdate.bind(this));
       }
     }
-
-    this._threadEls = commentThreadEls;
   }
 
   GrDiffBuilder.GroupType = {
@@ -373,31 +316,6 @@
     return button;
   };
 
-  /**
-   * @param {!GrDiffLine} line
-   * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which
-   *     to return the thread group (default: BOTH).
-   * @return {!Object}
-   */
-  GrDiffBuilder.prototype._commentThreadGroupForLine = function(
-      line, commentSide = GrDiffBuilder.Side.BOTH) {
-    const threadElsForGroup =
-        Gerrit.filterThreadElsForLocation(this._threadEls, line, commentSide);
-    if (!threadElsForGroup || threadElsForGroup.length === 0) {
-      return null;
-    }
-
-    const threadGroupEl = document.createElement('div');
-    threadGroupEl.className = 'thread-group';
-    for (const threadEl of threadElsForGroup) {
-      Polymer.dom(threadGroupEl).appendChild(threadEl);
-    }
-    if (commentSide) {
-      threadGroupEl.setAttribute('data-side', commentSide);
-    }
-    return threadGroupEl;
-  };
-
   GrDiffBuilder.prototype._createLineEl = function(
       line, number, type, opt_class) {
     const td = this._createElement('td');

polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html

@@ -75,70 +75,11 @@ limitations under the License.
         show_tabs: true,
         tab_size: 4,
       };
-      builder = new GrDiffBuilder({content: []}, [], prefs);
+      builder = new GrDiffBuilder({content: []}, prefs);
     });
 
     teardown(() => { sandbox.restore(); });
 
-    test('filterThreadElsForLocation with no threads', () => {
-      const line = {beforeNumber: 3, afterNumber: 5};
-
-      const threads = [];
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threads, line), []);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threads, line,
-          Gerrit.DiffSide.LEFT), []);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threads, line,
-          Gerrit.DiffSide.RIGHT), []);
-    });
-
-    test('filterThreadElsForLocation for line comments', () => {
-      const line = {beforeNumber: 3, afterNumber: 5};
-
-      const l3 = document.createElement('div');
-      l3.setAttribute('line-num', 3);
-      l3.setAttribute('comment-side', 'left');
-
-      const l5 = document.createElement('div');
-      l5.setAttribute('line-num', 5);
-      l5.setAttribute('comment-side', 'left');
-
-      const r3 = document.createElement('div');
-      r3.setAttribute('line-num', 3);
-      r3.setAttribute('comment-side', 'right');
-
-      const r5 = document.createElement('div');
-      r5.setAttribute('line-num', 5);
-      r5.setAttribute('comment-side', 'right');
-
-      const threadEls = [l3, l5, r3, r5];
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line),
-          [l3, r5]);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
-          Gerrit.DiffSide.LEFT), [l3]);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
-          Gerrit.DiffSide.RIGHT), [r5]);
-    });
-
-    test('filterThreadElsForLocation for file comments', () => {
-      const line = {beforeNumber: 'FILE', afterNumber: 'FILE'};
-
-      const l = document.createElement('div');
-      l.setAttribute('comment-side', 'left');
-
-      const r = document.createElement('div');
-      r.setAttribute('comment-side', 'right');
-
-      const threadEls = [l, r];
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line),
-          [l, r]);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
-          Gerrit.DiffSide.BOTH), [l, r]);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
-          Gerrit.DiffSide.LEFT), [l]);
-      assert.deepEqual(Gerrit.filterThreadElsForLocation(threadEls, line,
-          Gerrit.DiffSide.RIGHT), [r]);
-    });
-
     test('_createElement classStr applies all classes', () => {
       const node = builder._createElement('div', 'test classes');
       assert.isTrue(node.classList.contains('gr-diff'));
@@ -312,73 +253,6 @@ limitations under the License.
       }
     });
 
-    test('comment thread group creation', () => {
-      const l3 = document.createElement('div');
-      l3.className = 'comment-thread';
-      l3.setAttribute('comment-side', 'left');
-      l3.setAttribute('line-num', 3);
-
-      const l5 = document.createElement('div');
-      l5.className = 'comment-thread';
-      l5.setAttribute('comment-side', 'left');
-      l5.setAttribute('line-num', 5);
-
-      const r5 = document.createElement('div');
-      r5.className = 'comment-thread';
-      r5.setAttribute('comment-side', 'right');
-      r5.setAttribute('line-num', 5);
-
-      builder = new GrDiffBuilder({content: []}, [l3, l5, r5], prefs);
-
-      function checkThreadGroupProps(threadGroupEl,
-          expectedThreadEls) {
-        const threadEls = Polymer.dom(threadGroupEl).queryDistributedElements(
-            '.comment-thread');
-        assert.equal(threadEls.length, expectedThreadEls.length);
-        for (let i=0; i<expectedThreadEls.length; i++) {
-          assert.equal(threadEls[i], expectedThreadEls[i]);
-        }
-      }
-
-      let line = new GrDiffLine(GrDiffLine.Type.BOTH);
-      line.beforeNumber = 5;
-      line.afterNumber = 5;
-      let threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
-      threadGroupEl =
-          builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
-      checkThreadGroupProps(threadGroupEl, [r5]);
-
-      threadGroupEl =
-          builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
-      checkThreadGroupProps(threadGroupEl, [l5]);
-
-      threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
-      threadEl =
-          builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
-      checkThreadGroupProps(threadEl, [l5]);
-
-      threadGroupEl =
-          builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
-      checkThreadGroupProps(threadGroupEl, [r5]);
-
-      line = new GrDiffLine(GrDiffLine.Type.REMOVE);
-      line.beforeNumber = 5;
-      line.afterNumber = 5;
-      threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, [l5, r5]);
-
-      line = new GrDiffLine(GrDiffLine.Type.ADD);
-      line.beforeNumber = 3;
-      line.afterNumber = 5;
-      threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, [l3, r5]);
-    });
-
-
     test('_handlePreferenceError called with invalid preference', () => {
       sandbox.stub(element, '_handlePreferenceError');
       const prefs = {tab_size: 0};
@@ -913,7 +787,7 @@ limitations under the License.
         outputEl = element.queryEffectiveChildren('#diffTable');
         keyLocations = {left: {}, right: {}};
         sandbox.stub(element, '_getDiffBuilder', () => {
-          const builder = new GrDiffBuilder({content}, [], prefs, outputEl);
+          const builder = new GrDiffBuilder({content}, prefs, outputEl);
           sandbox.stub(builder, 'addColumns');
           builder.buildSectionElement = function(group) {
             const section = document.createElement('stub');

polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js

@@ -74,8 +74,16 @@
       this.debounce('selectionChange', this._handleSelection, 200);
     },
 
+    _getThreadEl(e) {
+      const path = Polymer.dom(e).path || [];
+      for (const pathEl of path) {
+        if (pathEl.classList.contains('comment-thread')) return pathEl;
+      }
+      return null;
+    },
+
     _handleCommentThreadMouseenter(e) {
-      const threadEl = Polymer.dom(e).localTarget;
+      const threadEl = this._getThreadEl(e);
       const index = this._indexForThreadEl(threadEl);
 
       if (index !== undefined) {
@@ -84,7 +92,7 @@
     },
 
     _handleCommentThreadMouseleave(e) {
-      const threadEl = Polymer.dom(e).localTarget;
+      const threadEl = this._getThreadEl(e);
       const index = this._indexForThreadEl(threadEl);
 
       if (index !== undefined) {

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

@@ -199,6 +199,7 @@ limitations under the License.
 
       test('comment-thread-mouseenter from line comments is ignored', () => {
         const threadEl = document.createElement('div');
+        threadEl.className = 'comment-thread';
         threadEl.setAttribute('comment-side', 'right');
         threadEl.setAttribute('line-num', 3);
         element.appendChild(threadEl);
@@ -212,6 +213,7 @@ limitations under the License.
 
       test('comment-thread-mouseenter from ranged comment causes set', () => {
         const threadEl = document.createElement('div');
+        threadEl.className = 'comment-thread';
         threadEl.setAttribute('comment-side', 'right');
         threadEl.setAttribute('line-num', 3);
         threadEl.setAttribute('range', JSON.stringify({
@@ -239,6 +241,7 @@ limitations under the License.
 
       test('comment-thread-mouseleave from line comments is ignored', () => {
         const threadEl = document.createElement('div');
+        threadEl.className = 'comment-thread';
         threadEl.setAttribute('comment-side', 'right');
         threadEl.setAttribute('line-num', 3);
         element.appendChild(threadEl);

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js

@@ -63,6 +63,12 @@
         a.end_character === b.end_character;
   }
 
+  /** @enum {string} */
+  Gerrit.DiffSide = {
+    LEFT: 'left',
+    RIGHT: 'right',
+  };
+
   /**
    * Wrapper around gr-diff.
    *
@@ -196,11 +202,6 @@
         type: Number,
         computed: '_computeParentIndex(patchRange.*)',
       },
-
-      _threadEls: {
-        type: Array,
-        value: () => [],
-      },
     },
 
     behaviors: [
@@ -344,7 +345,7 @@
      * @return {!Array<!HTMLElement>}
      */
     getThreadEls() {
-      return this._threadEls;
+      return Polymer.dom(this.$.diff).querySelectorAll('.comment-thread');
     },
 
     /** @param {HTMLElement} el */
@@ -471,7 +472,6 @@
       return isImageDiff(diff);
     },
 
-
     _commentsChanged(newComments) {
       const allComments = [];
       for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) {
@@ -590,21 +590,20 @@
     },
 
     _attachThreadElement(threadEl) {
-      this._threadEls.push(threadEl);
       Polymer.dom(this.$.diff).appendChild(threadEl);
     },
 
     _clearThreads() {
-      for (const threadEl of this._threadEls) {
+      for (const threadEl of this.getThreadEls()) {
         const parent = Polymer.dom(threadEl).parentNode;
         Polymer.dom(parent).removeChild(threadEl);
       }
-      this._threadEls = [];
     },
 
     _createThreadElement(thread) {
       const threadEl = document.createElement('gr-diff-comment-thread');
       threadEl.className = 'comment-thread';
+      threadEl.slot = `${thread.commentSide}-${thread.lineNum}`;
       threadEl.comments = thread.comments;
       threadEl.commentSide = thread.commentSide;
       threadEl.isOnParent = !!thread.isOnParent;
@@ -625,10 +624,6 @@
         const parent = Polymer.dom(threadEl).parentNode;
         Polymer.dom(parent).removeChild(threadEl);
 
-        const i = this._threadEls.findIndex(
-            threadEl => threadEl.rootId == e.detail.rootId);
-        this._threadEls.splice(i, 1);
-
         threadEl.removeEventListener('root-id-changed', rootIdChangedListener);
         threadEl.removeEventListener('thread-discard', threadDiscardListener);
       };
@@ -660,11 +655,56 @@
         return rangesEqual(threadRange, range);
       }
 
-      const filteredThreadEls = Gerrit.filterThreadElsForLocation(
+      const filteredThreadEls = this._filterThreadElsForLocation(
-          this._threadEls, line, commentSide).filter(matchesRange);
+          this.getThreadEls(), line, commentSide).filter(matchesRange);
       return filteredThreadEls.length ? filteredThreadEls[0] : null;
     },
 
+    /**
+     * @param {!Array<!HTMLElement>} threadEls
+     * @param {!{beforeNumber: (number|string|undefined|null),
+     *           afterNumber: (number|string|undefined|null)}}
+     *     lineInfo
+     * @param {!Gerrit.DiffSide=} side The side (LEFT, RIGHT) for
+     *     which to return the threads.
+     * @return {!Array<!HTMLElement>} The thread elements matching the given
+     *     location.
+     */
+    _filterThreadElsForLocation(threadEls, lineInfo, side) {
+      function matchesLeftLine(threadEl) {
+        return threadEl.getAttribute('comment-side') ==
+            Gerrit.DiffSide.LEFT &&
+            threadEl.getAttribute('line-num') == lineInfo.beforeNumber;
+      }
+      function matchesRightLine(threadEl) {
+        return threadEl.getAttribute('comment-side') ==
+            Gerrit.DiffSide.RIGHT &&
+            threadEl.getAttribute('line-num') == lineInfo.afterNumber;
+      }
+      function matchesFileComment(threadEl) {
+        return threadEl.getAttribute('comment-side') == side &&
+              // line/range comments have 1-based line set, if line is falsy it's
+              // a file comment
+              !threadEl.getAttribute('line-num');
+      }
+
+      // Select the appropriate matchers for the desired side and line
+      // If side is BOTH, we want both the left and right matcher.
+      const matchers = [];
+      if (side !== Gerrit.DiffSide.RIGHT) {
+        matchers.push(matchesLeftLine);
+      }
+      if (side !== Gerrit.DiffSide.LEFT) {
+        matchers.push(matchesRightLine);
+      }
+      if (lineInfo.afterNumber === 'FILE' ||
+          lineInfo.beforeNumber === 'FILE') {
+        matchers.push(matchesFileComment);
+      }
+      return threadEls.filter(threadEl =>
+          matchers.some(matcher => matcher(threadEl)));
+    },
+
     /**
      * Take a diff that was loaded with a ignore-whitespace other than
      * IGNORE_NONE, and convert delta chunks labeled as common into shared

polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html

@@ -769,10 +769,11 @@ limitations under the License.
       });
     });
 
-    test('getThreadEls() returns _threadEls', () => {
+    test('getThreadEls() returns .comment-threads', () => {
-      const returnValue = [document.createElement('b')];
+      const threadEl = document.createElement('div');
-      element._threadEls = returnValue;
+      threadEl.className = 'comment-thread';
-      assert.equal(element.getThreadEls(), returnValue);
+      Polymer.dom(element.$.diff).appendChild(threadEl);
+      assert.deepEqual(element.getThreadEls(), [threadEl]);
     });
 
     test('delegates addDraftAtLine(el)', () => {
@@ -1159,6 +1160,65 @@ limitations under the License.
       assert.equal(threads[1].patchNum, 3);
     });
 
+    test('_filterThreadElsForLocation with no threads', () => {
+      const line = {beforeNumber: 3, afterNumber: 5};
+
+      const threads = [];
+      assert.deepEqual(element._filterThreadElsForLocation(threads, line), []);
+      assert.deepEqual(element._filterThreadElsForLocation(threads, line,
+          Gerrit.DiffSide.LEFT), []);
+      assert.deepEqual(element._filterThreadElsForLocation(threads, line,
+          Gerrit.DiffSide.RIGHT), []);
+    });
+
+    test('_filterThreadElsForLocation for line comments', () => {
+      const line = {beforeNumber: 3, afterNumber: 5};
+
+      const l3 = document.createElement('div');
+      l3.setAttribute('line-num', 3);
+      l3.setAttribute('comment-side', 'left');
+
+      const l5 = document.createElement('div');
+      l5.setAttribute('line-num', 5);
+      l5.setAttribute('comment-side', 'left');
+
+      const r3 = document.createElement('div');
+      r3.setAttribute('line-num', 3);
+      r3.setAttribute('comment-side', 'right');
+
+      const r5 = document.createElement('div');
+      r5.setAttribute('line-num', 5);
+      r5.setAttribute('comment-side', 'right');
+
+      const threadEls = [l3, l5, r3, r5];
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line),
+          [l3, r5]);
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+          Gerrit.DiffSide.LEFT), [l3]);
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+          Gerrit.DiffSide.RIGHT), [r5]);
+    });
+
+    test('_filterThreadElsForLocation for file comments', () => {
+      const line = {beforeNumber: 'FILE', afterNumber: 'FILE'};
+
+      const l = document.createElement('div');
+      l.setAttribute('comment-side', 'left');
+
+      const r = document.createElement('div');
+      r.setAttribute('comment-side', 'right');
+
+      const threadEls = [l, r];
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line),
+          [l, r]);
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+          Gerrit.DiffSide.BOTH), [l, r]);
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+          Gerrit.DiffSide.LEFT), [l]);
+      assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
+          Gerrit.DiffSide.RIGHT), [r]);
+    });
+
     suite('_translateChunksToIgnore', () => {
       let content;
 

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html

@@ -294,7 +294,6 @@ limitations under the License.
               is-image-diff="[[isImageDiff]]"
               base-image="[[baseImage]]"
               revision-image="[[revisionImage]]">
-            <slot></slot>
             <table
                 id="diffTable"
                 class$="[[_diffTableClass]]"

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js

@@ -60,6 +60,20 @@
         node.classList.contains('comment-thread');
   }
 
+  /**
+   * Turn a slot element into the corresponding content element.
+   * Slots are only fully supported in Polymer 2 - in Polymer 1, they are
+   * replaced with content elements during template parsing. This conversion is
+   * not applied for imperatively created slot elements, so this method
+   * implements the same behavior as the template parsing for imperative slots.
+   */
+  Gerrit.slotToContent = function(slot) {
+    const content = document.createElement('content');
+    content.name = slot.name;
+    content.setAttribute('select', `[slot='${slot.name}']`);
+    return content;
+  };
+
   Polymer({
     is: 'gr-diff',
 
@@ -455,14 +469,16 @@
      * Gets or creates a comment thread group for a specific line and side on a
      * diff.
      * @param {!Object} contentEl
+     * @param {!Gerrit.DiffSide} commentSide
      * @return {!Node}
      */
-    _getOrCreateThreadGroup(contentEl) {
+    _getOrCreateThreadGroup(contentEl, commentSide) {
       // Check if thread group exists.
       let threadGroupEl = this._getThreadGroupForLine(contentEl);
       if (!threadGroupEl) {
         threadGroupEl = document.createElement('div');
         threadGroupEl.className = 'thread-group';
+        threadGroupEl.setAttribute('data-side', commentSide);
         contentEl.appendChild(threadGroupEl);
       }
       return threadGroupEl;
@@ -624,8 +640,17 @@
               lineNumString, commentSide);
           const contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
           const contentEl = contentText.parentElement;
-          const threadGroupEl = this._getOrCreateThreadGroup(contentEl);
+          const threadGroupEl = this._getOrCreateThreadGroup(
-          Polymer.dom(threadGroupEl).appendChild(threadEl);
+              contentEl, commentSide);
+          // Create a slot for the thread and attach it to the thread group.
+          // The Polyfill has some bugs and this only works if the slot is
+          // attached to the group after the group is attached to the DOM.
+          // The thread group may already have a slot with the right name, but
+          // that is okay because the first matching slot is used and the rest
+          // are ignored.
+          const slot = document.createElement('slot');
+          slot.name = threadEl.slot;
+          Polymer.dom(threadGroupEl).appendChild(Gerrit.slotToContent(slot));
         }
       });
     },