From 7416b235e4b0b202b010b4e8b847bdb609b13fe2 Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Thu, 27 Oct 2016 18:52:56 -0700
Subject: [PATCH] Sync patch set dropdowns on navigation

Previously, on a changelist or file view, if you change the patchset
to a lower number and then click the back button, dropdown value did not
change properly. The issue was related to how the selected option was
rendered in a dom-if. This change refactors the gr-select element to fit this
use case.

Bug: Issue 4800
Change-Id: I66ee5b7aba5421a01ca79f9e72a853a73c32e589
---
 .../change/gr-change-view/gr-change-view.html | 23 ++++++++----
 .../change/gr-change-view/gr-change-view.js   | 16 +++++---
 .../gr-change-view/gr-change-view_test.html   | 22 +++++------
 .../change/gr-file-list/gr-file-list.html     | 24 ++++++++----
 .../change/gr-file-list/gr-file-list.js       | 14 ++++---
 .../gr-file-list/gr-file-list_test.html       |  2 +-
 .../gr-patch-range-select.html                | 13 +++----
 .../gr-patch-range-select.js                  | 37 +++++++++++++------
 .../gr-patch-range-select_test.html           |  4 ++
 .../elements/shared/gr-select/gr-select.js    | 33 +++++++----------
 10 files changed, 111 insertions(+), 77 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index ecd26ad8c0..38c48c55f8 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -18,6 +18,7 @@ limitations under the License.
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
+<link rel="import" href="../../shared/gr-select/gr-select.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../../shared/gr-change-star/gr-change-star.html">
 <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
@@ -206,7 +207,7 @@ limitations under the License.
         }
       }
     </style>
-    <div class="container loading" hidden$="{{!_loading}}">Loading...</div>
+    <div class="container loading" hidden$="[[!_loading]]">Loading...</div>
     <div class="container" hidden$="{{_loading}}">
       <div class="header">
         <span class="header-title">
@@ -231,7 +232,7 @@ limitations under the License.
           <div id="change_plugins"></div>
         </div>
         <div class="changeInfo-column mainChangeInfo">
-          <div class="commitActions" hidden$="[[!_loggedIn]]"">
+          <div class="commitActions" hidden$="[[!_loggedIn]]">
             <gr-button
                 class="reply"
                 secondary
@@ -261,18 +262,24 @@ limitations under the License.
             <div class="relatedChanges">
               <gr-related-changes-list id="relatedChanges"
                   change="[[_change]]"
-                  patch-num="[[_computeLatestPatchNum(_allPatchSets)]]"></gr-related-changes-list>
+                  patch-num="[[_computeLatestPatchNum(_allPatchSets)]]">
+              </gr-related-changes-list>
             </div>
           </div>
         </div>
       </section>
-      <section class$="patchInfo [[_computePatchInfoClass(_patchRange.patchNum, _allPatchSets)]]">
+      <section class$="patchInfo [[_computePatchInfoClass(_patchRange.patchNum,
+          _allPatchSets)]]">
         <div class="patchInfo-header">
           <div>
-            <label class="patchSelectLabel" for="patchSetSelect">Patch set</label>
-            <select id="patchSetSelect" on-change="_handlePatchChange">
-              <template is="dom-repeat" items="[[_allPatchSets]]" as="patchNumber">
-                <option value$="[[patchNumber]]" selected$="[[_computePatchIndexIsSelected(index, _patchRange.patchNum)]]">
+            <label class="patchSelectLabel" for="patchSetSelect">
+              Patch set
+            </label>
+            <select id="patchSetSelect" bind-value="{{_selectedPatchSet}}"
+                is="gr-select" on-change="_handlePatchChange">
+              <template is="dom-repeat" items="[[_allPatchSets]]"
+                  as="patchNumber">
+                <option value$="[[patchNumber]]">
                   <span>[[patchNumber]]</span>
                   /
                   <span>[[_computeLatestPatchNum(_allPatchSets)]]</span>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index f6ecc9ea53..bac25bee2b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -73,7 +73,10 @@
         type: String,
         value: '',
       },
-      _patchRange: Object,
+      _patchRange: {
+        type: Object,
+        observer: '_updateSelected',
+      },
       _allPatchSets: {
         type: Array,
         computed: '_computeAllPatchSets(_change)',
@@ -89,6 +92,7 @@
         value: 'Reply',
         computed: '_computeReplyButtonLabel(_diffDrafts.*)',
       },
