PDF Viewer edit mode: Fix race condition

Fix a race condition where if the download button is clicked while a
form field is still focused, the plugin may not have yet notified the
UI that the document has edits. This results in the "Edited"/"Original"
menu not appearing, and the original document being directly downloaded
instead.

Check before saving the original document if a form field is focused,
and if so, wait for the form field focus to change, which gives the
plugin time to send the edit status update.

Bug: 1098082
Change-Id: Ib11aa2ca9c06d06b9d05041272b070a674b316f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2261188Reviewed-by: dpapad <dpapad@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782114}

chrome/browser/resources/pdf/elements/BUILD.gn

@@ -83,6 +83,7 @@ js_library("viewer-pdf-toolbar") {
     "//ui/webui/resources/cr_elements/cr_action_menu:cr_action_menu.m",
     "//ui/webui/resources/js:assert.m",
     "//ui/webui/resources/js:load_time_data.m",
+    "//ui/webui/resources/js:promise_resolver.m",
   ]
   externs_list = [ "$externs_path/pending.js" ]
 }

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

@@ -12,11 +12,13 @@ import './viewer-page-selector.js';
 import './viewer-toolbar-dropdown.js';
 // <if expr="chromeos">
 import './viewer-pen-options.js';
+
 // </if>
 
 import {AnchorAlignment} from 'chrome://resources/cr_elements/cr_action_menu/cr_action_menu.m.js';
 import {assert} from 'chrome://resources/js/assert.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
+import {PromiseResolver} from 'chrome://resources/js/promise_resolver.m.js';
 import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
 import {Bookmark} from '../bookmark_type.js';
