From 180eb167077e405efe6428f006f3dc635ddef5a5 Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Fri, 25 Aug 2017 14:37:13 -0700
Subject: [PATCH] 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
---
 .../gr-change-actions/gr-change-actions.html  |  5 +-
 .../gr-change-actions/gr-change-actions.js    | 13 +++
 .../gr-change-actions_test.html               | 14 +++
 .../change/gr-change-view/gr-change-view.html | 15 ++-
 .../change/gr-change-view/gr-change-view.js   | 15 +++
 .../gr-change-view/gr-change-view_test.html   | 43 +++++++++
 .../shared/gr-overlay/gr-overlay.html         | 10 ++
 .../elements/shared/gr-overlay/gr-overlay.js  | 40 ++++++++
 .../shared/gr-overlay/gr-overlay_test.html    | 91 +++++++++++++++++++
 9 files changed, 241 insertions(+), 5 deletions(-)
 create mode 100644 polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay_test.html

diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
index 18ba163d97..f2ed33acfb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
@@ -74,9 +74,12 @@ limitations under the License.
           margin: .5em;
           text-align: center;
         }
+        #mainContent.mobileOverlayOpened {
+          display: none;
+        }
       }
     </style>
-    <div>
+    <div id="mainContent">
       <span
           id="actionLoadingMessage"
           hidden$="[[!_actionLoadingMessage]]">
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index 4691836932..3bccdd87fc 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -291,6 +291,11 @@
       '_actionsChanged(actions.*, revisionActions.*, _additionalActions.*)',
     ],
 
+    listeners: {
+      'fullscreen-overlay-opened': '_handleHideBackgroundContent',
+      'fullscreen-overlay-closed': '_handleShowBackgroundContent',
+    },
+
     ready() {
       this.$.jsAPI.addElement(this.$.jsAPI.Element.CHANGE_ACTIONS, this);
       this._loading = false;
@@ -939,6 +944,14 @@
       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
      * values.
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 2390c217a7..55cd9829eb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -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', () => {
       let fireActionStub;
 
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 0290cb1d2e..700f835a33 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
@@ -326,13 +326,19 @@ limitations under the License.
         .scrollable {
           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>
     <div class="container loading" hidden$="[[!_loading]]">Loading...</div>
     <div
+        id="mainContent"
         class$="container [[_computeEditLoadedClass(_editLoaded)]]"
         hidden$="{{_loading}}">
-      <div class$="[[_computeHeaderClass(_change)]]">
+      <div class$="hideOnMobileOverlay [[_computeHeaderClass(_change)]]">
         <span class="header-title">
           <gr-change-star
               id="changeStar"
@@ -360,7 +366,7 @@ limitations under the License.
         </span>
       </div>
       <section class="changeInfo">
-        <div class="changeInfo-column changeMetadata">
+        <div class="changeInfo-column changeMetadata hideOnMobileOverlay">
           <gr-change-metadata
               change="{{_change}}"
               commit-info="[[_commitInfo]]"
@@ -394,7 +400,7 @@ limitations under the License.
                 on-download-tap="_handleDownloadTap"></gr-change-actions>
           </div>
           <hr class="mobile">
-          <div id="commitAndRelated">
+          <div id="commitAndRelated" class="hideOnMobileOverlay">
             <div class="commitContainer">
               <div
                   id="commitMessage"
@@ -459,7 +465,7 @@ limitations under the License.
           </div>
         </div>
       </section>
-      <section class$="patchInfo [[_computePatchInfoClass(_patchRange.patchNum,
+      <section class$="patchInfo hideOnMobileOverlay [[_computePatchInfoClass(_patchRange.patchNum,
           _allPatchSets)]]">
         <div class="patchInfo-header">
           <div class="patchInfo-header-wrapper">
@@ -537,6 +543,7 @@ limitations under the License.
             file-list-increment="{{_numFilesShown}}"></gr-file-list>
       </section>
       <gr-messages-list id="messageList"
+          class="hideOnMobileOverlay"
           change-num="[[_changeNum]]"
           messages="[[_change.messages]]"
           reviewer-updates="[[_change.reviewer_updates]]"
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 edf7dd3aaf..83c4d8087e 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
@@ -204,6 +204,13 @@
 
     listeners: {
       '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: [
       '_labelsChanged(_change.labels.*)',
@@ -420,6 +427,14 @@
       }
     },
 
+    _handleHideBackgroundContent() {
+      this.$.mainContent.classList.add('overlayOpen');
+    },
+
+    _handleShowBackgroundContent() {
+      this.$.mainContent.classList.remove('overlayOpen');
+    },
+
     _handleReplySent(e) {
       this.$.replyOverlay.close();
       this._reload();
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 9bf7a7d42f..57b3e760ad 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
@@ -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', () => {
         const handleExpand =
             sandbox.stub(element.$.messageList, 'handleExpandCollapse');
diff --git a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.html b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.html
index 419d2f7e4e..1b59d355da 100644
--- a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.html
+++ b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.html
@@ -25,6 +25,16 @@ limitations under the License.
         background: #fff;
         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>
     <content></content>
   </template>
diff --git a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js
index 8db800464d..ebf2f027c1 100644
--- a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js
+++ b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js
@@ -16,21 +16,61 @@
 
   const AWAIT_MAX_ITERS = 10;
   const AWAIT_STEP = 5;
+  const BREAKPOINT_FULLSCREEN_OVERLAY = '50em';
 
   Polymer({
     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: [
       Polymer.IronOverlayBehavior,
     ],
 
+    listeners: {
+      'iron-overlay-closed': '_close',
+      'iron-overlay-cancelled': '_close',
+    },
+
     open(...args) {
       return new Promise(resolve => {
         Polymer.IronOverlayBehaviorImpl.open.apply(this, args);
+        if (this._isMobile()) {
+          this.fire('fullscreen-overlay-opened');
+          this._fullScreenOpen = true;
+        }
         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.
      */
diff --git a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay_test.html b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay_test.html
new file mode 100644
index 0000000000..3f427ca211
--- /dev/null
+++ b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay_test.html
@@ -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>