+      _selectedPatchSet: String,
       _initialLoadComplete: {
         type: Boolean,
         value: false,
@@ -458,6 +462,8 @@
           this._patchRange.patchNum ||
               this._computeLatestPatchNum(this._allPatchSets));
 
+      this._updateSelected();
+
       var title = change.subject + ' (' + change.change_id.substr(0, 9) + ')';
       this.fire('title-change', {title: title});
     },
@@ -528,10 +534,6 @@
       }
     },
 
-    _computePatchIndexIsSelected: function(index, patchNum) {
-      return this._allPatchSets[index] == patchNum;
-    },
-
     _computeLabelNames: function(labels) {
       return Object.keys(labels).sort();
     },
@@ -776,5 +778,9 @@
         this.$.fileList.reload(),
       ]);
     },
+
+    _updateSelected: function() {
+      this._selectedPatchSet = this._patchRange.patchNum;
+    },
   });
 })();
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index 8358e40280..e61d6b0b1a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -277,12 +277,10 @@ limitations under the License.
       var optionEls = Polymer.dom(element.root).querySelectorAll(
           '.patchInfo-header option');
       assert.equal(optionEls.length, 4);
-      assert.isFalse(element.$$('.patchInfo-header option[value="1"]')
-          .hasAttribute('selected'));
-      assert.isTrue(element.$$('.patchInfo-header option[value="2"]')
-          .hasAttribute('selected'));
-      assert.isFalse(element.$$('.patchInfo-header option[value="3"]')
-          .hasAttribute('selected'));
+      var select = element.$$('.patchInfo-header #patchSetSelect').bindValue;
+      assert.notEqual(select, 1);
+      assert.equal(select, 2);
+      assert.notEqual(select, 3);
       assert.equal(optionEls[3].value, 13);
 
       var showStub = sandbox.stub(page, 'show');
@@ -329,12 +327,12 @@ limitations under the License.
       var optionEls = Polymer.dom(element.root).querySelectorAll(
           '.patchInfo-header option');
       assert.equal(optionEls.length, 4);
-      assert.isFalse(element.$$('.patchInfo-header option[value="1"]')
-          .hasAttribute('selected'));
-      assert.isTrue(element.$$('.patchInfo-header option[value="2"]')
-          .hasAttribute('selected'));
-      assert.isFalse(element.$$('.patchInfo-header option[value="3"]')
-          .hasAttribute('selected'));
+      assert.notEqual(
+        element.$$('.patchInfo-header #patchSetSelect').bindValue, 1);
+      assert.equal(
+        element.$$('.patchInfo-header #patchSetSelect').bindValue, 2);
+      assert.notEqual(
+        element.$$('.patchInfo-header #patchSetSelect').bindValue, 3);
       assert.equal(optionEls[3].value, 13);
 
       var showStub = sandbox.stub(page, 'show');
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 007c0fc9f1..f11cca245c 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -22,6 +22,7 @@ limitations under the License.
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../../shared/gr-linked-text/gr-linked-text.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+<link rel="import" href="../../shared/gr-select/gr-select.html">
 
 <dom-module id="gr-file-list">
   <template>
@@ -182,13 +183,17 @@ limitations under the License.
         <span class="separator">/</span>
         <label>
           Diff against
-          <select id="patchChange" on-change="_handlePatchChange">
+          <select id="patchChange" bind-value="{{_diffAgainst}}" is="gr-select"
+              on-change="_handlePatchChange">
             <option value="PARENT">Base</option>
-            <template is="dom-repeat" items="[[_computePatchSets(revisions, patchRange.*)]]" as="patchNum">
-              <option
-                  value$="[[patchNum]]"
-                  selected$="[[_computePatchSetSelected(patchNum, patchRange.basePatchNum)]]"
-                  disabled$="[[_computePatchSetDisabled(patchNum, patchRange.patchNum)]]">[[patchNum]]</option>
+            <template 
+                is="dom-repeat" 
+                items="[[_computePatchSets(revisions, patchRange.*)]]"
+                as="patchNum">
+              <option value$="[[patchNum]]" disabled$=
+                  "[[_computePatchSetDisabled(patchNum, patchRange.patchNum)]]">
+                [[patchNum]]
+              </option>
             </template>
           </select>
         </label>
