Fix an edge case where inline diffs would not expand

Bogus <gr-diff-host> elements can hang around invisibly associated with
the wrong files, so looking up the <gr-diff-host> element must take into
account that only visible elements should be considered.

Bug: Issue 12283
Change-Id: I221d6cc3b5d3195d049f600aba5c965e57dcfd00

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js

@@ -334,8 +334,12 @@
     }
 
     get diffs() {
-      return Array.from(
+      const diffs = Polymer.dom(this.root).querySelectorAll('gr-diff-host');
-          Polymer.dom(this.root).querySelectorAll('gr-diff-host'));
+      // It is possible that a bogus diff element is hanging around invisibly
+      // from earlier with a different patch set choice and associated with a
+      // different entry in the files array. So filter on visible items only.
+      return Array.from(diffs).filter(
+          el => !!el && !!el.style && el.style.display !== 'none');
     }
 
     openDiffPrefs() {
@@ -1074,6 +1078,10 @@
         console.log('Expanding diff', iter, 'of', initialCount, ':',
             path);
         const diffElem = this._findDiffByPath(path, diffElements);
+        if (!diffElem) {
+          console.warn(`Did not find <gr-diff-host> element for ${path}`);
+          return Promise.resolve();
+        }
         diffElem.comments = this.changeComments.getCommentsBySideForPath(
             path, this.patchRange, this.projectConfig);
         const promises = [diffElem.reload()];

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html

@@ -673,35 +673,48 @@ limitations under the License.
         });
 
         test('i key shows/hides selected inline diff', () => {
+          const paths = Object.keys(element._filesByPath);
           sandbox.stub(element, '_expandedPathsChanged');
           flushAsynchronousOperations();
           const files = Polymer.dom(element.root).querySelectorAll('.file-row');
           element.$.fileCursor.stops = files;
           element.$.fileCursor.setCursorAtIndex(0);
+          assert.equal(element.diffs.length, 0);
+          assert.equal(element._expandedFilePaths.length, 0);
+
           MockInteractions.keyUpOn(element, 73, null, 'i');
           flushAsynchronousOperations();
-          assert.include(element._expandedFilePaths, element.diffs[0].path);
+          assert.equal(element.diffs.length, 1);
+          assert.equal(element.diffs[0].path, paths[0]);
+          assert.equal(element._expandedFilePaths.length, 1);
+          assert.equal(element._expandedFilePaths[0], paths[0]);
+
           MockInteractions.keyUpOn(element, 73, null, 'i');
           flushAsynchronousOperations();
-          assert.notInclude(element._expandedFilePaths, element.diffs[0].path);
+          assert.equal(element.diffs.length, 0);
+          assert.equal(element._expandedFilePaths.length, 0);
+
           element.$.fileCursor.setCursorAtIndex(1);
           MockInteractions.keyUpOn(element, 73, null, 'i');
           flushAsynchronousOperations();
-          assert.include(element._expandedFilePaths, element.diffs[1].path);
+          assert.equal(element.diffs.length, 1);
+          assert.equal(element.diffs[0].path, paths[1]);
+          assert.equal(element._expandedFilePaths.length, 1);
+          assert.equal(element._expandedFilePaths[0], paths[1]);
 
           MockInteractions.keyUpOn(element, 73, 'shift', 'i');
           flushAsynchronousOperations();
+          assert.equal(element.diffs.length, paths.length);
+          assert.equal(element._expandedFilePaths.length, paths.length);
           for (const index in element.diffs) {
             if (!element.diffs.hasOwnProperty(index)) { continue; }
             assert.include(element._expandedFilePaths, element.diffs[index].path);
           }
+
           MockInteractions.keyUpOn(element, 73, 'shift', 'i');
           flushAsynchronousOperations();
-          for (const index in element.diffs) {
+          assert.equal(element.diffs.length, 0);
-            if (!element.diffs.hasOwnProperty(index)) { continue; }
+          assert.equal(element._expandedFilePaths.length, 0);
-            assert.notInclude(element._expandedFilePaths,
-                element.diffs[index].path);
-          }
         });
 
         test('r key toggles reviewed flag', () => {
@@ -1075,6 +1088,7 @@ limitations under the License.
         const path = 'path/to/my/file.txt';
         const diffs = [{
           path,
+          style: {},
           reload() {
             done();
           },
@@ -1131,18 +1145,21 @@ limitations under the License.
         let callCount = 0;
         const diffs = [{
           path: 'p0',
+          style: {},
           reload() {
             assert.equal(callCount++, 2);
             return Promise.resolve();
           },
         }, {
           path: 'p1',
+          style: {},
           reload() {
             assert.equal(callCount++, 1);
             return Promise.resolve();
           },
         }, {
           path: 'p2',
+          style: {},
           reload() {
             assert.equal(callCount++, 0);
             return Promise.resolve();
@@ -1162,6 +1179,7 @@ limitations under the License.
         let callCount = 0;
         const diffs = [{
           path: 'p0',
+          style: {},
           reload() {
             assert.equal(reviewStub.callCount, 2);
             assert.equal(callCount++, 2);
@@ -1169,6 +1187,7 @@ limitations under the License.
           },
         }, {
           path: 'p1',
+          style: {},
           reload() {
             assert.equal(reviewStub.callCount, 1);
             assert.equal(callCount++, 1);
@@ -1176,6 +1195,7 @@ limitations under the License.
           },
         }, {
           path: 'p2',
+          style: {},
           reload() {
             assert.equal(reviewStub.callCount, 0);
             assert.equal(callCount++, 0);
@@ -1195,6 +1215,7 @@ limitations under the License.
         const reviewStub = sandbox.stub(element, '_reviewFile');
         const diffs = [{
           path: 'p',
+          style: {},
           reload() { return Promise.resolve(); },
         }];