Print Preview Refresh: Fix error handling

(1) Make errors expand/contract rather than pre-allocating space
(2) Adjust padding so that padding is always 16px regardless of error
state
(3) Fix an issue with pages error not appearing when radio button is
reselected
(4) Make error line-height correct.
(5) Other changes to ensure labels stay correctly aligned with
controls regardless of control height.
(6) No hand pointer on error text.

Bug: 884752, 884729, 884603, 884640, 885039
Change-Id: I9ff4e0bbc536b0890687b678cf52640c01e5a2ef
Reviewed-on: https://chromium-review.googlesource.com/1229618
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592629}

chrome/browser/resources/print_preview/new/copies_settings.html

@@ -19,8 +19,7 @@
       <div slot="opt-inside-content" class="checkbox" aria-live="polite"
           hidden$="[[collateHidden_(currentValue_, inputValid_)]]">
         <cr-checkbox id="collate" on-change="onCollateChange_"
-            disabled$="[[getDisabled(state)]]"
-            aria-labelledby="copies-collate-label">
+            disabled="[[disabled]]" aria-labelledby="copies-collate-label">
           <span id="copies-collate-label">$i18n{optionCollate}</span>
         </cr-checkbox>
       </div>

chrome/browser/resources/print_preview/new/number_settings_section.html

@@ -8,10 +8,14 @@
 <dom-module id="print-preview-number-settings-section">
   <template>
     <style include="print-preview-shared">
-      print-preview-settings-section {
+      :host([error-displayed_]) print-preview-settings-section {
         margin-bottom: 8px;
       }
 
+      :host([error-displayed_]) #section-title {
+        margin-top: calc(-14px - 2 * .75rem);
+      }
+
       :host {
         --cr-input-width: 50%;
       }
@@ -28,20 +32,20 @@
         display: flex;
         width: 100%;
       }
+
+      :host(:not([error-displayed_])) cr-input {
+        --cr-input-error-display: none;
+      }
     </style>
-    <print-preview-settings-section
-        class="input-settings-section multirow-controls">
+    <print-preview-settings-section>
       <span slot="title" id="section-title">[[inputLabel]]</span>
       <div slot="controls" id="controls">
-        <slot name="opt-outside-content"></slot>
         <span class="input-wrapper">
           <cr-input id="userValue" type="number"
               max="[[maxValue]]" min="[[minValue]]" data-timeout-delay="250"
               disabled$="[[getDisabled_(disabled)]]" on-keydown="onKeydown_"
               on-blur="onBlur_" aria-labelled-by="section-title"
-              error-message="[[getHintMessage_(
-                  hintMessage, inputValid, inputString_)]]"
-              auto-validate>
+              error-message="[[hintMessage]]" auto-validate>
             <span slot="suffix">
               <slot name="opt-inside-content"></slot>
             </span>

chrome/browser/resources/print_preview/new/number_settings_section.js

