PDFViewerUpdate: Annotation mode fixes

- Hide and disable the sidenav in annotation mode.
- Fix a timing issue, where the annotations bar would briefly appear
  when the annotation mode was toggled before disappearing and then
  re-appearing when the ink controller finished loading.
- Fix a positioning issue in which the document scrolled slightly when
  changing between annotation mode and the plugin.
- With the third fix above, positioning is much closer to expectations
  in the annotations feature enabled test. Verified that scrolling/zoom
  seems correct now, and updated the mismatched expectation for the
  case of the update being enabled as recommended by the test's original
  author.

Bug: 1124183
Change-Id: I8f9357d883088e31fc8882f3359e662685978de0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2405539
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806882}

chrome/browser/resources/pdf/BUILD.gn

@@ -163,6 +163,7 @@ js_library("pdf_viewer") {
     ":toolbar_manager",
     "elements:viewer-error-screen",
     "elements:viewer-password-screen",
+    "elements:viewer-pdf-sidenav",
     "elements:viewer-pdf-toolbar",
     "elements:viewer-pdf-toolbar-new",
     "elements:viewer-zoom-toolbar",

chrome/browser/resources/pdf/elements/viewer-pdf-toolbar-new.html

 <style include="pdf-shared">
   :host {
+    --viewer-pdf-toolbar-height: 56px;
     box-shadow:
         0 -2px 8px rgba(0, 0, 0, 0.09),
         0 4px 8px rgba(0, 0, 0, 0.06),
         0 1px 2px rgba(0, 0, 0, 0.3),
         0 2px 6px rgba(0, 0, 0, 0.15);
+    height: var(--viewer-pdf-toolbar-height);
     position: relative;
   }
 
@@ -13,7 +15,7 @@
     background-color: var(--viewer-pdf-toolbar-background-color);
     color: white;
     display: flex;
-    height: 56px;
+    height: var(--viewer-pdf-toolbar-height);
     padding: 0 16px;
   }
 
@@ -165,7 +167,7 @@
 <div id="toolbar">
   <div id="start">
     <cr-icon-button id="sidenavToggle" iron-icon="cr20:menu"
-        on-click="onSidenavToggleClick_">
+        disabled="[[annotationMode]]" on-click="onSidenavToggleClick_">
     </cr-icon-button>
     <span id="title">[[docTitle]]</span>
   </div>

chrome/browser/resources/pdf/elements/viewer-pdf-toolbar-new.js