@@ -207,7 +212,8 @@ limitations under the License.
         <div class$="[[_computeClass('status', file.__path)]]">
           [[_computeFileStatus(file.status)]]
         </div>
-        <a class="path" href$="[[_computeDiffURL(changeNum, patchRange, file.__path)]]">
+        <a class="path"
+            href$="[[_computeDiffURL(changeNum, patchRange, file.__path)]]">
           <div title$="[[_computeFileDisplayName(file.__path)]]">
             [[_computeFileDisplayName(file.__path)]]
           </div>
@@ -217,7 +223,9 @@ limitations under the License.
           </div>
         </a>
         <div class="comments">
-          <span class="drafts">[[_computeDraftsString(drafts, patchRange.patchNum, file.__path)]]</span>
+          <span class="drafts">
+            [[_computeDraftsString(drafts, patchRange.patchNum, file.__path)]]
+          </span>
           [[_computeCommentsString(comments, patchRange.patchNum, file.__path)]]
         </div>
         <div class$="[[_computeClass('stats', file.__path)]]">
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 340cb630fc..bf6dcf38ed 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -25,7 +25,10 @@
     is: 'gr-file-list',
 
     properties: {
-      patchRange: Object,
+      patchRange: {
+        type: Object,
+        observer: '_updateSelected',
+      },
       patchNum: String,
       changeNum: String,
       comments: Object,
@@ -58,6 +61,7 @@
         type: Array,
         value: function() { return []; },
       },
+      _diffAgainst: String,
       _diffPrefs: Object,
       _userPrefs: Object,
       _localPrefs: Object,
@@ -181,10 +185,6 @@
       return parseInt(patchNum, 10) >= parseInt(currentPatchNum, 10);
     },
 
-    _computePatchSetSelected: function(patchNum, basePatchNum) {
-      return parseInt(patchNum, 10) === parseInt(basePatchNum, 10);
-    },
-
     _handleHiddenChange: function(e) {
       var model = e.model;
       model.set('file.__expanded', !model.file.__expanded);
@@ -527,6 +527,10 @@
       this._numFilesShown = this._files.length;
     },
 