@@ -40,6 +40,16 @@ Polymer({
     hintMessage: String,
 
     disabled: Boolean,
+
+    /**
+     * Whether the error message should be displayed on the input.
+     * @private {boolean}
+     */
+    errorDisplayed_: {
+      type: Boolean,
+      reflectToAttribute: true,
+      computed: 'computeErrorDisplayed_(inputString_, inputValid)',
+    },
   },
 
   listeners: {
@@ -97,7 +107,7 @@ Polymer({
 
   /**
    * @return {boolean} Whether input value represented by inputString_ is
-   *     valid.
+   *     valid and non-empty, so that it can be used to update the setting.
    * @private
    */
   computeValid_: function() {
@@ -107,11 +117,10 @@ Polymer({
   },
 
   /**
-   * @return {string} The error message, or an empty string if there is no
-   *     error.
+   * @return {boolean} Whether the error message should be empty.
    * @private
    */
-  getHintMessage_: function() {
-    return this.inputValid || this.inputString_ == '' ? '' : this.hintMessage;
+  computeErrorDisplayed_: function() {
+    return !this.inputValid && this.inputString_ != '';
   },
 });

chrome/browser/resources/print_preview/new/pages_settings.html

@@ -13,15 +13,25 @@
 <dom-module id="print-preview-pages-settings">
   <template>
     <style include="print-preview-shared">
-      print-preview-settings-section {
-        margin-bottom: 8px;
+      :host([error-state_='0']) #pageSettingsCustomInput {
+        --cr-input-error-display: none;
       }
 
-      #custom-radio-button {
+      :host(:not([error-state_='0'])) #custom-radio-button {
+        /* Margin = -1 * error height = -1 * (16px + 2 lines * line-height) */
         --cr-radio-button-disc: {
-          margin-top: -1.5rem;
+          margin-top: calc(-16px - 2 * .75rem);
         };
       }
+
+      /* Margin = standard margin (16px) - error field margin (8px) */
+      :host(:not([error-state_='0'])) print-preview-settings-section {
+        margin-bottom: 8px;
+      }
+
+      #pageSettingsCustomInput {
+        cursor: default;
+      }
     </style>
     <print-preview-settings-section
         class="input-settings-section multirow-controls">

chrome/browser/resources/print_preview/new/pages_settings.js

@@ -44,6 +44,7 @@ Polymer({
     /** @private {number} */
     errorState_: {
       type: Number,
+      reflectToAttribute: true,
       value: PagesInputErrorState.NO_ERROR,
     },
 
@@ -281,11 +282,11 @@ Polymer({
   onCustomRadioClick_: function() {
     /** @type {!CrInputElement} */ (this.$.pageSettingsCustomInput)
         .inputElement.focus();
-    this.customSelected_ = true;
   },
 
   /** @private */
   onCustomInputFocus_: function() {
+    this.$.pageSettingsCustomInput.validate();
     this.customSelected_ = true;
   },
 

chrome/browser/resources/print_preview/new/print_preview_shared_css.html

@@ -15,6 +15,8 @@
         --md-select-width: calc(100% - 17px);
         --print-preview-settings-border: 1px solid rgb(232, 234, 237);
         --print-preview-dialog-margin: 34px;
+        --cr-form-field-label-height: 1.5rem;
+        --cr-form-field-label-line-height: .75rem;
       }
 
       /* Default state ********************************************************/

chrome/browser/resources/print_preview/new/scaling_settings.html

@@ -5,24 +5,30 @@
 <link rel="import" href="number_settings_section.html">
 <link rel="import" href="print_preview_shared_css.html">
 <link rel="import" href="settings_behavior.html">
+<link rel="import" href="settings_section.html">
 
 <dom-module id="print-preview-scaling-settings">
   <template>
     <style include="print-preview-shared">
+      print-preview-settings-section {
+        margin-bottom: 0;
+      }
     </style>
-    <print-preview-number-settings-section max-value="200" min-value="10"
-        default-value="100" input-label="$i18n{scalingLabel}"
-        disabled="[[disabled]]" current-value="{{currentValue_}}"
-        input-valid="{{inputValid_}}" hint-message="$i18n{scalingInstruction}"
-        class="multirow-controls">
-      <div slot="opt-outside-content" class="checkbox"
-          hidden$="[[!settings.fitToPage.available]]">
+    <print-preview-settings-section hidden$="[[!settings.fitToPage.available]]">
+      <span slot="title">$i18n{scalingLabel}</span>
+      <div slot="controls" class="checkbox">
         <cr-checkbox id="fit-to-page-checkbox" tabindex="1"
             disabled$="[[getDisabled_(disabled, inputValid_)]]"
             on-change="onFitToPageChange_" aria-live="polite">
           $i18n{optionFitToPage}
         </cr-checkbox>
       </div>
+    </print-preview-settings-section>
+    <print-preview-number-settings-section max-value="200" min-value="10"
+        default-value="100"
+        input-label="[[getScalingInputLabel_(settings.fitToPage.available)]]"
+        disabled="[[disabled]]" current-value="{{currentValue_}}"
+        input-valid="{{inputValid_}}" hint-message="$i18n{scalingInstruction}">
     </print-preview-number-settings-section>
   </template>
   <script src="scaling_settings.js"></script>

chrome/browser/resources/print_preview/new/scaling_settings.js

@@ -201,5 +201,15 @@ Polymer({
       this.currentValue_ = this.documentInfo.fitToPageScaling.toString();
       this.ignoreValue_ = false;
     }
-  }
+  },
+
+  /**
+   * @return {string} The label to use on the scaling input.
+   * @private
+   */
+  getScalingInputLabel_: function() {
+    return this.getSetting('fitToPage').available ?
+        '' :
+        loadTimeData.getString('scalingLabel');
+  },
 });

chrome/test/data/webui/print_preview/new_print_preview_ui_browsertest.js

@@ -305,39 +305,6 @@ TEST_F('PrintPreviewNumberSettingsSectionTest', 'BlocksInvalidKeys',
   this.runMochaTest(number_settings_section_test.TestNames.BlocksInvalidKeys);
 });
 
-PrintPreviewPagesSettingsTest = class extends NewPrintPreviewTest {
-  /** @override */
-  get browsePreload() {
-    return 'chrome://print/new/pages_settings.html';
-  }
-
-  /** @override */
-  get extraLibraries() {
-    return super.extraLibraries.concat([
-      '../settings/test_util.js',
-      'print_preview_test_utils.js',
-      'pages_settings_test.js',
-    ]);
-  }
-
-  /** @override */
-  get suiteName() {
-    return pages_settings_test.suiteName;
-  }
-};
-
-TEST_F('PrintPreviewPagesSettingsTest', 'ValidPageRanges', function() {
-  this.runMochaTest(pages_settings_test.TestNames.ValidPageRanges);
-});
-
-TEST_F('PrintPreviewPagesSettingsTest', 'InvalidPageRanges', function() {
-  this.runMochaTest(pages_settings_test.TestNames.InvalidPageRanges);
-});
-
-TEST_F('PrintPreviewPagesSettingsTest', 'NupChangesPages', function() {
-  this.runMochaTest(pages_settings_test.TestNames.NupChangesPages);
-});
-
 PrintPreviewRestoreStateTest = class extends NewPrintPreviewTest {
   /** @override */
   get browsePreload() {

chrome/test/data/webui/print_preview/pages_settings_test.js

@@ -67,17 +67,22 @@ cr.define('pages_settings_test', function() {
       pagesSection.notifyPath('documentInfo.pageCount');
       Polymer.dom.flush();
 
-      // Select custom
-      pagesSection.$$('#custom-radio-button').click();
-
-      // Set input string
       const input = pagesSection.$.pageSettingsCustomInput.inputElement;
-      input.value = inputString;
-      input.dispatchEvent(
-          new CustomEvent('input', {composed: true, bubbles: true}));
+      const readyForInput = pagesSection.$$('#custom-radio-button').checked ?
+          Promise.resolve() :
+          test_util.eventToPromise('focus', input);
 
-      // Validate results
-      return test_util.eventToPromise('input-change', pagesSection);
+      // Select custom
+      pagesSection.$$('#custom-radio-button').click();
+      return readyForInput.then(() => {
+        // Set input string
+        input.value = inputString;
+        input.dispatchEvent(
+            new CustomEvent('input', {composed: true, bubbles: true}));
+
+        // Validate results
+        return test_util.eventToPromise('input-change', pagesSection);
+      });
     }
 
     /** @param {!Array<number>} expectedPages The expected pages value. */

chrome/test/data/webui/print_preview/print_preview_interactive_ui_tests.js

@@ -99,3 +99,36 @@ TEST_F(
       this.runMochaTest(
           destination_dialog_interactive_test.TestNames.FocusSearchBox);
     });
+
+PrintPreviewPagesSettingsTest = class extends PrintPreviewInteractiveUITest {
+  /** @override */
+  get browsePreload() {
+    return 'chrome://print/new/pages_settings.html';
+  }
+
+  /** @override */
+  get extraLibraries() {
+    return super.extraLibraries.concat([
+      '../settings/test_util.js',
+      'print_preview_test_utils.js',
+      'pages_settings_test.js',
+    ]);
+  }
+
+  /** @override */
+  get suiteName() {
+    return pages_settings_test.suiteName;
+  }
+};
+
+TEST_F('PrintPreviewPagesSettingsTest', 'ValidPageRanges', function() {
+  this.runMochaTest(pages_settings_test.TestNames.ValidPageRanges);
+});
+
+TEST_F('PrintPreviewPagesSettingsTest', 'InvalidPageRanges', function() {
+  this.runMochaTest(pages_settings_test.TestNames.InvalidPageRanges);
+});
+
+TEST_F('PrintPreviewPagesSettingsTest', 'NupChangesPages', function() {
+  this.runMochaTest(pages_settings_test.TestNames.NupChangesPages);
+});

chrome/test/data/webui/print_preview/settings_section_test.js

@@ -375,19 +375,20 @@ cr.define('settings_sections_tests', function() {
 
       // HTML to non-PDF destination -> only input shown
       initDocumentInfo(false, false);
-      const fitToPageContainer = scalingElement.$$('.checkbox');
+      const fitToPageSection =
+          scalingElement.$$('print-preview-settings-section');
       const scalingInputWrapper =
           scalingElement.$$('print-preview-number-settings-section')
               .$$('.input-wrapper');
       assertFalse(scalingElement.hidden);
-      assertTrue(fitToPageContainer.hidden);
+      assertTrue(fitToPageSection.hidden);
       assertFalse(scalingInputWrapper.hidden);
 
       // PDF to non-PDF destination -> checkbox and input shown. Check that if
       // more settings is collapsed the section is hidden.
       initDocumentInfo(true, false);
       assertFalse(scalingElement.hidden);
-      assertFalse(fitToPageContainer.hidden);
+      assertFalse(fitToPageSection.hidden);
       assertFalse(scalingInputWrapper.hidden);
 
       // PDF to PDF destination -> section disappears.
@@ -581,6 +582,11 @@ cr.define('settings_sections_tests', function() {
       // Set selection of pages 1 and 2.
       customRadio.click();
 
+      // Manually set |customSelected_| since focus may not work correctly on
+      // MacOS. The PageSettingsTests verify this behavior is correct on all
+      // platforms.
+      pagesElement.set('customSelected_', true);
+
       triggerInputEvent(pagesInput, '1-2');
       return test_util.eventToPromise('input-change', pagesElement)
           .then(function() {

chrome/test/data/webui/print_preview/system_dialog_browsertest.js

@@ -77,16 +77,18 @@ cr.define('system_dialog_browsertest', function() {
       assertFalse(linkContainer.disabled);
       assertFalse(link.hidden);
 
-      const pageSettings = page.$$('print-preview-pages-settings');
-      assertFalse(pageSettings.hidden);
+      const moreSettingsElement = page.$$('print-preview-more-settings');
+      moreSettingsElement.$.label.click();
+      const scalingSettings = page.$$('print-preview-scaling-settings');
+      assertFalse(scalingSettings.hidden);
       nativeLayer.resetResolver('getPreview');
 
-      // Set page settings to a bad value
-      pageSettings.$$('#custom-radio-button').click();
-      const pageSettingsInput =
-          pageSettings.$.pageSettingsCustomInput.inputElement;
-      pageSettingsInput.value = 'abc';
-      pageSettingsInput.dispatchEvent(
+      // Set scaling settings to a bad value
+      const scalingSettingsInput =
+          scalingSettings.$$('print-preview-number-settings-section')
+              .$.userValue.inputElement;
+      scalingSettingsInput.value = '0';
+      scalingSettingsInput.dispatchEvent(
           new CustomEvent('input', {composed: true, bubbles: true}));
 
       // No new preview
@@ -94,7 +96,7 @@ cr.define('system_dialog_browsertest', function() {
         assertTrue(false);
       });
 
-      return test_util.eventToPromise('input-change', pageSettings)
+      return test_util.eventToPromise('input-change', scalingSettings)
           .then(function() {
             // Expect disabled print button
             const header = page.$$('print-preview-header');

testing/buildbot/filters/webui_polymer2_browser_tests.filter

@@ -303,7 +303,6 @@ PrintPreviewLinkContainerTest.*
 PrintPreviewModelTest.*
 PrintPreviewNewDestinationSearchTest.*
 PrintPreviewNumberSettingsSectionTest.*
-PrintPreviewPagesSettingsTest.*
 PrintPreviewPolicyTest.*
 PrintPreviewPreviewGenerationTest.*
 PrintPreviewPrintButtonTest.*

testing/buildbot/filters/webui_polymer2_interactive_ui_tests.filter

@@ -15,4 +15,5 @@ MaterialBookmarksFocusTest.All
 MaterialHistoryFocusTest.All
 PrintPreviewDestinationDialogInteractiveTest.FocusSearchBox
 PrintPreviewPrintHeaderInteractiveTest.FocusPrintOnReady
+PrintPreviewPagesSettingsTest.*
 SettingsUIBrowserTest.All

ui/webui/resources/cr_elements/cr_input/cr_input.html

@@ -72,8 +72,8 @@
         color: var(--cr-input-error-color);
         display: var(--cr-input-error-display, block);
         font-size: var(--cr-form-field-label-font-size);
-        height: var(--cr-form-field-label-font-size);
-        line-height: var(--cr-form-field-label-font-size);
+        height: var(--cr-form-field-label-height);
+        line-height: var(--cr-form-field-label-line-height);
         margin: 8px 0;
         visibility: hidden;
       }

ui/webui/resources/cr_elements/shared_vars_css.html

@@ -142,13 +142,15 @@
     --cr-disabled-opacity: 0.38;
     --cr-form-field-bottom-spacing: 16px;
     --cr-form-field-label-font-size: 0.625rem;
+    --cr-form-field-label-height: 0.625rem;
+    --cr-form-field-label-line-height: 0.625rem;
     --cr-form-field-label: {
       color: var(--google-grey-refresh-700);
       display: block;
       font-size: var(--cr-form-field-label-font-size);
       font-weight: 500;
       letter-spacing: 0.4px;
-      line-height: var(--cr-form-field-label-font-size);
+      line-height: var(--cr-form-field-label-line-height);
       margin-bottom: 8px;
     }
     --google-blue-50: #E8F0FE;