@@ -78,6 +80,11 @@ Polymer({
 
     hasEnteredAnnotationMode: Boolean,
 
+    isFormFieldFocused: {
+      type: Boolean,
+      observer: 'onFormFieldFocusedChanged_',
+    },
+
     /** The current loading progress of the PDF document (0 - 100). */
     loadProgress: {
       type: Number,
@@ -126,6 +133,9 @@ Polymer({
    */
   pdfFormSaveEnabled_: false,
 
+  /** @private {?PromiseResolver<boolean>} */
+  waitForFormFocusChange_: null,
+
   /**
    * @param {number} newProgress
    * @param {number} oldProgress
@@ -223,17 +233,52 @@ Polymer({
   },
 
   /** @private */
-  onDownloadClick_() {
-    if (!this.hasEnteredAnnotationMode &&
-        (!this.hasEdits || !this.pdfFormSaveEnabled_)) {
-      this.fire('save', SaveRequestType.ORIGINAL);
-      return;
-    }
+  showDownloadMenu_() {
     this.$.downloadMenu.showAt(this.$.download, {
       anchorAlignmentX: AnchorAlignment.CENTER,
       anchorAlignmentY: AnchorAlignment.AFTER_END,
       noOffset: true,
     });
+    // For tests
+    this.fire('download-menu-shown-for-testing');
+  },
+
+  /** @private */
+  onDownloadClick_() {
+    this.waitForEdits_().then(hasEdits => {
+      if (hasEdits) {
+        this.showDownloadMenu_();
+      } else {
+        this.fire('save', SaveRequestType.ORIGINAL);
+      }
+    });
+  },
+
+  /**
+   * @return {!Promise<boolean>} Promise that resolves with true if the PDF has
+   *     edits and/or annotations, and false otherwise.
+   * @private
+   */
+  waitForEdits_() {
+    if (this.hasEnteredAnnotationMode ||
+        (this.hasEdits && this.pdfFormSaveEnabled_)) {
+      return Promise.resolve(true);
+    }
+    if (!this.isFormFieldFocused || !this.pdfFormSaveEnabled_) {
+      return Promise.resolve(false);
+    }
+    this.waitForFormFocusChange_ = new PromiseResolver();
+    return this.waitForFormFocusChange_.promise;
+  },
+
+  /** @private */
+  onFormFieldFocusedChanged_() {
+    if (!this.waitForFormFocusChange_) {
+      return;
+    }
+
+    this.waitForFormFocusChange_.resolve(this.hasEdits);
+    this.waitForFormFocusChange_ = null;
   },
 
   /** @private */

chrome/browser/resources/pdf/pdf_viewer.html

@@ -23,6 +23,7 @@
     bookmarks="[[bookmarks_]]" doc-title="[[title_]]"
     has-edits="[[hasEdits_]]"
     has-entered-annotation-mode="[[hasEnteredAnnotationMode_]]"
+    is-form-field-focused="[[isFormFieldFocused_]]"
     on-save="onToolbarSave_" on-print="onPrint_"
     on-annotation-mode-toggled="onAnnotationModeToggled_"
     on-annotation-tool-changed="onAnnotationToolChanged_"

chrome/browser/resources/pdf/pdf_viewer.js

@@ -120,6 +120,8 @@ class PDFViewerElement extends PDFViewerBaseElement {
       canSerializeDocument_: Boolean,
 
       title_: String,
+
+      isFormFieldFocused_: Boolean,
     };
   }
 
@@ -154,10 +156,11 @@ class PDFViewerElement extends PDFViewerBaseElement {
     /** @private {string} */
     this.title_ = '';
 
-    // Non-Polymer properties
     /** @private {boolean} */
     this.isFormFieldFocused_ = false;
 
+    // Non-Polymer properties
+
     /** @private {number} */
     this.beepCount_ = 0;
 

chrome/test/data/pdf/annotations_feature_enabled_test.js

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
+import {testAsync} from './test_util.js';
 
 window.onerror = e => chrome.test.fail(e.stack);
 window.onunhandledrejection = e => chrome.test.fail(e.reason);
@@ -29,15 +30,6 @@ function isAnnotationMode() {
   return viewer.shadowRoot.querySelector('#toolbar').annotationMode;
 }
 
-async function testAsync(f) {
-  try {
-    await f();
-    chrome.test.succeed();
-  } catch (e) {
-    chrome.test.fail(e.stack);
-  }
-}
-
 chrome.test.runTests([
   function testAnnotationsEnabled() {
     const toolbar = viewer.shadowRoot.querySelector('#toolbar');

chrome/test/data/pdf/material_elements_test.js

@@ -4,9 +4,10 @@
 
 import {FittingType, SaveRequestType} from 'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/constants.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
+import {listenOnce} from 'chrome://resources/js/util.m.js';
 import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
-import {createBookmarksForTest} from './test_util.js';
+import {createBookmarksForTest, testAsync} from './test_util.js';
 
 /**
  * Captures 'fit-to-changed' events and verifies the last one has the expected
@@ -319,65 +320,101 @@ const tests = [
     chrome.test.assertFalse(actionMenu.open);
     loadTimeData.overrideValues({pdfFormSaveEnabled: false});
     pdfToolbar.strings = Object.assign({}, pdfToolbar.strings);
+    pdfToolbar.isFormFieldFocused = false;
 
-    let lastRequestType;
     let numRequests = 0;
-    pdfToolbar.addEventListener('save', e => {
-      lastRequestType = e.detail;
-      numRequests++;
+    pdfToolbar.addEventListener('save', () => numRequests++);
+
+    /** @return {!Promise<SaveRequestType>} */
+    const whenSave = function() {
+      return new Promise(resolve => {
+        listenOnce(pdfToolbar, 'save', e => resolve(e.detail));
+      });
+    };
+
+    /** @return {!Promise} */
+    const whenDownloadMenuShown = function() {
+      return new Promise(
+          resolve => listenOnce(
+              pdfToolbar, 'download-menu-shown-for-testing', resolve));
+    };
+
+    testAsync(async () => {
+      // No edits, and feature is off.
+      let onSave = whenSave();
+      downloadButton.click();
+      let requestType = await onSave;
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(SaveRequestType.ORIGINAL, requestType);
+      chrome.test.assertEq(1, numRequests);
+
+      // Still does not show the menu if there are no edits.
+      loadTimeData.overrideValues({pdfFormSaveEnabled: true});
+      pdfToolbar.strings = Object.assign({}, pdfToolbar.strings);
+      onSave = whenSave();
+      downloadButton.click();
+      requestType = await onSave;
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(SaveRequestType.ORIGINAL, requestType);
+      chrome.test.assertEq(2, numRequests);
+
+      // Set form field focused.
+      pdfToolbar.isFormFieldFocused = true;
+      onSave = whenSave();
+      downloadButton.click();
+
+      // Unfocus, without making any edits. Saves the original document.
+      pdfToolbar.isFormFieldFocused = false;
+      requestType = await onSave;
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(SaveRequestType.ORIGINAL, requestType);
+      chrome.test.assertEq(3, numRequests);
+
+      // Focus again.
+      pdfToolbar.isFormFieldFocused = true;
+      downloadButton.click();
+
+      // Set editing mode and change the form focus. Now, the menu should
+      // open.
+      pdfToolbar.hasEdits = true;
+      pdfToolbar.isFormFieldFocused = false;
+      await whenDownloadMenuShown();
+      chrome.test.assertTrue(actionMenu.open);
+      chrome.test.assertEq(3, numRequests);
+
+      // Click on "Edited".
+      const buttons = pdfToolbar.shadowRoot.querySelectorAll('button');
+      onSave = whenSave();
+      buttons[0].click();
+      requestType = await onSave;
+      chrome.test.assertEq(SaveRequestType.EDITED, requestType);
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(4, numRequests);
+
+      // Click again to re-open menu.
+      downloadButton.click();
+      await whenDownloadMenuShown();
+      chrome.test.assertTrue(actionMenu.open);
+
+      // Click on "Original".
+      onSave = whenSave();
+      buttons[1].click();
+      requestType = await onSave;
+      chrome.test.assertEq(SaveRequestType.ORIGINAL, requestType);
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(5, numRequests);
+
+      // Even if the document has been edited, always download the original
+      // if the feature flag is off.
+      loadTimeData.overrideValues({pdfFormSaveEnabled: false});
+      pdfToolbar.strings = Object.assign({}, pdfToolbar.strings);
+      onSave = whenSave();
+      downloadButton.click();
+      requestType = await onSave;
+      chrome.test.assertFalse(actionMenu.open);
+      chrome.test.assertEq(SaveRequestType.ORIGINAL, requestType);
+      chrome.test.assertEq(6, numRequests);
     });
-
-    // No edits, and feature is off.
-    downloadButton.click();
-    chrome.test.assertFalse(actionMenu.open);
-    chrome.test.assertEq(SaveRequestType.ORIGINAL, lastRequestType);
-    chrome.test.assertEq(1, numRequests);
-
-    // Reset.
-    lastRequestType = SaveRequestType.EDITED;
-
-    // Still does not show the menu if there are no edits.
-    loadTimeData.overrideValues({pdfFormSaveEnabled: true});
-    pdfToolbar.strings = Object.assign({}, pdfToolbar.strings);
-    downloadButton.click();
-    chrome.test.assertFalse(actionMenu.open);
-    chrome.test.assertEq(SaveRequestType.ORIGINAL, lastRequestType);
-    chrome.test.assertEq(2, numRequests);
-
-    // Set editing mode. Now, clicking download opens the menu.
-    pdfToolbar.hasEdits = true;
-    downloadButton.click();
-    chrome.test.assertTrue(actionMenu.open);
-    chrome.test.assertEq(2, numRequests);
-
-    // Click on "Edited".
-    const buttons = pdfToolbar.shadowRoot.querySelectorAll('button');
-    buttons[0].click();
-    chrome.test.assertEq(SaveRequestType.EDITED, lastRequestType);
-    chrome.test.assertFalse(actionMenu.open);
-    chrome.test.assertEq(3, numRequests);
-
-    // Click again.
-    downloadButton.click();
-    chrome.test.assertTrue(actionMenu.open);
-    chrome.test.assertEq(3, numRequests);
-
-    // Click on "Original".
-    buttons[1].click();
-    chrome.test.assertEq(SaveRequestType.ORIGINAL, lastRequestType);
-    chrome.test.assertFalse(actionMenu.open);
-    chrome.test.assertEq(4, numRequests);
-
-    // Even if the document has been edited, always download the original if the
-    // feature flag is off.
-    loadTimeData.overrideValues({pdfFormSaveEnabled: false});
-    pdfToolbar.strings = Object.assign({}, pdfToolbar.strings);
-    downloadButton.click();
-    chrome.test.assertFalse(actionMenu.open);
-    chrome.test.assertEq(SaveRequestType.ORIGINAL, lastRequestType);
-    chrome.test.assertEq(5, numRequests);
-
-    chrome.test.succeed();
   },
 ];
 

chrome/test/data/pdf/test_util.js

@@ -160,3 +160,12 @@ export function getZoomableViewport(
   viewport.setZoomFactorRange([0.25, 0.4, 0.5, 1, 2]);
   return viewport;
 }
+
+export async function testAsync(f) {
+  try {
+    await f();
+    chrome.test.succeed();
+  } catch (e) {
+    chrome.test.fail(e.stack);
+  }
+}