From cf12a39726c95c85c116201681ab77866be34c33 Mon Sep 17 00:00:00 2001
From: John Munoz <johnmunoz@google.com>
Date: Fri, 14 Dec 2018 14:34:30 -0800
Subject: [PATCH 1/3] Make modal width response

---
 .../components/LearnerViewDirective.js        | 15 +++++++-----
 .../MonospaceDisplayModalDirective.js         | 23 ++++++++++---------
 client/question/question.js                   |  2 +-
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/client/question/components/LearnerViewDirective.js b/client/question/components/LearnerViewDirective.js
index 3d759f05..253681ef 100644
--- a/client/question/components/LearnerViewDirective.js
+++ b/client/question/components/LearnerViewDirective.js
@@ -24,6 +24,10 @@ tie.directive('learnerView', [function() {
       <div class="tie-wrapper protractor-test-tie-wrapper" ng-class="{'night-mode': isDarkModeEnabled()}">
         <div class="tie-question-ui-outer">
           <div class="tie-question-ui-inner">
+            <monospace-display-modal
+                ng-show="MonospaceDisplayModalService.isDisplayed()"
+                content="content">
+            </monospace-display-modal>
             <div class="tie-question-ui protractor-test-question-ui">
               <div class="tie-question-window">
                 <div class="tie-question-container" ng-class="{'pulse-animation-enabled': pulseAnimationEnabled}" ng-attr-aria-hidden="{{MonospaceDisplayModalService.isDisplayed()}}">
@@ -49,10 +53,6 @@ tie.directive('learnerView', [function() {
                   </div>
                   <speech-balloons-container></speech-balloons-container>
                 </div>
-                <monospace-display-modal
-                    ng-show="MonospaceDisplayModalService.isDisplayed()"
-                    content="content">
-                </monospace-display-modal>
               </div>
               <div>
                 <select class="tie-select-menu protractor-test-theme-select"
@@ -470,11 +470,11 @@ tie.directive('learnerView', [function() {
           background-color: #333A42;
           color: #E0E0E0;
         }
-        .tie-question-window.tie-monospace-modal-container {
+        .tie-monospace-modal-container {
           border: 1px solid #d3d3d3;
           resize: none;
         }
-        .night-mode .tie-question-window.tie-monospace-modal-container {
+        .night-mode .tie-monospace-modal-container {
           border: 1px solid #333;
         }
         .tie-run-button {
@@ -589,6 +589,9 @@ tie.directive('learnerView', [function() {
             padding: 0;
             width: 662px;
           }
+          .tie-question-ui-inner .tie-monospace-modal-container {
+            width: 662px;
+          }
         }
       </style>
     `,
diff --git a/client/question/components/MonospaceDisplayModalDirective.js b/client/question/components/MonospaceDisplayModalDirective.js
index 81c7a898..06580be3 100644
--- a/client/question/components/MonospaceDisplayModalDirective.js
+++ b/client/question/components/MonospaceDisplayModalDirective.js
@@ -49,8 +49,8 @@ tie.directive('monospaceDisplayModal', [function() {
 
       <style>
         monospace-display-modal {
-          position: absolute;
-          transition: top 0.3s cubic-bezier(0.4, 0.0, 0.2, 1);
+          position: inherit;
+          flex: auto;
         }
         .tie-feedback-modal-displayed {
           top: 9px;
@@ -61,8 +61,13 @@ tie.directive('monospaceDisplayModal', [function() {
           display: table;
           height: calc(100% - 80px);
           position: absolute;
-          width: calc(100% - 783px);
+          width: calc(100% - 760px);
+          min-width: 300px;
+          max-width: 700px;
+          margin-left: 8px;
+          top: 6px;
           z-index: 100;
+          transition: top 0.3s cubic-bezier(0.4, 0.0, 0.2, 1);
         }
         .night-mode monospace-display-modal .tie-monospace-modal-container {
           border-color: #505050;
@@ -145,12 +150,9 @@ tie.directive('monospaceDisplayModal', [function() {
 
           var questionWindowDiv =
               document.getElementsByClassName('tie-question-window')[0];
-          var monospaceDisplayModalElement =
-              document.getElementsByTagName('monospace-display-modal')[0];
           var modalContainerDiv = document.getElementsByClassName(
               'tie-monospace-modal-container')[0];
 
-          var modalWidth = questionWindowDiv.offsetWidth;
           var modalHeight = questionWindowDiv.offsetHeight;
           var modalTopOffsetString =
               (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET).toString() + 'px';
@@ -158,19 +160,18 @@ tie.directive('monospaceDisplayModal', [function() {
               '-' + (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET +
               FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET).toString() + 'px';
 
-          modalContainerDiv.style.width = modalWidth.toString() + 'px';
           modalContainerDiv.style.height = modalTopOffsetString;
-          monospaceDisplayModalElement.style.top = modalHideTopOffsetString;
+          modalContainerDiv.style.top = modalHideTopOffsetString;
 
           $timeout(function() {
-            monospaceDisplayModalElement.style.top = "";
-            monospaceDisplayModalElement.classList.add(
+            modalContainerDiv.style.top = "";
+            modalContainerDiv.classList.add(
                 'tie-feedback-modal-displayed');
           }, 0);
         });
 
         /**
-         *  Close the modal.
+         * Close the modal.
          */
         $scope.closeModal = function() {
           var questionWindowDiv =
diff --git a/client/question/question.js b/client/question/question.js
index 4907ed89..85af3dba 100644
--- a/client/question/question.js
+++ b/client/question/question.js
@@ -858,7 +858,7 @@ tie.constant('GITHUB_HOMEPAGE_URL', 'http://google.github.io/tie');
  * @type {number}
  * @constant
  */
-var FEEDBACK_MODAL_HEIGHT_OFFSET = 66;
+var FEEDBACK_MODAL_HEIGHT_OFFSET = 69;
 tie.constant('FEEDBACK_MODAL_HEIGHT_OFFSET', FEEDBACK_MODAL_HEIGHT_OFFSET);
 
 /**

From 75565503bdc16236808f1fcf55849533982f319e Mon Sep 17 00:00:00 2001
From: John Munoz <johnmunoz@google.com>
Date: Fri, 14 Dec 2018 15:11:48 -0800
Subject: [PATCH 2/3] Fix broken modal dismissal.

---
 .../components/LearnerViewDirective.js        | 41 +++++++++++++++++--
 .../MonospaceDisplayModalDirective.js         | 28 ++++++++-----
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/client/question/components/LearnerViewDirective.js b/client/question/components/LearnerViewDirective.js
index 253681ef..33b5de5f 100644
--- a/client/question/components/LearnerViewDirective.js
+++ b/client/question/components/LearnerViewDirective.js
@@ -610,7 +610,8 @@ tie.directive('learnerView', [function() {
       'FEEDBACK_CATEGORIES', 'DEFAULT_EVENT_BATCH_PERIOD_SECONDS',
       'DELAY_STYLE_CHANGES', 'THEME_NAME_LIGHT', 'THEME_NAME_DARK',
       'CODE_RESET_CONFIRMATION_MESSAGE', 'PRIVACY_URL', 'ABOUT_TIE_URL',
-      'ABOUT_TIE_LABEL', 'TERMS_OF_USE_URL',
+      'ABOUT_TIE_LABEL', 'TERMS_OF_USE_URL', 'FEEDBACK_MODAL_HEIGHT_OFFSET',
+      'FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET',
       function(
           $scope, $interval, $timeout, $location, $window,
           ConversationManagerService, QuestionDataService, LANGUAGE_PYTHON,
@@ -626,7 +627,8 @@ tie.directive('learnerView', [function() {
           FEEDBACK_CATEGORIES, DEFAULT_EVENT_BATCH_PERIOD_SECONDS,
           DELAY_STYLE_CHANGES, THEME_NAME_LIGHT, THEME_NAME_DARK,
           CODE_RESET_CONFIRMATION_MESSAGE, PRIVACY_URL, ABOUT_TIE_URL,
-          ABOUT_TIE_LABEL, TERMS_OF_USE_URL) {
+          ABOUT_TIE_LABEL, TERMS_OF_USE_URL, FEEDBACK_MODAL_HEIGHT_OFFSET,
+          FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET) {
         $scope.PRIVACY_URL = PRIVACY_URL;
         $scope.ABOUT_TIE_URL = ABOUT_TIE_URL;
         $scope.ABOUT_TIE_LABEL = ABOUT_TIE_LABEL;
@@ -1180,13 +1182,46 @@ tie.directive('learnerView', [function() {
             tasks[currentTaskIndex].getId());
         };
 
+        /**
+         * Event handler to hide modal after transition animation ends.
+         */
+        $scope.hideModalAfterAnimation = function(event) {
+          $timeout(function() {
+            MonospaceDisplayModalService.hideModal();
+          }, 0);
+          event.target.removeEventListener("transitionend",
+              $scope.hideModalAfterAnimation, false);
+        };
+
+        /**
+         *  Close modal by calling MonospaceDisplayModalService.closeModal.
+         */
+        $scope.closeModal = function() {
+          questionWindowDiv =
+              document.getElementsByClassName('tie-question-window')[0];
+          var modalContainerDiv = document.getElementsByClassName(
+              'tie-monospace-modal-container')[0];
+          var modalHeight = questionWindowDiv.offsetHeight;
+          var modalHideTopOffsetString =
+              '-' + (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET +
+              FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET).toString() + 'px';
+
+          modalContainerDiv.style.top = modalHideTopOffsetString;
+          modalContainerDiv.classList.remove(
+              'tie-feedback-modal-displayed');
+          modalContainerDiv.addEventListener("transitionend",
+              $scope.hideModalAfterAnimation, false);
+        };
+
         /**
          * Calls the processes necessary to start the code submission process.
          *
          * @param {string} code
          */
         $scope.submitCode = function(code) {
-          MonospaceDisplayModalService.hideModal();
+          if (MonospaceDisplayModalService.isDisplayed()) {
+            $scope.closeModal();
+          }
           SessionHistoryService.addCodeBalloon(code);
 
           // Gather all tasks from the first one up to the current one.
diff --git a/client/question/components/MonospaceDisplayModalDirective.js b/client/question/components/MonospaceDisplayModalDirective.js
index 06580be3..c5af5d24 100644
--- a/client/question/components/MonospaceDisplayModalDirective.js
+++ b/client/question/components/MonospaceDisplayModalDirective.js
@@ -171,26 +171,34 @@ tie.directive('monospaceDisplayModal', [function() {
         });
 
         /**
-         * Close the modal.
+         * Event handler to hide modal after transition animation ends.
+         */
+        $scope.hideModalAfterAnimation = function(event) {
+          $timeout(function() {
+            MonospaceDisplayModalService.hideModal();
+          }, 0);
+          event.target.removeEventListener("transitionend",
+              $scope.hideModalAfterAnimation, false);
+        };
+
+        /**
+         *  Close modal by calling MonospaceDisplayModalService.closeModal.
          */
         $scope.closeModal = function() {
           var questionWindowDiv =
               document.getElementsByClassName('tie-question-window')[0];
-          var monospaceDisplayModalElement =
-              document.getElementsByTagName('monospace-display-modal')[0];
-
+          var modalContainerDiv = document.getElementsByClassName(
+              'tie-monospace-modal-container')[0];
           var modalHeight = questionWindowDiv.offsetHeight;
           var modalHideTopOffsetString =
               '-' + (modalHeight + FEEDBACK_MODAL_HEIGHT_OFFSET +
               FEEDBACK_MODAL_HIDE_HEIGHT_OFFSET).toString() + 'px';
 
-          monospaceDisplayModalElement.style.top = modalHideTopOffsetString;
-          monospaceDisplayModalElement.classList.remove(
+          modalContainerDiv.style.top = modalHideTopOffsetString;
+          modalContainerDiv.classList.remove(
               'tie-feedback-modal-displayed');
-
-          $timeout(function() {
-            MonospaceDisplayModalService.hideModal();
-          }, 0);
+          modalContainerDiv.addEventListener("transitionend",
+              $scope.hideModalAfterAnimation, false);
         };
       }
     ]

From e2b880084dde0644d7af6cce8fadbe8e951d90a7 Mon Sep 17 00:00:00 2001
From: John Munoz <johnmunoz@google.com>
Date: Tue, 8 Jan 2019 14:16:16 -0800
Subject: [PATCH 3/3] Added E2E tests.

---
 .../components/CodeSnippetDirective.js        |  2 +-
 .../components/ErrorSnippetDirective.js       |  2 +-
 .../components/OutputSnippetDirective.js      |  2 +-
 tests/e2e/question.pageObject.js              | 81 +++++++++++++++++++
 tests/e2e/question.spec.js                    | 29 +++++++
 5 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/client/question/components/CodeSnippetDirective.js b/client/question/components/CodeSnippetDirective.js
index 9f53a3d8..48c75c00 100644
--- a/client/question/components/CodeSnippetDirective.js
+++ b/client/question/components/CodeSnippetDirective.js
@@ -28,7 +28,7 @@ tie.directive('codeSnippet', [function() {
         <br>
       </span>
       <span ng-if="codeLines.length > MAX_NUM_LINES_IN_SNIPPET">
-        <a href ng-click="openCodeModal()" ng-if="!isModalOpen()">View full code</a>
+        <a class="tie-modal-link" href ng-click="openCodeModal()" ng-if="!isModalOpen()">View full code</a>
         <span ng-if="isModalOpen()">View full code</span>
       </span>
       <style>
diff --git a/client/question/components/ErrorSnippetDirective.js b/client/question/components/ErrorSnippetDirective.js
index 1326f499..a9b6842c 100644
--- a/client/question/components/ErrorSnippetDirective.js
+++ b/client/question/components/ErrorSnippetDirective.js
@@ -26,7 +26,7 @@ tie.directive('errorSnippet', [function() {
       <div>
         <p>
           If you cannot figure out the problem, you can click on
-          <a href ng-click="openSyntaxErrorModal()" ng-if="!isModalOpen()">this link</a>
+          <a class="tie-modal-link" href ng-click="openSyntaxErrorModal()" ng-if="!isModalOpen()">this link</a>
           <span ng-if="isModalOpen()">this link</span>
           to display the error message.
         </p>
diff --git a/client/question/components/OutputSnippetDirective.js b/client/question/components/OutputSnippetDirective.js
index 34bb7ac6..4043b380 100644
--- a/client/question/components/OutputSnippetDirective.js
+++ b/client/question/components/OutputSnippetDirective.js
@@ -25,7 +25,7 @@ tie.directive('outputSnippet', [function() {
     template: `
       <div class="output-toggle-container">
         You can click on
-        <a href ng-click="openOutputModal()" ng-if="!isModalOpen()"">this link</a>
+        <a class="tie-modal-link" href ng-click="openOutputModal()" ng-if="!isModalOpen()"">this link</a>
         <span ng-if="isModalOpen()">this link</span>
         to display the input, expected output and actual output.
       </div>
diff --git a/tests/e2e/question.pageObject.js b/tests/e2e/question.pageObject.js
index a6402cc3..322ff926 100644
--- a/tests/e2e/question.pageObject.js
+++ b/tests/e2e/question.pageObject.js
@@ -57,6 +57,37 @@ var QuestionPage = function() {
    */
   var resetCodeButton = element(by.css('.protractor-test-reset-code-button'));
 
+  /**
+   * monospace-display-modal element.
+   *
+   * @type {webdriver.WebElement}
+   */
+  this.monospaceDisplayModalElement =
+      element(by.tagName('monospace-display-modal'));
+
+  /**
+   * Monospace modal container element.
+   *
+   * @type {webdriver.WebElement}
+   */
+  this.monospaceModalContainerElement =
+      element(by.css('.tie-monospace-modal-container'));
+
+  /**
+   * Modal link. To be tested with a single runCode() after resetCode().
+   *
+   * @type {webdriver.WebElement}
+   */
+  var modalLink = element(by.css('.tie-modal-link'));
+
+  /**
+   * Dismiss button on modal.
+   *
+   * @type {webdriver.WebElement}
+   */
+  var dismissModalButton =
+      element(by.css('.tie-monospace-modal-action-button'));
+
   /**
    * Run Code button.
    *
@@ -119,6 +150,56 @@ var QuestionPage = function() {
     await resetCodeButton.click();
   };
 
+  /**
+   * Simulates clicking on the modal link (code, output, or error).
+   * To be tested with a single runCode() after resetCode().
+   */
+  this.openModal = async function() {
+    await modalLink.click();
+  };
+
+  /**
+   * Simulates clicking on the dismiss modal button.
+   */
+  this.dismissModal = async function() {
+    await dismissModalButton.click();
+  };
+
+  /**
+   * Gets class name for given element.
+   *
+   * @param {webdriver.WebElement} element Element to retrieve class attribute
+   * for.
+   *
+   * @returns {string} String containing the class names.
+   */
+  this.getClassNames = async function(element) {
+    return await element.getAttribute('class');
+  };
+
+  /**
+   * Returns whether or not aria-hidden attribute is set to true.
+   *
+   * @param {webdriver.WebElement} element Element to check aria-hidden
+   * attribute.
+   *
+   * @returns {boolean}
+   */
+  this.isAriaHidden = async function(element) {
+    return (await element.getAttribute('aria-hidden') === 'true');
+  };
+
+  /**
+   * Returns style attribute.
+   *
+   * @param {webdriver.WebElement} element Element to check top style value.
+   *
+   * @returns {string} String containing styles applied to element.
+   */
+  this.getStyles = async function(element) {
+    return (await element.getAttribute('style'));
+  };
+
   /**
    * Simulates writing the given code string in the code editor.
    *
diff --git a/tests/e2e/question.spec.js b/tests/e2e/question.spec.js
index 918c651b..65eedd30 100644
--- a/tests/e2e/question.spec.js
+++ b/tests/e2e/question.spec.js
@@ -41,6 +41,35 @@ describe('Question Page', function() {
     await questionPage.runCode();
   });
 
+  it('should successfully open and dismiss a modal', async function() {
+    await questionPage.resetCode();
+    await questionPage.runCode();
+
+    await browser.sleep(3000);
+    questionPage.openModal();
+    await browser.sleep(2000);
+
+    expect(await questionPage.getClassNames(element(by.tagName(
+        'monospace-display-modal')))).not.toContain('ng-hide');
+    expect(await questionPage.getClassNames(
+        questionPage.monospaceModalContainerElement)).toContain(
+        'tie-feedback-modal-displayed');
+    expect(await questionPage.isAriaHidden(
+        questionPage.monospaceDisplayModalElement)).toBe(false);
+    expect(await questionPage.getStyles(
+        questionPage.monospaceModalContainerElement)).not.toContain('top: ');
+
+    questionPage.dismissModal();
+    await browser.sleep(2000);
+
+    expect(await questionPage.getClassNames(
+        questionPage.monospaceDisplayModalElement)).toContain('ng-hide');
+    expect(await questionPage.isAriaHidden(
+        questionPage.monospaceDisplayModalElement)).toBe(true);
+    expect(await questionPage.getStyles(
+        questionPage.monospaceModalContainerElement)).toContain('top: ');
+  });
+
   it('should display a feedback text paragraph after a run', async function() {
     await questionPage.resetCode();
     await questionPage.runCode();