From 6c16e97d520ea3ca312390994e87b79589471c95 Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Fri, 6 Apr 2018 21:22:23 -0700
Subject: [PATCH] Update gr-fixed-panel

Previously, when keepOnScroll was set to true, and the header was not
yet to the top, there was a miscalculation in the 'top' number, casuing
the header to scroll up higher than its gr-fixed-panel parent.

This occurred because calculations for '_topLast' became inaccurate, and
compounded on one another.

This change fixes the problem by utilizing the position of the
gr-fixed-panel element itself to get a top offset, since it is reatively
positioned, this can be computed.

gr-fixed-panel tests were not being run, and they were already failing.
The tests have been updated to relfect how they should behave.

Change-Id: If0156cc7b009498ee63dddf0cef1b7e8d786b17c
---
 .../shared/gr-fixed-panel/gr-fixed-panel.js   | 33 +++++++++----------
 .../gr-fixed-panel/gr-fixed-panel_test.html   | 11 ++++---
 polygerrit-ui/app/test/index.html             |  1 +
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
index ce430312dd..2c32709176 100644
--- a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
+++ b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
@@ -93,10 +93,6 @@
       ].join(' ');
     },
 
-    _getScrollY() {
-      return window.scrollY;
-    },
-
     unfloat() {
       if (this.floatingDisabled) {
         return;
@@ -127,26 +123,29 @@
       this._reposition();
     },
 
+    _getElementTop() {
+      return this.getBoundingClientRect().top;
+    },
+
     _reposition() {
       if (!this._headerFloating) {
         return;
       }
       const header = this.$.header;
-      const scrollY = this._topInitial - this._getScrollY();
+      // Since the outer element is relative positioned, can  use its top
+      // to determine how to position the inner header element.
+      const elemTop = this._getElementTop();
       let newTop;
-      if (this.keepOnScroll) {
-        if (scrollY > 0) {
-          // Reposition to imitate natural scrolling.
-          newTop = scrollY;
-        } else {
-          newTop = 0;
-        }
-      } else if (scrollY > -this._headerHeight ||
-          this._topLast < -this._headerHeight) {
-        // Allow to scroll away, but ignore when far behind the edge.
-        newTop = scrollY;
+      if (this.keepOnScroll && elemTop < 0) {
+        // Should stick to the top.
+        newTop = 0;
       } else {
-        newTop = -this._headerHeight;
+        // Keep in line with the outer element.
+        newTop = elemTop;
+      }
+      // Initialize top style if it doesn't exist yet.
+      if (!header.style.top && this._topLast === newTop) {
+        header.style.top = newTop;
       }
       if (this._topLast !== newTop) {
         if (newTop === undefined) {
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html
index ec3ebe2c80..9eac7f74fb 100644
--- a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html
@@ -74,24 +74,27 @@ limitations under the License.
       };
 
       const emulateScrollY = function(distance) {
-        element._getScrollY.returns(distance);
+        element._getElementTop.returns(element._headerTopInitial - distance);
         element._updateDebounced();
         element.flushDebouncer('scroll');
       };
 
       setup(() => {
         element._headerTopInitial = 10;
-        sandbox.stub(element, '_getScrollY').returns(0);
+        sandbox.stub(element, '_getElementTop')
+            .returns(element._headerTopInitial);
       });
 
       test('scrolls header along with document', () => {
         emulateScrollY(20);
-        assert.equal(getHeaderTop(), '-12px');
+        // No top property is set when !_headerFloating.
+        assert.equal(getHeaderTop(), '');
       });
 
       test('does not stick to the top by default', () => {
         emulateScrollY(150);
-        assert.equal(getHeaderTop(), '-100px');
+        // No top property is set when !_headerFloating.
+        assert.equal(getHeaderTop(), '');
       });
 
       test('sticks to the top if enabled', () => {
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index dbc7dc9ff6..d70c566a1d 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -162,6 +162,7 @@ limitations under the License.
     'shared/gr-js-api-interface/gr-js-api-interface_test.html',
     'shared/gr-js-api-interface/gr-plugin-endpoints_test.html',
     'shared/gr-js-api-interface/gr-plugin-rest-api_test.html',
+    'shared/gr-fixed-panel/gr-fixed-panel_test.html',
     'shared/gr-limited-text/gr-limited-text_test.html',
     'shared/gr-linked-chip/gr-linked-chip_test.html',
     'shared/gr-linked-text/gr-linked-text_test.html',