PDF Viewer update: Change single/two-page to a checkmark toggle.

Fixed: 1123224
Change-Id: Ifa40c81d5514ad903e6065f95109a43caa0a2dc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2384911Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Auto-Submit: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803236}

chrome/browser/pdf/pdf_extension_util.cc

@@ -39,7 +39,6 @@ void AddCommonStrings(base::Value* dict) {
       {"tooltipFitToWidth", IDS_PDF_TOOLTIP_FIT_WIDTH},
       {"tooltipZoomIn", IDS_PDF_TOOLTIP_ZOOM_IN},
       {"tooltipZoomOut", IDS_PDF_TOOLTIP_ZOOM_OUT},
-      {"twoUpViewDisable", IDS_PDF_TWO_UP_VIEW_DISABLE},
       {"twoUpViewEnable", IDS_PDF_TWO_UP_VIEW_ENABLE},
   };
   for (const auto& resource : kPdfResources)

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

@@ -130,10 +130,6 @@
     margin-inline-end: 12px;
     width: 16px;
   }
-
-  #show-annotations-button {
-    border-top: var(--cr-separator-line);
-  }
 </style>
 <!-- TODO(dpapad): Add aria-label and title for all buttons -->
 <div id="toolbar">
@@ -195,18 +191,9 @@
 </paper-progress>
 
 <cr-action-menu>
-  <button id="single-page-view-button"
-      class="dropdown-item" on-click="onSinglePageViewClick_"
-      role="radio"
-      aria-checked="[[getSinglePageAriaChecked_(twoUpViewEnabled_)]]">
-    <span class="check-container">
-      <iron-icon icon="pdf:check" hidden="[[twoUpViewEnabled_]]"></iron-icon>
-    </span>
-    $i18n{twoUpViewDisable}
-  </button>
   <button id="two-page-view-button"
-      class="dropdown-item" on-click="onTwoPageViewClick_"
-      role="radio"
+      class="dropdown-item" on-click="toggleTwoPageViewClick_"
+      role="checkbox"
       aria-checked="[[getTwoPageViewAriaChecked_(twoUpViewEnabled_)]]">
     <span class="check-container">
       <iron-icon icon="pdf:check" hidden="[[!twoUpViewEnabled_]]"></iron-icon>

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

@@ -216,16 +216,10 @@ export class ViewerPdfToolbarNewElement extends PolymerElement {
   }
 
   /** @private */
-  onSinglePageViewClick_() {
-    this.twoUpViewEnabled_ = false;
-    this.dispatchEvent(new CustomEvent('two-up-view-changed', {detail: false}));
-    this.getMenu_().close();
-  }
-
-  /** @private */
-  onTwoPageViewClick_() {
-    this.twoUpViewEnabled_ = true;
-    this.dispatchEvent(new CustomEvent('two-up-view-changed', {detail: true}));
+  toggleTwoPageViewClick_() {
+    this.twoUpViewEnabled_ = !this.twoUpViewEnabled_;
+    this.dispatchEvent(new CustomEvent(
+        'two-up-view-changed', {detail: this.twoUpViewEnabled_}));
     this.getMenu_().close();
   }
 

chrome/test/data/pdf/viewer_pdf_toolbar_new_test.js

@@ -26,6 +26,16 @@ function getCrIconButtons(toolbar, parentId) {
       toolbar.shadowRoot.querySelectorAll(`#${parentId} cr-icon-button`));
 }
 