@@ -40,7 +40,6 @@ export class ViewerPdfToolbarNewElement extends PolymerElement {
       // </if>
       annotationMode: {
         type: Boolean,
-        notify: true,
         value: false,
         reflectToAttribute: true,
       },
@@ -324,11 +323,11 @@ export class ViewerPdfToolbarNewElement extends PolymerElement {
 
   // <if expr="chromeos">
   toggleAnnotation() {
-    this.annotationMode = !this.annotationMode;
+    const newAnnotationMode = !this.annotationMode;
     this.dispatchEvent(new CustomEvent(
-        'annotation-mode-toggled', {detail: this.annotationMode}));
+        'annotation-mode-toggled', {detail: newAnnotationMode}));
 
-    if (this.annotationMode && !this.displayAnnotations_) {
+    if (newAnnotationMode && !this.displayAnnotations_) {
       this.toggleDisplayAnnotations_();
     }
   }

chrome/browser/resources/pdf/pdf_viewer.html

@@ -142,6 +142,7 @@
 </template>
 <template is="dom-if" if="[[pdfViewerUpdateEnabled_]]">
   <viewer-pdf-toolbar-new id="toolbar"
+    annotation-mode="[[annotationMode_]]"
     doc-title="[[title_]]" doc-length="[[docLength_]]" page-no="[[pageNo_]]"
     load-progress="[[loadProgress_]]" has-edits="[[hasEdits_]]"
     has-entered-annotation-mode="[[hasEnteredAnnotationMode_]]"

chrome/browser/resources/pdf/pdf_viewer.js

@@ -4,7 +4,6 @@
 
 import './elements/viewer-error-screen.js';
 import './elements/viewer-password-screen.js';
-import './elements/viewer-pdf-sidenav.js';
 import './elements/viewer-pdf-toolbar.js';
 import './elements/viewer-zoom-toolbar.js';
 import './elements/shared-vars.js';
@@ -18,12 +17,13 @@ import 'chrome://resources/cr_elements/shared_vars_css.m.js';
 
 import {assert, assertNotReached} from 'chrome://resources/js/assert.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
-import {hasKeyModifiers} from 'chrome://resources/js/util.m.js';
+import {hasKeyModifiers, listenOnce} from 'chrome://resources/js/util.m.js';
 import {html} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
 import {Bookmark} from './bookmark_type.js';
 import {BrowserApi} from './browser_api.js';
 import {Attachment, FittingType, Point, SaveRequestType} from './constants.js';
+import {ViewerPdfSidenavElement} from './elements/viewer-pdf-sidenav.js';
 import {ViewerPdfToolbarNewElement} from './elements/viewer-pdf-toolbar-new.js';
 // <if expr="chromeos">
 import {InkController} from './ink_controller.js';
@@ -259,6 +259,12 @@ export class PDFViewerElement extends PDFViewerBaseElement {
     /** @private {boolean} */
     this.sidenavCollapsed_ = false;
 
+    /**
+     * The state to restore sidenavCollapsed_ to after exiting annotation mode.
+     * @private {boolean}
+     */
+    this.sidenavRestoreState_ = false;
+
     if (this.pdfViewerUpdateEnabled_) {
       // TODO(dpapad): Add tests after crbug.com/1111459 is fixed.
       this.sidenavCollapsed_ = Boolean(Number.parseInt(
@@ -458,6 +464,30 @@ export class PDFViewerElement extends PDFViewerBaseElement {
   }
 
   // <if expr="chromeos">
+  /**
+   * @return {!Promise} Resolves when the sidenav animation is complete.
+   * @private
+   */
+  waitForSidenavTransition_() {
+    return new Promise(resolve => {
+      listenOnce(
+          /** @type {!ViewerPdfSidenavElement} */ (
+              this.shadowRoot.querySelector('#sidenav-container')),
+          'transitionend', e => resolve());
+    });
+  }
+
+  /**
+   * @return {!Promise} Resolves when the sidenav is restored to
+   *     |sidenavRestoreState_|, after having been closed for annotation mode.
+   * @private
+   */
+  restoreSidenav_() {
+    this.sidenavCollapsed_ = this.sidenavRestoreState_;
+    return this.sidenavCollapsed_ ? Promise.resolve() :
+                                    this.waitForSidenavTransition_();
+  }
+
   /**
    * Handles the annotation mode being toggled on or off.
    * @param {!CustomEvent<boolean>} e
@@ -465,12 +495,21 @@ export class PDFViewerElement extends PDFViewerBaseElement {
    */
   async onAnnotationModeToggled_(e) {
     const annotationMode = e.detail;
-    this.annotationMode_ = annotationMode;
     if (annotationMode) {
       // Enter annotation mode.
       assert(this.currentController === this.pluginController);
       // TODO(dstockwell): set plugin read-only, begin transition
       this.updateProgress(0);
+
+      if (this.pdfViewerUpdateEnabled_) {
+        this.sidenavRestoreState_ = this.sidenavCollapsed_;
+        this.sidenavCollapsed_ = true;
+        if (!this.sidenavRestoreState_) {
+          // Wait for the animation before proceeding.
+          await this.waitForSidenavTransition_();
+        }
+      }
+
       // TODO(dstockwell): handle save failure
       const saveResult =
           await this.pluginController.save(SaveRequestType.ANNOTATION);
@@ -483,12 +522,12 @@ export class PDFViewerElement extends PDFViewerBaseElement {
         } catch (e) {
           // The user aborted entering annotation mode. Revert to the plugin.
           this.getToolbar_().annotationMode = false;
-          this.annotationMode_ = false;
           this.updateProgress(100);
           return;
         }
       }
       PDFMetrics.record(PDFMetrics.UserAction.ENTER_ANNOTATION_MODE);
+      this.annotationMode_ = true;
       this.hasEnteredAnnotationMode_ = true;
       // TODO(dstockwell): feed real progress data from the Ink component
       this.updateProgress(50);
@@ -502,6 +541,7 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       assert(this.currentController === this.inkController_);
       // TODO(dstockwell): set ink read-only, begin transition
       this.updateProgress(0);
+      this.annotationMode_ = false;
       // This runs separately to allow other consumers of `loaded` to queue
       // up after this task.
       this.loaded.then(() => {
@@ -513,6 +553,9 @@ export class PDFViewerElement extends PDFViewerBaseElement {
           await this.inkController_.save(SaveRequestType.ANNOTATION);
       // Data always exists when save is called with requestType = ANNOTATION.
       const result = /** @type {!RequiredSaveResult} */ (saveResult);
+      if (this.pdfViewerUpdateEnabled_) {
+        await this.restoreSidenav_();
+      }
       await this.pluginController.load(result.fileName, result.dataToSave);
       // Ensure the plugin gets the initial viewport.
       this.pluginController.afterZoom();
@@ -529,6 +572,9 @@ export class PDFViewerElement extends PDFViewerBaseElement {
     }
     this.getToolbar_().toggleAnnotation();
     this.annotationMode_ = false;
+    if (this.pdfViewerUpdateEnabled_) {
+      await this.restoreSidenav_();
+    }
     await this.loaded;
   }
   // </if>
@@ -546,7 +592,8 @@ export class PDFViewerElement extends PDFViewerBaseElement {
    * @private
    */
   onScroll_(e) {
-    if (this.currentController === this.pluginController) {
+    if (this.currentController === this.pluginController &&
+        !this.annotationMode_) {
       this.pluginController.updateScroll(
           e.target.scrollLeft, e.target.scrollTop);
     }

chrome/test/data/pdf/annotations_feature_enabled_test.js

@@ -68,12 +68,6 @@ chrome.test.runTests([
 
       chrome.test.assertEq(3, cameras.length);
 
-      if (updateEnabled) {
-        // TODO (https://crbug.com/1120279): Determine what the expectations
-        // below should be for the new UI and fix if needed to meet them.
-        return;
-      }
-
       const expectations = [
         {top: 44.25, left: -106.5, right: 718.5, bottom: -448.5},
         {top: 23.25, left: -3.75, right: 408.75, bottom: -223.125},
@@ -82,7 +76,10 @@ chrome.test.runTests([
 
       for (const expectation of expectations) {
         const actual = cameras.shift();
-        chrome.test.assertEq(expectation.top, actual.top);
+        const expectationTop = updateEnabled ?
+            Math.min(2.25, expectation.top - 21) :
+            expectation.top;
+        chrome.test.assertEq(expectationTop, actual.top);
         chrome.test.assertEq(expectation.left, actual.left);
         chrome.test.assertEq(expectation.bottom, actual.bottom);
         chrome.test.assertEq(expectation.right, actual.right);
@@ -392,7 +389,7 @@ chrome.test.runTests([
       const toolbar = document.createElement('viewer-pdf-toolbar-new');
       document.body.appendChild(toolbar);
       toolbar.toggleAnnotation();
-      chrome.test.assertTrue(toolbar.annotationMode);
+      toolbar.annotationMode = true;
 
       await toolbar.addEventListener('display-annotations-changed', async e => {
         chrome.test.assertFalse(e.detail);