Remove jank in reply dialog

Most of the issues in the previous reply dialog related to the fact
that:

1) The background content was taller than the viewport.
2) Background scrolling is possible with the dialog open.

This change addresses this issue by hiding the bulk of change view
content when any change view dialog is opened, and then re-displaying
it when the dialog is closed, and letting the dialog take up the entire
screen on smaller devices.

Bug: Issue 7080, Issue 7070
Change-Id: I5834fc61e8daeee2972e8dca4fad9ac54bf9ebca
This commit is contained in:
Becky Siegel 2017-08-25 14:37:13 -07:00
parent e657f82676
commit 180eb16707
9 changed files with 241 additions and 5 deletions

View File

@ -74,9 +74,12 @@ limitations under the License.
margin: .5em; margin: .5em;
text-align: center; text-align: center;
} }
#mainContent.mobileOverlayOpened {
display: none;
}
} }
</style> </style>
<div> <div id="mainContent">
<span <span
id="actionLoadingMessage" id="actionLoadingMessage"
hidden$="[[!_actionLoadingMessage]]"> hidden$="[[!_actionLoadingMessage]]">

View File

@ -291,6 +291,11 @@
'_actionsChanged(actions.*, revisionActions.*, _additionalActions.*)', '_actionsChanged(actions.*, revisionActions.*, _additionalActions.*)',
], ],
listeners: {
'fullscreen-overlay-opened': '_handleHideBackgroundContent',
'fullscreen-overlay-closed': '_handleShowBackgroundContent',
},
ready() { ready() {
this.$.jsAPI.addElement(this.$.jsAPI.Element.CHANGE_ACTIONS, this); this.$.jsAPI.addElement(this.$.jsAPI.Element.CHANGE_ACTIONS, this);
this._loading = false; this._loading = false;
@ -939,6 +944,14 @@
this._fireAction('/wip', this.actions.wip, false); this._fireAction('/wip', this.actions.wip, false);
}, },
_handleHideBackgroundContent() {
this.$.mainContent.classList.add('overlayOpen');
},
_handleShowBackgroundContent() {
this.$.mainContent.classList.remove('overlayOpen');
},
/** /**
* Merge sources of change actions into a single ordered array of action * Merge sources of change actions into a single ordered array of action
* values. * values.

View File

@ -346,6 +346,20 @@ limitations under the License.
}); });
}); });
test('fullscreen-overlay-opened hides content', () => {
sandbox.spy(element, '_handleHideBackgroundContent');
element.$.overlay.fire('fullscreen-overlay-opened');
assert.isTrue(element._handleHideBackgroundContent.called);
assert.isTrue(element.$.mainContent.classList.contains('overlayOpen'));
});
test('fullscreen-overlay-closed shows content', () => {
sandbox.spy(element, '_handleShowBackgroundContent');
element.$.overlay.fire('fullscreen-overlay-closed');
assert.isTrue(element._handleShowBackgroundContent.called);
assert.isFalse(element.$.mainContent.classList.contains('overlayOpen'));
});
suite('cherry-pick', () => { suite('cherry-pick', () => {
let fireActionStub; let fireActionStub;

View File

@ -326,13 +326,19 @@ limitations under the License.
.scrollable { .scrollable {
overflow: auto; overflow: auto;
} }
/* Change actions are the only thing thant need to remain visible due
to the fact that they may have the currently visible overlay open. */
#mainContent.overlayOpen .hideOnMobileOverlay {
display: none;
}
} }
</style> </style>
<div class="container loading" hidden$="[[!_loading]]">Loading...</div> <div class="container loading" hidden$="[[!_loading]]">Loading...</div>
<div <div
id="mainContent"
class$="container [[_computeEditLoadedClass(_editLoaded)]]" class$="container [[_computeEditLoadedClass(_editLoaded)]]"
hidden$="{{_loading}}"> hidden$="{{_loading}}">
<div class$="[[_computeHeaderClass(_change)]]"> <div class$="hideOnMobileOverlay [[_computeHeaderClass(_change)]]">
<span class="header-title"> <span class="header-title">
<gr-change-star <gr-change-star
id="changeStar" id="changeStar"
@ -360,7 +366,7 @@ limitations under the License.
</span> </span>
</div> </div>
<section class="changeInfo"> <section class="changeInfo">
<div class="changeInfo-column changeMetadata"> <div class="changeInfo-column changeMetadata hideOnMobileOverlay">
<gr-change-metadata <gr-change-metadata
change="{{_change}}" change="{{_change}}"
commit-info="[[_commitInfo]]" commit-info="[[_commitInfo]]"
@ -394,7 +400,7 @@ limitations under the License.
on-download-tap="_handleDownloadTap"></gr-change-actions> on-download-tap="_handleDownloadTap"></gr-change-actions>
</div> </div>
<hr class="mobile"> <hr class="mobile">
<div id="commitAndRelated"> <div id="commitAndRelated" class="hideOnMobileOverlay">
<div class="commitContainer"> <div class="commitContainer">
<div <div
id="commitMessage" id="commitMessage"
@ -459,7 +465,7 @@ limitations under the License.
</div> </div>
</div> </div>
</section> </section>
<section class$="patchInfo [[_computePatchInfoClass(_patchRange.patchNum, <section class$="patchInfo hideOnMobileOverlay [[_computePatchInfoClass(_patchRange.patchNum,
_allPatchSets)]]"> _allPatchSets)]]">
<div class="patchInfo-header"> <div class="patchInfo-header">
<div class="patchInfo-header-wrapper"> <div class="patchInfo-header-wrapper">
@ -537,6 +543,7 @@ limitations under the License.
file-list-increment="{{_numFilesShown}}"></gr-file-list> file-list-increment="{{_numFilesShown}}"></gr-file-list>
</section> </section>
<gr-messages-list id="messageList" <gr-messages-list id="messageList"
class="hideOnMobileOverlay"
change-num="[[_changeNum]]" change-num="[[_changeNum]]"
messages="[[_change.messages]]" messages="[[_change.messages]]"
reviewer-updates="[[_change.reviewer_updates]]" reviewer-updates="[[_change.reviewer_updates]]"