+/**
+ * @param {!HTMLElement} button
+ * @param {boolean} enabled
+ */
+function assertCheckboxMenuButton(button, enabled) {
+  chrome.test.assertEq(
+      enabled ? 'true' : 'false', button.getAttribute('aria-checked'));
+  chrome.test.assertEq(enabled, !button.querySelector('iron-icon').hidden);
+}
+
 // Unit tests for the viewer-pdf-toolbar-new element.
 const tests = [
   /**
@@ -169,69 +179,50 @@ const tests = [
     chrome.test.succeed();
   },
 
-  function testSinglePageView() {
-    const toolbar = createToolbar();
-    const singlePageViewButton =
-        toolbar.shadowRoot.querySelector('#single-page-view-button');
-    const twoPageViewButton =
-        toolbar.shadowRoot.querySelector('#two-page-view-button');
-
-    toolbar.addEventListener('two-up-view-changed', function(e) {
-      chrome.test.assertEq(false, e.detail);
-      chrome.test.assertEq(
-          'true', singlePageViewButton.getAttribute('aria-checked'));
-      chrome.test.assertFalse(
-          singlePageViewButton.querySelector('iron-icon').hidden);
-      chrome.test.assertEq(
-          'false', twoPageViewButton.getAttribute('aria-checked'));
-      chrome.test.assertTrue(
-          twoPageViewButton.querySelector('iron-icon').hidden);
-      chrome.test.succeed();
-    });
-    singlePageViewButton.click();
-  },
-
-  function testTwoPageView() {
+  function testTwoPageViewToggle() {
     const toolbar = createToolbar();
-    const singlePageViewButton =
-        toolbar.shadowRoot.querySelector('#single-page-view-button');
-    const twoPageViewButton =
-        toolbar.shadowRoot.querySelector('#two-page-view-button');
-
-    toolbar.addEventListener('two-up-view-changed', function(e) {
-      chrome.test.assertEq(true, e.detail);
-      chrome.test.assertEq(
-          'true', twoPageViewButton.getAttribute('aria-checked'));
-      chrome.test.assertFalse(
-          twoPageViewButton.querySelector('iron-icon').hidden);
-      chrome.test.assertEq(
-          'false', singlePageViewButton.getAttribute('aria-checked'));
-      chrome.test.assertTrue(
-          singlePageViewButton.querySelector('iron-icon').hidden);
-      chrome.test.succeed();
-    });
-    twoPageViewButton.click();
+    const button = /** @type {!HTMLElement} */ (
+        toolbar.shadowRoot.querySelector('#two-page-view-button'));
+    assertCheckboxMenuButton(button, false);
+
+    let whenChanged = eventToPromise('two-up-view-changed', toolbar);
+    button.click();
+    whenChanged
+        .then(e => {
+          chrome.test.assertEq(true, e.detail);
+          assertCheckboxMenuButton(button, true);
+          whenChanged = eventToPromise('two-up-view-changed', toolbar);
+          button.click();
+          return whenChanged;
+        })
+        .then(e => {
+          chrome.test.assertEq(false, e.detail);
+          assertCheckboxMenuButton(button, false);
+          chrome.test.succeed();
+        });
   },
 
   function testShowAnnotationsToggle() {
     const toolbar = createToolbar();
-
-    const showAnnotationsButton =
-        toolbar.shadowRoot.querySelector('#show-annotations-button');
-    chrome.test.assertEq(
-        'true', showAnnotationsButton.getAttribute('aria-checked'));
-    chrome.test.assertFalse(
-        showAnnotationsButton.querySelector('iron-icon').hidden);
-
-    toolbar.addEventListener('display-annotations-changed', (e) => {
-      chrome.test.assertEq(false, e.detail);
-      chrome.test.assertEq(
-          'false', showAnnotationsButton.getAttribute('aria-checked'));
-      chrome.test.assertTrue(
-          showAnnotationsButton.querySelector('iron-icon').hidden);
-      chrome.test.succeed();
-    });
-    showAnnotationsButton.click();
+    const button = /** @type {!HTMLElement} */ (
+        toolbar.shadowRoot.querySelector('#show-annotations-button'));
+    assertCheckboxMenuButton(button, true);
+
+    let whenChanged = eventToPromise('display-annotations-changed', toolbar);
+    button.click();
+    whenChanged
+        .then(e => {
+          chrome.test.assertEq(false, e.detail);
+          assertCheckboxMenuButton(button, false);
+          whenChanged = eventToPromise('display-annotations-changed', toolbar);
+          button.click();
+          return whenChanged;
+        })
+        .then(e => {
+          chrome.test.assertEq(true, e.detail);
+          assertCheckboxMenuButton(button, true);
+          chrome.test.succeed();
+        });
   },
 
   function testSidenavToggleButton() {

components/pdf_strings.grdp

@@ -58,9 +58,6 @@
     <message name="IDS_PDF_TWO_UP_VIEW_ENABLE" desc="Button text for the button which enables displaying two pages of a PDF on the screen horizontally">
       Two page view
     </message>
-    <message name="IDS_PDF_TWO_UP_VIEW_DISABLE" desc="Button text for the button which stops displaying two pages of a PDF on the screen horizontally">
-      Single page view
-    </message>
     <message name="IDS_PDF_ANNOTATIONS_SHOW_TOGGLE" desc="Button text for toggling showing annotations on the PDF document">
       Show annotations
     </message>

components/pdf_strings_grdp/IDS_PDF_TWO_UP_VIEW_DISABLE.png.sha1

-c947bd9cda772b4f1933139637fe856acf4b094b
\ No newline at end of file