+    _updateSelected: function(patchRange) {
+      this._diffAgainst = patchRange.basePatchNum;
+    },
+
     /**
      * _getDiffViewMode: Get the diff view (side-by-side or unified) based on
      * the current state.
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index 0caa6bec35..b530baed53 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -436,7 +436,7 @@ limitations under the License.
       document.getElementById('blank').restore();
     });
 
-    test('show/hide diffs disabled for large amounds of files', function(done) {
+    test('show/hide diffs disabled for large amounts of files', function(done) {
       element._files = [];
       element.changeNum = '42';
       element.patchRange = {
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
index c4967034f7..54bccb3800 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
@@ -13,7 +13,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 -->
-
+<link rel="import" href="../../shared/gr-select/gr-select.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 
 <dom-module id="gr-patch-range-select">
@@ -28,12 +28,11 @@ limitations under the License.
     </style>
     Patch set:
     <span class="patchRange">
-      <select id="leftPatchSelect" on-change="_handlePatchChange">
-        <option value="PARENT"
-            selected$="[[_computeLeftSelected('PARENT', patchRange)]]">Base</option>
+      <select id="leftPatchSelect" bind-value="{{_leftSelected}}"
+          on-change="_handlePatchChange" is="gr-select">
+        <option value="PARENT">Base</option>
         <template is="dom-repeat" items="{{availablePatches}}" as="patchNum">
           <option value$="[[patchNum]]"
-              selected$="[[_computeLeftSelected(patchNum, patchRange)]]"
               disabled$="[[_computeLeftDisabled(patchNum, patchRange)]]">[[patchNum]]</option>
         </template>
       </select>
@@ -46,10 +45,10 @@ limitations under the License.
     </span>
     &rarr;
     <span class="patchRange">
-      <select id="rightPatchSelect" on-change="_handlePatchChange">
+      <select id="rightPatchSelect" bind-value="{{_rightSelected}}"
+          on-change="_handlePatchChange" is="gr-select">
         <template is="dom-repeat" items="{{availablePatches}}" as="patchNum">
           <option value$="[[patchNum]]"
-              selected$="[[_computeRightSelected(patchNum, patchRange)]]"
               disabled$="[[_computeRightDisabled(patchNum, patchRange)]]">[[patchNum]]</option>
         </template>
       </select>
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
index b50043e81c..350429f102 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
@@ -21,13 +21,23 @@
       availablePatches: Array,
       changeNum: String,
       filesWeblinks: Object,
-      patchRange: Object,
       path: String,
+      patchRange: {
+        type: Object,
+        observer: '_updateSelected'
+      },
+      _rightSelected: String,
+      _leftSelected: String,
+    },
+
+    _updateSelected: function() {
+      this._rightSelected = this.patchRange.patchNum;
+      this._leftSelected = this.patchRange.basePatchNum;
     },
 
     _handlePatchChange: function(e) {
-      var leftPatch = this.$.leftPatchSelect.value;
-      var rightPatch = this.$.rightPatchSelect.value;
+      var leftPatch = this._leftSelected;
+      var rightPatch = this._rightSelected;
       var rangeStr = rightPatch;
       if (leftPatch != 'PARENT') {
         rangeStr = leftPatch + '..' + rangeStr;
@@ -36,14 +46,6 @@
       e.target.blur();
     },
 
-    _computeLeftSelected: function(patchNum, patchRange) {
-      return patchNum == patchRange.basePatchNum;
-    },
-
-    _computeRightSelected: function(patchNum, patchRange) {
-      return patchNum == patchRange.patchNum;
-    },
-
     _computeLeftDisabled: function(patchNum, patchRange) {
       return parseInt(patchNum, 10) >= parseInt(patchRange.patchNum, 10);
     },
@@ -52,5 +54,18 @@
       if (patchRange.basePatchNum == 'PARENT') { return false; }
       return parseInt(patchNum, 10) <= parseInt(patchRange.basePatchNum, 10);
     },
+
+    // On page load, the dom-if for options getting added occurs after
+    // the value was set in the select. This ensures that after they
+    // are loaded, the correct value will get selected.  I attempted to
+    // debounce these, but because they are detecting two different
+    // events, sometimes the timing was off and one ended up missing.
+    _synchronizeSelectionRight: function() {
+      this.$.rightPatchSelect.value = this._rightSelected;
+    },
+
+    _synchronizeSelectionLeft: function() {
+      this.$.leftPatchSelect.value = this._leftSelected;
+    },
   });
 })();
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html
index 95789bcd45..68eeaa94c9 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html
@@ -67,6 +67,10 @@ limitations under the License.
       element.changeNum = '42';
       element.path = 'path/to/file.txt';
       element.availablePatches = ['1', '2', '3'];
+      element.patchRange = {
+        basePatchNum: 'PARENT',
+        patchNum: '3',
+      };
       flushAsynchronousOperations();
 
       var numEvents = 0;
diff --git a/polygerrit-ui/app/elements/shared/gr-select/gr-select.js b/polygerrit-ui/app/elements/shared/gr-select/gr-select.js
index 9e14f08114..bef260e965 100644
--- a/polygerrit-ui/app/elements/shared/gr-select/gr-select.js
+++ b/polygerrit-ui/app/elements/shared/gr-select/gr-select.js
@@ -16,40 +16,33 @@
 
   Polymer({
     is: 'gr-select',
-
     extends: 'select',
-
     properties: {
       bindValue: {
         type: String,
         notify: true,
+        observer: '_updateValue',
       },
     },
 
-    observers: [
-      '_valueChanged(bindValue)',
-    ],
+    listeners: {
+      change: '_valueChanged',
+      'dom-change': '_updateValue',
+    },
 
-    attached: function() {
-      this.addEventListener('change', function() {
-        this.bindValue = this.value;
-      });
+    _updateValue: function() {
+      if (this.bindValue) {
+        this.value = this.bindValue;
+      }
+    },
+
+    _valueChanged: function() {
+      this.bindValue = this.value;
     },
 
     ready: function() {
       // If not set via the property, set bind-value to the element value.
       if (!this.bindValue) { this.bindValue = this.value; }
     },
-
-    _valueChanged: function(bindValue) {
-      var options = Polymer.dom(this.root).querySelectorAll('option');
-      for (var i = 0; i < options.length; i++) {
-        if (options[i].getAttribute('value') === bindValue + '') {
-          options[i].setAttribute('selected', true);
-          this.value = bindValue;
-          break;
-        }
-      }
-    },
   });
 })();