View File

@ -204,6 +204,13 @@
listeners: { listeners: {
'topic-changed': '_handleTopicChanged', 'topic-changed': '_handleTopicChanged',
// When an overlay is opened in a mobile viewport, the overlay has a full
// screen view. When it has a full screen view, we do not want the
// background to be scrollable. This will eliminate background scroll by
// hiding most of the contents on the screen upon opening, and showing
// again upon closing.
'fullscreen-overlay-opened': '_handleHideBackgroundContent',
'fullscreen-overlay-closed': '_handleShowBackgroundContent',
}, },
observers: [ observers: [
'_labelsChanged(_change.labels.*)', '_labelsChanged(_change.labels.*)',
@ -420,6 +427,14 @@
} }
}, },
_handleHideBackgroundContent() {
this.$.mainContent.classList.add('overlayOpen');
},
_handleShowBackgroundContent() {
this.$.mainContent.classList.remove('overlayOpen');
},
_handleReplySent(e) { _handleReplySent(e) {
this.$.replyOverlay.close(); this.$.replyOverlay.close();
this._reload(); this._reload();

View File

@ -121,6 +121,49 @@ limitations under the License.
}); });
}); });
test('fullscreen-overlay-opened hides content', () => {
element._loggedIn = true;
element._loading = false;
element._change = {
owner: {_account_id: 1},
labels: {},
actions: {
abandon: {
enabled: true,
label: 'Abandon',
method: 'POST',
title: 'Abandon',
},
},
};
sandbox.spy(element, '_handleHideBackgroundContent');
element.$.replyDialog.fire('fullscreen-overlay-opened');
assert.isTrue(element._handleHideBackgroundContent.called);
assert.isTrue(element.$.mainContent.classList.contains('overlayOpen'));
assert.equal(getComputedStyle(element.$.actions).display, 'block');
});
test('fullscreen-overlay-closed shows content', () => {
element._loggedIn = true;
element._loading = false;
element._change = {
owner: {_account_id: 1},
labels: {},
actions: {
abandon: {
enabled: true,
label: 'Abandon',
method: 'POST',
title: 'Abandon',
},
},
};
sandbox.spy(element, '_handleShowBackgroundContent');
element.$.replyDialog.fire('fullscreen-overlay-closed');
assert.isTrue(element._handleShowBackgroundContent.called);
assert.isFalse(element.$.mainContent.classList.contains('overlayOpen'));
});
test('X should expand all messages', () => { test('X should expand all messages', () => {
const handleExpand = const handleExpand =
sandbox.stub(element.$.messageList, 'handleExpandCollapse'); sandbox.stub(element.$.messageList, 'handleExpandCollapse');

View File

@ -25,6 +25,16 @@ limitations under the License.
background: #fff; background: #fff;
box-shadow: rgba(0, 0, 0, 0.3) 0 1px 3px; box-shadow: rgba(0, 0, 0, 0.3) 0 1px 3px;
} }
@media screen and (max-width: 50em) {
:host {
height: 100%;
left: 0;
position: fixed;
right: 0;
top: 0;
}
}
</style> </style>
<content></content> <content></content>
</template> </template>

View File

@ -16,21 +16,61 @@
const AWAIT_MAX_ITERS = 10; const AWAIT_MAX_ITERS = 10;
const AWAIT_STEP = 5; const AWAIT_STEP = 5;
const BREAKPOINT_FULLSCREEN_OVERLAY = '50em';
Polymer({ Polymer({
is: 'gr-overlay', is: 'gr-overlay',
/**
* Fired when a fullscreen overlay is closed
*
* @event fullscreen-overlay-closed
*/
/**
* Fired when an overlay is opened in full screen mode
*
* @event fullscreen-overlay-opened
*/
properties: {
_fullScreenOpen: {
type: Boolean,
value: false,
},
},
behaviors: [ behaviors: [
Polymer.IronOverlayBehavior, Polymer.IronOverlayBehavior,
], ],
listeners: {
'iron-overlay-closed': '_close',
'iron-overlay-cancelled': '_close',
},
open(...args) { open(...args) {
return new Promise(resolve => { return new Promise(resolve => {
Polymer.IronOverlayBehaviorImpl.open.apply(this, args); Polymer.IronOverlayBehaviorImpl.open.apply(this, args);
if (this._isMobile()) {
this.fire('fullscreen-overlay-opened');
this._fullScreenOpen = true;
}
this._awaitOpen(resolve); this._awaitOpen(resolve);
}); });
}, },
_isMobile() {
return window.matchMedia(`(max-width: ${BREAKPOINT_FULLSCREEN_OVERLAY})`);
},
_close() {
if (this._fullScreenOpen) {
this.fire('fullscreen-overlay-closed');
this._fullScreenOpen = false;
}
},
/** /**
* Override the focus stops that iron-overlay-behavior tries to find. * Override the focus stops that iron-overlay-behavior tries to find.
*/ */

View File

@ -0,0 +1,91 @@
<!DOCTYPE html>
<!--
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.
-->
<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
<title>gr-overlay</title>
<script src="../../../bower_components/page/page.js"></script>
<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
<script src="../../../bower_components/web-component-tester/browser.js"></script>
<link rel="import" href="../../../test/common-test-setup.html"/>
<link rel="import" href="gr-overlay.html">
<script>void(0);</script>
<test-fixture id="basic">
<template>
<gr-overlay>
<div>content</div>
</gr-overlay>
</template>
</test-fixture>
<script>
suite('gr-overlay tests', () => {
let element;
let sandbox;
setup(() => {
sandbox = sinon.sandbox.create();
element = fixture('basic');
});
teardown(() => {
sandbox.restore();
});
test('events are fired on fullscreen view', done => {
sandbox.stub(element, '_isMobile').returns(true);
const openHandler = sandbox.stub();
const closeHandler = sandbox.stub();
element.addEventListener('fullscreen-overlay-opened', openHandler);
element.addEventListener('fullscreen-overlay-closed', closeHandler);
element.open().then(() => {
assert.isTrue(element._isMobile.called);
assert.isTrue(element._fullScreenOpen);
assert.isTrue(openHandler.called);
element._close();
assert.isFalse(element._fullScreenOpen);
assert.isTrue(closeHandler.called);
done();
});
});
test('events are not fired on desktop view', done => {
sandbox.stub(element, '_isMobile').returns(false);
const openHandler = sandbox.stub();
const closeHandler = sandbox.stub();
element.addEventListener('fullscreen-overlay-opened', openHandler);
element.addEventListener('fullscreen-overlay-closed', closeHandler);
element.open().then(() => {
assert.isTrue(element._isMobile.called);
assert.isFalse(element._fullScreenOpen);
assert.isFalse(openHandler.called);
element._close();
assert.isFalse(element._fullScreenOpen);
assert.isFalse(closeHandler.called);
done();
});
});
});
</script>