Remove <gr-fixed-panel> in favor of position:sticky
<gr-fixed-panel> was a super complex attempt at doing what
position:sticky does out of the box. Presumably <gr-fixed-panel> was
added before all modern browers had decent support of position:sticky.
The desire to remove this was triggered by observing buggy positioning
in diff view: "File" is half obscured when you load Gerrit with a diff
page directly. And this bug could be traced down to <gr-fixed-panel>
not updating the height in the right moment.
Stickiness of the main header did currently not work anyway, and IMO is
also not desirable. So we only have stickiness of the file header on the
two file pages (diff-view and editor-view).
The one thing that position:sticky cannot do is to change its appearance
based on whether it is currently floating or not. So we cannot retain
the old behavior of showing the box-shadow only in floating mode. But
arguably the box-shadow is actually also nice to have in normal mode.
This was previously attempted in change 143855, but we believe that
with top:0 everything is just working fine.
Screenshots: https://imgur.com/a/PaE4epn
Change-Id: I421941c12023deea9f7d7f9886810ff0e9a7f7e7
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
index 9d68ba3..b206f22 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
@@ -101,6 +101,9 @@
_listeningForScroll: Boolean,
/**
+ * TODO(dhruvsri): <gr-fixed-panel> is gone.
+ * Maybe scrollTopMargin can be removed??
+ *
* gr-diff-view has gr-fixed-panel on top. The panel can
* intersect a main element and partially hides a content of
* the main element. To correctly calculates visibility of an
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index 72a1b6d..dd6638e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -20,7 +20,6 @@
import '../../shared/gr-button/gr-button.js';
import '../../shared/gr-dropdown/gr-dropdown.js';
import '../../shared/gr-dropdown-list/gr-dropdown-list.js';
-import '../../shared/gr-fixed-panel/gr-fixed-panel.js';
import '../../shared/gr-icons/gr-icons.js';
import '../../shared/gr-rest-api-interface/gr-rest-api-interface.js';
import '../../shared/gr-select/gr-select.js';
@@ -211,10 +210,6 @@
type: Object,
computed: '_computeCommentSkips(_commentMap, _fileList, _path)',
},
- _panelFloatingDisabled: {
- type: Boolean,
- value: () => window.PANEL_FLOATING_DISABLED,
- },
_editMode: {
type: Boolean,
computed: '_computeEditMode(_patchRange.*)',
@@ -238,6 +233,9 @@
},
/**
+ * TODO(dhruvsri): <gr-fixed-panel> is gone.
+ * Maybe _scrollTopMargin can be removed??
+ *
* gr-diff-view has gr-fixed-panel on top. The panel can
* intersect a main element and partially hides a content of
* the main element. To correctly calculates visibility of an
@@ -1365,10 +1363,8 @@
return skips;
}
- _computeDiffClass(panelFloatingDisabled) {
- if (panelFloatingDisabled) {
- return 'noOverflow';
- }
+ _computeContainerClass(editMode) {
+ return editMode ? 'editMode' : '';
}
/**
@@ -1379,13 +1375,6 @@
return patchNumEquals(patchRange.patchNum, SPECIAL_PATCH_SET_NUM.EDIT);
}
- /**
- * @param {boolean} editMode
- */
- _computeContainerClass(editMode) {
- return editMode ? 'editMode' : '';
- }
-
_computeBlameToggleLabel(loaded, loading) {
if (loaded) { return 'Hide blame'; }
return 'Show blame';
@@ -1571,10 +1560,6 @@
this._getDiffPreferences();
}
- _onChangeHeaderPanelHeightChanged(e) {
- this._scrollTopMargin = e.detail.value;
- }
-
_computeCanEdit(loggedIn, changeChangeRecord) {
if ([changeChangeRecord, changeChangeRecord.base]
.some(arg => arg === undefined)) {
@@ -1583,6 +1568,10 @@
return loggedIn && changeIsOpen(changeChangeRecord.base);
}
+ _computeIsLoggedIn(loggedIn) {
+ return loggedIn ? true : false;
+ }
+
/**
* Wrapper for using in the element template and computed properties
*/
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index e7e477c..64d5e06 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -19,6 +19,7 @@
export const htmlTemplate = html`
<style include="shared-styles">
:host {
+ display: block;
background-color: var(--view-background-color);
}
.hidden {
@@ -33,10 +34,17 @@
border-bottom: 1px solid var(--border-color);
}
}
- gr-fixed-panel {
+ .stickyHeader {
background-color: var(--view-background-color);
border-bottom: 1px solid var(--border-color);
+ position: sticky;
+ top: 0;
+ /* TODO(dhruvsri): This is required only because of 'position:relative' in
+ <gr-diff-highlight> (which could maybe be removed??). */
z-index: 1;
+ box-shadow: var(--elevation-level-1);
+ /* This is just for giving the box-shadow some space. */
+ margin-bottom: 2px;
}
header,
.subHeader {
@@ -46,7 +54,6 @@
}
header {
padding: var(--spacing-s) var(--spacing-xl);
- border-bottom: 1px solid var(--border-color);
}
.changeNumberColon {
color: transparent;
@@ -202,13 +209,7 @@
--gr-comment-thread-display: none;
}
</style>
- <gr-fixed-panel
- class$="[[_computeContainerClass(_editMode)]]"
- floating-disabled="[[_panelFloatingDisabled]]"
- keep-on-scroll=""
- ready-for-measure="[[!_loading]]"
- on-floating-height-changed="_onChangeHeaderPanelHeightChanged"
- >
+ <div class$="stickyHeader [[_computeContainerClass(_editMode)]]">
<header>
<div>
<a
@@ -381,13 +382,12 @@
></a
>
</div>
- </gr-fixed-panel>
+ </div>
<div class="loading" hidden$="[[!_loading]]">Loading...</div>
<gr-diff-host
id="diffHost"
hidden=""
hidden$="[[_loading]]"
- class$="[[_computeDiffClass(_panelFloatingDisabled)]]"
is-image-diff="{{_isImageDiff}}"
files-weblinks="{{_filesWeblinks}}"
diff="{{_diff}}"
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js
index d1677bd..058f476 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js
@@ -18,7 +18,6 @@
import '../../plugins/gr-endpoint-param/gr-endpoint-param.js';
import '../../shared/gr-button/gr-button.js';
import '../../shared/gr-editable-label/gr-editable-label.js';
-import '../../shared/gr-fixed-panel/gr-fixed-panel.js';
import '../../shared/gr-rest-api-interface/gr-rest-api-interface.js';
import '../../shared/gr-storage/gr-storage.js';
import '../gr-default-editor/gr-default-editor.js';
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_html.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_html.ts
index bd8304f..1297c97 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_html.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_html.ts
@@ -21,9 +21,11 @@
:host {
background-color: var(--view-background-color);
}
- gr-fixed-panel {
+ .stickyHeader {
background-color: var(--edit-mode-background-color);
border-bottom: 1px var(--border-color) solid;
+ position: sticky;
+ top: 0;
z-index: 1;
}
header,
@@ -76,7 +78,7 @@
}
}
</style>
- <gr-fixed-panel keep-on-scroll="">
+ <div class="stickyHeader">
<header>
<span class="controlGroup">
<span>Edit mode</span>
@@ -102,7 +104,7 @@
>
</span>
</header>
- </gr-fixed-panel>
+ </div>
<div class="textareaWrapper">
<gr-endpoint-decorator id="editorEndpoint" name="editor">
<gr-endpoint-param
diff --git a/polygerrit-ui/app/elements/gr-app-element.js b/polygerrit-ui/app/elements/gr-app-element.js
index 41802c2..6ae5245 100644
--- a/polygerrit-ui/app/elements/gr-app-element.js
+++ b/polygerrit-ui/app/elements/gr-app-element.js
@@ -37,7 +37,6 @@
import './settings/gr-cla-view/gr-cla-view.js';
import './settings/gr-registration-dialog/gr-registration-dialog.js';
import './settings/gr-settings-view/gr-settings-view.js';
-import './shared/gr-fixed-panel/gr-fixed-panel.js';
import './shared/gr-lib-loader/gr-lib-loader.js';
import './shared/gr-rest-api-interface/gr-rest-api-interface.js';
import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners.js';
@@ -428,7 +427,6 @@
this.$.registrationOverlay.refit();
});
}
- this.$.header.unfloat();
}
_handleShortcutTriggered(event) {
diff --git a/polygerrit-ui/app/elements/gr-app-element_html.ts b/polygerrit-ui/app/elements/gr-app-element_html.ts
index 66624e3..e76316c2 100644
--- a/polygerrit-ui/app/elements/gr-app-element_html.ts
+++ b/polygerrit-ui/app/elements/gr-app-element_html.ts
@@ -24,13 +24,6 @@
flex-direction: column;
min-height: 100%;
}
- gr-fixed-panel {
- /**
- * This one should be greater that the z-index in gr-diff-view
- * because gr-main-header contains overlay.
- */
- z-index: 10;
- }
gr-main-header,
footer {
color: var(--primary-text-color);
@@ -99,16 +92,14 @@
}
</style>
<gr-endpoint-decorator name="banner"></gr-endpoint-decorator>
- <gr-fixed-panel id="header">
- <gr-main-header
- id="mainHeader"
- search-query="{{params.query}}"
- on-mobile-search="_mobileSearchToggle"
- on-show-keyboard-shortcuts="handleShowKeyboardShortcuts"
- login-url="[[_loginUrl]]"
- >
- </gr-main-header>
- </gr-fixed-panel>
+ <gr-main-header
+ id="mainHeader"
+ search-query="{{params.query}}"
+ on-mobile-search="_mobileSearchToggle"
+ on-show-keyboard-shortcuts="handleShowKeyboardShortcuts"
+ login-url="[[_loginUrl]]"
+ >
+ </gr-main-header>
<main>
<gr-smart-search
id="search"
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
deleted file mode 100644
index bc79737..0000000
--- a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
+++ /dev/null
@@ -1,242 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * 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.
- */
-import '../../../styles/shared-styles.js';
-import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners.js';
-import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js';
-import {PolymerElement} from '@polymer/polymer/polymer-element.js';
-import {htmlTemplate} from './gr-fixed-panel_html.js';
-
-/** @extends PolymerElement */
-class GrFixedPanel extends GestureEventListeners(
- LegacyElementMixin(
- PolymerElement)) {
- static get template() { return htmlTemplate; }
-
- static get is() { return 'gr-fixed-panel'; }
-
- static get properties() {
- return {
- floatingDisabled: {
- type: Boolean,
- value: false,
- },
- readyForMeasure: {
- type: Boolean,
- observer: '_readyForMeasureObserver',
- },
- keepOnScroll: {
- type: Boolean,
- value: false,
- },
- _isMeasured: {
- type: Boolean,
- value: false,
- },
-
- /**
- * Initial offset from the top of the document, in pixels.
- */
- _topInitial: Number,
-
- /**
- * Current offset from the top of the window, in pixels.
- */
- _topLast: Number,
-
- _headerHeight: Number,
- _headerFloating: {
- type: Boolean,
- value: false,
- },
- _observer: {
- type: Object,
- value: null,
- },
- /**
- * If place before any other content defines how much
- * of the content below it is covered by this panel
- */
- floatingHeight: {
- type: Number,
- value: 0,
- notify: true,
- },
-
- _webComponentsReady: Boolean,
- };
- }
-
- static get observers() {
- return [
- '_updateFloatingHeight(floatingDisabled, _isMeasured, _headerHeight)',
- ];
- }
-
- _updateFloatingHeight(floatingDisabled, isMeasured, headerHeight) {
- if ([
- floatingDisabled,
- isMeasured,
- headerHeight,
- ].some(arg => arg === undefined)) {
- return;
- }
- this.floatingHeight =
- (!floatingDisabled && isMeasured) ? headerHeight : 0;
- }
-
- /** @override */
- attached() {
- super.attached();
- if (this.floatingDisabled) {
- return;
- }
- // Enable content measure unless blocked by param.
- if (this.readyForMeasure !== false) {
- this.readyForMeasure = true;
- }
- this.listen(window, 'resize', 'update');
- this.listen(window, 'scroll', '_updateOnScroll');
- this._observer = new MutationObserver(this.update.bind(this));
- this._observer.observe(this.$.header, {childList: true, subtree: true});
- }
-
- /** @override */
- detached() {
- super.detached();
- this.unlisten(window, 'scroll', '_updateOnScroll');
- this.unlisten(window, 'resize', 'update');
- if (this._observer) {
- this._observer.disconnect();
- }
- }
-
- _readyForMeasureObserver(readyForMeasure) {
- if (readyForMeasure) {
- this.update();
- }
- }
-
- _computeHeaderClass(headerFloating, topLast) {
- const fixedAtTop = this.keepOnScroll && topLast === 0;
- return [
- headerFloating ? 'floating' : '',
- fixedAtTop ? 'fixedAtTop' : '',
- ].join(' ');
- }
-
- unfloat() {
- if (this.floatingDisabled) {
- return;
- }
- this.$.header.style.top = '';
- this._headerFloating = false;
- this.updateStyles({'--header-height': ''});
- }
-
- update() {
- this.debounce('update', () => {
- this._updateDebounced();
- }, 100);
- }
-
- _updateOnScroll() {
- this.debounce('update', () => {
- this._updateDebounced();
- });
- }
-
- _updateDebounced() {
- if (this.floatingDisabled) {
- return;
- }
- this._isMeasured = false;
- this._maybeFloatHeader();
- this._reposition();
- }
-
- _getElementTop() {
- return this.getBoundingClientRect().top;
- }
-
- _reposition() {
- if (!this._headerFloating) {
- return;
- }
- const header = this.$.header;
- // 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 && elemTop < 0) {
- // Should stick to the top.
- newTop = 0;
- } else {
- // 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) {
- header.style.top = '';
- } else {
- header.style.top = newTop + 'px';
- }
- this._topLast = newTop;
- }
- }
-
- _measure() {
- if (this._isMeasured) {
- return; // Already measured.
- }
- const rect = this.$.header.getBoundingClientRect();
- if (rect.height === 0 && rect.width === 0) {
- return; // Not ready for measurement yet.
- }
- const top = document.body.scrollTop + rect.top;
- this._topLast = top;
- this._headerHeight = rect.height;
- this._topInitial =
- this.getBoundingClientRect().top + document.body.scrollTop;
- this._isMeasured = true;
- }
-
- _isFloatingNeeded() {
- return this.keepOnScroll ||
- document.body.scrollWidth > document.body.clientWidth;
- }
-
- _maybeFloatHeader() {
- if (!this._isFloatingNeeded()) {
- return;
- }
- this._measure();
- if (this._isMeasured) {
- this._floatHeader();
- }
- }
-
- _floatHeader() {
- this.updateStyles({'--header-height': this._headerHeight + 'px'});
- this._headerFloating = true;
- }
-}
-
-customElements.define(GrFixedPanel.is, GrFixedPanel);
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_html.ts b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_html.ts
deleted file mode 100644
index ce475c6..0000000
--- a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_html.ts
+++ /dev/null
@@ -1,50 +0,0 @@
-/**
- * @license
- * Copyright (C) 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * 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.
- */
-import {html} from '@polymer/polymer/lib/utils/html-tag';
-
-export const htmlTemplate = html`
- <style include="shared-styles">
- :host {
- box-sizing: border-box;
- display: block;
- min-height: var(--header-height);
- position: relative;
- }
- header {
- background: inherit;
- border: inherit;
- display: inline;
- height: inherit;
- }
- .floating {
- left: 0;
- position: fixed;
- width: 100%;
- will-change: top;
- }
- .fixedAtTop {
- border-bottom: 1px solid #a4a4a4;
- box-shadow: var(--elevation-level-2);
- }
- </style>
- <header
- id="header"
- class$="[[_computeHeaderClass(_headerFloating, _topLast)]]"
- >
- <slot></slot>
- </header>
-`;
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.js b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.js
deleted file mode 100644
index b9378ba..0000000
--- a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.js
+++ /dev/null
@@ -1,99 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * 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.
- */
-
-import '../../../test/common-test-setup-karma.js';
-import './gr-fixed-panel.js';
-import {html} from '@polymer/polymer/lib/utils/html-tag.js';
-
-const basicFixture = fixtureFromTemplate(html`
-<gr-fixed-panel>
- <div style="height: 100px"></div>
- </gr-fixed-panel>
-`);
-
-suite('gr-fixed-panel', () => {
- let element;
-
- setup(() => {
- element = basicFixture.instantiate();
- element.readyForMeasure = true;
- });
-
- test('can be disabled with floatingDisabled', () => {
- element.floatingDisabled = true;
- sinon.stub(element, '_reposition');
- window.dispatchEvent(new CustomEvent('resize'));
- element.flushDebouncer('update');
- assert.isFalse(element._reposition.called);
- });
-
- test('header is the height of the content', () => {
- assert.equal(element.getBoundingClientRect().height, 100);
- });
-
- test('scroll triggers _reposition', () => {
- sinon.stub(element, '_reposition');
- window.dispatchEvent(new CustomEvent('scroll'));
- element.flushDebouncer('update');
- assert.isTrue(element._reposition.called);
- });
-
- suite('_reposition', () => {
- const getHeaderTop = function() {
- return element.$.header.style.top;
- };
-
- const emulateScrollY = function(distance) {
- element._getElementTop.returns(element._headerTopInitial - distance);
- element._updateDebounced();
- element.flushDebouncer('scroll');
- };
-
- setup(() => {
- element._headerTopInitial = 10;
- sinon.stub(element, '_getElementTop')
- .returns(element._headerTopInitial);
- });
-
- test('scrolls header along with document', () => {
- emulateScrollY(20);
- // No top property is set when !_headerFloating.
- assert.equal(getHeaderTop(), '');
- });
-
- test('does not stick to the top by default', () => {
- emulateScrollY(150);
- // No top property is set when !_headerFloating.
- assert.equal(getHeaderTop(), '');
- });
-
- test('sticks to the top if enabled', () => {
- element.keepOnScroll = true;
- emulateScrollY(120);
- assert.equal(getHeaderTop(), '0px');
- });
-
- test('drops a shadow when fixed to the top', () => {
- element.keepOnScroll = true;
- emulateScrollY(5);
- assert.isFalse(element.$.header.classList.contains('fixedAtTop'));
- emulateScrollY(120);
- assert.isTrue(element.$.header.classList.contains('fixedAtTop'));
- });
- });
-});
-
