Print Preview Custom Margins: Fix undo

Allow undo (via ctrl + Z or context menu) to work for the Print Preview
custom margins input boxes by:
(1) Moving the units outside of the text input area (aligned with the
end of the text) rather than appending them to the text directly
(2) Checking if the parsed value in pts matches before overriding
the input value.

Note that (1) requires making these inputs normal <input>s rather than
<cr-input>s, and using the cr input styling via cr-input-style.

Also fixing validation regexes, to handle the following cases:
(1) Entries using the thousands delimeter (e.g. '1,000') - currently
everything after the thousands delimeter is ignored.
(2) Entries with numbers only after the decimal (e.g. '.5') - currently
shows an invalid syntax error (users have to enter e.g. '0.5').

Bug: 452844
Change-Id: I933e56358b75785cf8b42f10585df4522cefe734
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1672526
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671959}

chrome/browser/resources/print_preview/ui/BUILD.gn

@@ -348,6 +348,7 @@ js_library("margin_control") {
     ":input_behavior",
     "../data:coordinate2d",
     "../data:margins",
+    "../data:measurement_system",
     "../data:size",
     "//ui/webui/resources/js:i18n_behavior",
   ]

chrome/browser/resources/print_preview/ui/input_behavior.js

@@ -29,8 +29,9 @@ cr.define('print_preview', function() {
     },
 
     /**
-     * @return {!CrInputElement} The cr-input element the behavior should use.
-     *     Should be overridden by elements using this behavior.
+     * @return {(!CrInputElement|!HTMLInputElement)} The cr-input or input
+     *     element the behavior should use. Should be overridden by elements
+     *     using this behavior.
      */
     getInput: function() {},
 

chrome/browser/resources/print_preview/ui/margin_control.html

 <link rel="import" href="chrome://resources/html/polymer.html">
 
 <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
-<link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html">
+<link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input_style_css.html">
 <link rel="import" href="chrome://resources/html/i18n_behavior.html">
 <link rel="import" href="../print_preview_utils.html">
 <link rel="import" href="../data/coordinate2d.html">
 <link rel="import" href="../data/margins.html">
+<link rel="import" href="../data/measurement_system.html">
 <link rel="import" href="../data/size.html">
 <link rel="import" href="input_behavior.html">
 <link rel="import" href="strings.html">
 
 <dom-module id="print-preview-margin-control">
   <template>
-    <style>
+    <style include="cr-input-style">
       :host {
         display: block;
         position: absolute;
@@ -62,13 +63,13 @@
         height: 100%;
       }
 
-      #textbox {
-        --cr-input-error-display: none;
-        box-sizing: border-box;
+      #row-container {
+        border-radius: 4px;
         font-size: 0.8rem;
+        min-height: 25px;
+        overflow: hidden;
         padding: 1px;
         position: absolute;
-        text-align: center;
         width: 60px;
       }
 
@@ -77,46 +78,86 @@
       }
 
       @media (prefers-color-scheme: light) {
-        #textbox {
+        #row-container {
           --cr-input-background-color: var(--cr-primary-text-color);
           --cr-input-color: white;
+          background-color: var(--cr-primary-text-color);
           color: white;
         }
       }
 
       @media (prefers-color-scheme: dark) {
-        #textbox {
-          --cr-input-background-color: #1b1c1e;  /* GG900 + 30% black */
+        #row-container {
+          --cr-input-background-color: rgb(27, 28, 30);  /* GG900 + 30% black */
           --cr-input-color: var(--cr-primary-text-color);
+          background-color: rgb(27, 28, 30);  /* GG900 + 30% black */
+          color: var(--cr-primary-text-color);
         }
       }
 
-      :host([side=top]) #textbox {
+      :host([side=top]) #row-container {
         left: 50%;
-        top: 8px;
+        top: 9px;
       }
 
-      :host([side=right]) #textbox {
-        right: 8px;
+      :host([side=right]) #row-container {
+        right: 9px;
         top: 50%;
       }
 
-      :host([side=bottom]) #textbox {
-        bottom: 8px;
+      :host([side=bottom]) #row-container {
+        bottom: 9px;
         right: 50%;
       }
 
-      :host([side=left]) #textbox {
+      :host([side=left]) #row-container {
         bottom: 50%;
-        left: 8px;
+        left: 9px;
+      }
+
+      :host([disabled]) #row-container {
+        opacity: var(--cr-disabled-opacity);
+      }
+
+      :host([invalid]) #input {
+        caret-color: var(--cr-input-error-color);
+      }
+
+      :host([invalid]) #underline {
+        border-color: var(--cr-input-error-color);
+      }
+
+      #row-container,
+      #input-container {
+        align-items: center;
+        display: flex;
+      }
+
+      #input-container {
+        position: relative;
+      }
+
+      #input {
+        padding-inline-end: 0;
+        text-align: end;
+      }
+
+      #unit {
+        padding-inline-end: 8px;
       }
     </style>
-    <div id="line"></div>
-    <cr-input id="textbox" aria-hidden$="[[getAriaHidden_(invisible)]]"
-        aria-label$="[[i18n(side)]]" type="text" disabled="[[disabled]]"
-        invalid="[[invalid]]" on-blur="onBlur_" on-focus="onFocus_"
-        data-timeout-delay="1000">
-    </cr-input>
+      <div id="line"></div>
+      <div id="row-container">
+        <div id="input-container">
+          <input id="input" disabled="[[disabled]]" aria-label$="[[i18n(side)]]"
+              aria-hidden$="[[getAriaHidden_(invisible)]]"
+              on-focus="onFocus_" on-blur="onBlur_" on-change="onInputChange_"
+              data-timeout-delay="1000">
+          <span id="unit">[[measurementSystem.unitSymbol]]</span>
+        </div>
+        <div id="underline"></div>
+      </div>
+    </div>
   </template>
   <script src="margin_control.js"></script>
 </dom-module>

chrome/browser/resources/print_preview/ui/margin_control.js

@@ -20,6 +20,7 @@ Polymer({
     disabled: {
       type: Boolean,
       reflectToAttribute: true,
+      observer: 'onDisabledChange_',
     },
 
     side: {
@@ -27,7 +28,10 @@ Polymer({
       reflectToAttribute: true,
     },
 
-    invalid: Boolean,
+    invalid: {
+      type: Boolean,
+      reflectToAttribute: true,
+    },
 
     invisible: {
       type: Boolean,
@@ -35,6 +39,16 @@ Polymer({
       observer: 'onClipSizeChange_',
     },
 
+    /** @type {?print_preview.MeasurementSystem} */
+    measurementSystem: Object,
+
+    /** @private {boolean} */
+    focused_: {
+      type: Boolean,
+      reflectToAttribute: true,
+      value: false,
+    },
+
     /** @private {number} */
     positionInPts_: {
       type: Number,
@@ -76,17 +90,26 @@ Polymer({
     'input-change': 'onInputChange_',
   },
 
-  /** @return {!CrInputElement} The cr-input element for InputBehavior. */
+  /** @return {!HTMLInputElement} The input element for InputBehavior. */
   getInput: function() {
-    return this.$.textbox;
+    return this.$.input;
   },
 
-  /** @param {string} value New value of the margin control's textbox. */
-  setTextboxValue: function(value) {
-    const textbox = this.$.textbox;
-    if (textbox.value != value) {
-      textbox.value = value;
+  /**
+   * @param {number} valueInPts New value of the margin control's textbox in
+   *     pts.
+   */
+  setTextboxValue: function(valueInPts) {
+    const textbox = this.$.input;
+    const pts = textbox.value ? this.parseValueToPts_(textbox.value) : null;
+    if (!!pts && valueInPts === Math.round(pts)) {
+      // If the textbox's value represents the same value in pts as the new one,
+      // don't reset. This allows the "undo" command to work as expected, see
+      // https://crbug.com/452844.
+      return;
     }
+
+    textbox.value = this.serializeValueFromPts_(valueInPts);
   },
 
   /** @return {number} The current position of the margin control. */
@@ -115,20 +138,20 @@ Polymer({
    */
   convertPixelsToPts: function(pixels) {
     let pts;
-    const orientationEnum = print_preview.ticket_items.CustomMarginsOrientation;
-    if (this.side == orientationEnum.TOP) {
+    const Orientation = print_preview.ticket_items.CustomMarginsOrientation;
+    if (this.side == Orientation.TOP) {
       pts = pixels - this.translateTransform.y + RADIUS_PX;
       pts /= this.scaleTransform;
-    } else if (this.side == orientationEnum.RIGHT) {
+    } else if (this.side == Orientation.RIGHT) {
       pts = pixels - this.translateTransform.x + RADIUS_PX;
       pts /= this.scaleTransform;
       pts = this.pageSize.width - pts;
-    } else if (this.side == orientationEnum.BOTTOM) {
+    } else if (this.side == Orientation.BOTTOM) {
       pts = pixels - this.translateTransform.y + RADIUS_PX;
       pts /= this.scaleTransform;
       pts = this.pageSize.height - pts;
     } else {
-      assert(this.side == orientationEnum.LEFT);
+      assert(this.side == Orientation.LEFT);
       pts = pixels - this.translateTransform.x + RADIUS_PX;
       pts /= this.scaleTransform;
     }
@@ -140,26 +163,85 @@ Polymer({
    * @return {boolean} Whether the margin should start being dragged.
    */
   shouldDrag: function(event) {
-    return !this.$.textbox.disabled && event.button == 0 &&
+    return !this.disabled && event.button == 0 &&
         (event.path[0] == this || event.path[0] == this.$.line);
   },
 
+  /** @private */
+  onDisabledChange_: function() {
+    if (this.disabled) {
+      this.focused_ = false;
+    }
+  },
+
+  /**
+   * @param {string} value Value to parse to points. E.g. '3.40' or '200'.
+   * @return {?number} Value in points represented by the input value.
+   * @private
+   */
+  parseValueToPts_: function(value) {
+    // Removing whitespace anywhere in the string.
+    value = value.replace(/\s*/g, '');
+    if (value.length == 0) {
+      return null;
+    }
+    assert(this.measurementSystem);
+    const decimal = this.measurementSystem.decimalDelimeter;
+    const thousands = this.measurementSystem.thousandsDelimeter;
+    const validationRegex = new RegExp(
+        `^(^-?)(((\\d)(\\${thousands}\\d{3})*)(\\${decimal}\\d*)?|` +
+        `((\\d)?(\\${thousands}\\d{3})*)(\\${decimal}\\d*))`);
+    if (validationRegex.test(value)) {
+      // Replacing decimal point with the dot symbol in order to use
+      // parseFloat() properly.
+      value = value.replace(new RegExp(`\\${decimal}{1}`), '.');
+      value = value.replace(new RegExp(`\\${thousands}`, 'g'), '');
+      return this.measurementSystem.convertToPoints(parseFloat(value));
+    }
+    return null;
+  },
+
+  /**
+   * @param {number} value Value in points to serialize.
+   * @return {string} String representation of the value in the system's local
+   *     units.
+   * @private
+   */
+  serializeValueFromPts_: function(value) {
+    assert(this.measurementSystem);
+    value = this.measurementSystem.convertFromPoints(value);
+    value = this.measurementSystem.roundValue(value);
+    return value.toString();
+  },
+
   /**
    * @param {!CustomEvent<string>} e Contains the new value of the input.
    * @private
    */
   onInputChange_: function(e) {
-    this.fire('text-change', e.detail);
+    if (!e.detail) {
+      return;
+    }
+
+    const value = this.parseValueToPts_(e.detail);
+    if (value === null) {
+      this.invalid = true;
+      return;
+    }
+
+    this.fire('text-change', value);
   },
 
   /** @private */
   onBlur_: function() {
+    this.focused_ = false;
     this.resetAndUpdate();
     this.fire('text-blur', this.invalid);
   },
 
   /** @private */
   onFocus_: function() {
+    this.focused_ = true;
     this.fire('text-focus');
   },
 
@@ -169,20 +251,20 @@ Polymer({
       return;
     }
 
-    const orientationEnum = print_preview.ticket_items.CustomMarginsOrientation;
+    const Orientation = print_preview.ticket_items.CustomMarginsOrientation;
     let x = this.translateTransform.x;
     let y = this.translateTransform.y;
     let width = null;
     let height = null;
-    if (this.side == orientationEnum.TOP) {
+    if (this.side == Orientation.TOP) {
       y = this.scaleTransform * this.positionInPts_ +
           this.translateTransform.y - RADIUS_PX;
       width = this.scaleTransform * this.pageSize.width;
-    } else if (this.side == orientationEnum.RIGHT) {
+    } else if (this.side == Orientation.RIGHT) {
       x = this.scaleTransform * (this.pageSize.width - this.positionInPts_) +
           this.translateTransform.x - RADIUS_PX;
       height = this.scaleTransform * this.pageSize.height;
-    } else if (this.side == orientationEnum.BOTTOM) {
+    } else if (this.side == Orientation.BOTTOM) {
       y = this.scaleTransform * (this.pageSize.height - this.positionInPts_) +
           this.translateTransform.y - RADIUS_PX;
       width = this.scaleTransform * this.pageSize.width;

chrome/browser/resources/print_preview/ui/margin_control_container.html

@@ -28,8 +28,11 @@
       <print-preview-margin-control side="[[item]]" invisible="[[invisible_]]"
           disabled="[[controlsDisabled_(state, invisible_)]]"
           translate-transform="[[translateTransform_]]"
-          clip-size="[[clipSize_]]" scale-transform="[[scaleTransform_]]"
-          page-size="[[pageSize]]" on-pointerdown="onPointerDown_"
+          clip-size="[[clipSize_]]"
+          measurement-system="[[measurementSystem]]"
+          scale-transform="[[scaleTransform_]]"
+          page-size="[[pageSize]]"
+          on-pointerdown="onPointerDown_"
           on-text-change="onTextChange_" on-text-blur="onTextBlur_"
           on-text-focus="onTextFocus_" on-transition-end="onTransitionEnd_">
       </print-preview-margin-control>

chrome/browser/resources/print_preview/ui/margin_control_container.js

@@ -172,7 +172,7 @@ Polymer({
           const key = print_preview.MARGIN_KEY_MAP.get(control.side);
           const newValue = margins[key] || 0;
           control.setPositionInPts(newValue);
-          control.setTextboxValue(this.serializeValueFromPts_(newValue));
+          control.setTextboxValue(newValue);
         });
   },
 
@@ -259,20 +259,7 @@ Polymer({
    */
   moveControlWithConstraints_: function(control, posInPts) {
     control.setPositionInPts(posInPts);
-    control.setTextboxValue(this.serializeValueFromPts_(posInPts));
-  },
-
-  /**
-   * @param {number} value Value in points to serialize.
-   * @return {string} String representation of the value in the system's local
-   *     units.
-   * @private
-   */
-  serializeValueFromPts_: function(value) {
-    assert(this.measurementSystem);
-    value = this.measurementSystem.convertFromPoints(value);
-    value = this.measurementSystem.roundValue(value);
-    return value + this.measurementSystem.unitSymbol;
+    control.setTextboxValue(posInPts);
   },
 
   /**
@@ -448,19 +435,14 @@ Polymer({
   },
 
   /**
-   * @param {!CustomEvent<string>} e Event containing the new textbox value.
+   * @param {!CustomEvent<number>} e Event containing the new textbox value.
    * @private
    */
   onTextChange_: function(e) {
-    const marginValue = this.parseValueToPts_(e.detail);
     const control =
         /** @type {!PrintPreviewMarginControlElement} */ (e.target);
-    if (marginValue == null) {
-      control.invalid = true;
-      return;
-    }
     control.invalid = false;
-    const clippedValue = this.clipAndRoundValue_(control.side, marginValue);
+    const clippedValue = this.clipAndRoundValue_(control.side, e.detail);
     control.setPositionInPts(clippedValue);
     this.setMargin_(control.side, clippedValue);
   },
@@ -475,8 +457,7 @@ Polymer({
     if (e.detail /* detail is true if the control is in an invalid state */) {
       const control =
           /** @type {!PrintPreviewMarginControlElement} */ (e.target);
-      control.setTextboxValue(
-          this.serializeValueFromPts_(control.getPositionInPts()));
+      control.setTextboxValue(control.getPositionInPts());
       control.invalid = false;
     }
     this.textboxFocused_ = false;
@@ -521,33 +502,6 @@ Polymer({
     }
   },
 
-  /**
-   * @param {string} value Value to parse to points. E.g. '3.40"' or '200mm'.
-   * @return {?number} Value in points represented by the input value.
-   * @private
-   */
-  parseValueToPts_: function(value) {
-    // Removing whitespace anywhere in the string.
-    value = value.replace(/\s*/g, '');
-    if (value.length == 0) {
-      return null;
-    }
-    assert(this.measurementSystem);
-    const validationRegex = new RegExp(
-        '^(^-?)(\\d)+(\\' + this.measurementSystem.thousandsDelimeter +
-        '\\d{3})*(\\' + this.measurementSystem.decimalDelimeter + '\\d*)?' +
-        '(' + this.measurementSystem.unitSymbol + ')?$');
-    if (validationRegex.test(value)) {
-      // Replacing decimal point with the dot symbol in order to use
-      // parseFloat() properly.
-      const replacementRegex =
-          new RegExp('\\' + this.measurementSystem.decimalDelimeter + '{1}');
-      value = value.replace(replacementRegex, '.');
-      return this.measurementSystem.convertToPoints(parseFloat(value));
-    }
-    return null;
-  },
-
   /**
    * Updates the translation transformation that translates pixel values in
    * the space of the HTML DOM.

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

@@ -163,24 +163,34 @@ cr.define('custom_margins_test', function() {
      * @param {number} currentValue The current margin value in points.
      * @param {string} input The new textbox input for the margin.
      * @param {boolean} invalid Whether the new value is invalid.
+     * @param {number=} newValuePts the new margin value in pts. If not
+     *     specified, computes the value assuming it is in bounds.
      * @return {!Promise} Promise that resolves when the test is complete.
      */
-    function testControlTextbox(control, key, currentValuePts, input, invalid) {
-      const newValuePts = invalid ?
-          currentValuePts :
-          Math.round(parseFloat(input) * pointsPerInch);
+    function testControlTextbox(
+        control, key, currentValuePts, input, invalid, newValuePts) {
+      if (newValuePts === undefined) {
+        newValuePts = invalid ? currentValuePts :
+                                Math.round(parseFloat(input) * pointsPerInch);
+      }
       assertEquals(
           currentValuePts, container.getSettingValue('customMargins')[key]);
-      controlTextbox = control.$.textbox.inputElement;
+      controlTextbox = control.$.input;
       controlTextbox.value = input;
       controlTextbox.dispatchEvent(
           new CustomEvent('input', {composed: true, bubbles: true}));
 
-      return test_util.eventToPromise('text-change', control).then(() => {
-        assertEquals(
-            newValuePts, container.getSettingValue('customMargins')[key]);
-        assertEquals(invalid, control.invalid);
-      });
+      if (!invalid) {
+        return test_util.eventToPromise('text-change', control).then(() => {
+          assertEquals(
+              newValuePts, container.getSettingValue('customMargins')[key]);
+          assertFalse(control.invalid);
+        });
+      } else {
+        return test_util.eventToPromise('input-change', control).then(() => {
+          assertTrue(control.invalid);
+        });
+      }
     }
 
     /*
@@ -392,42 +402,57 @@ cr.define('custom_margins_test', function() {
     // Test that setting the margin controls with their textbox inputs updates
     // the custom margins setting.
     test(assert(TestNames.SetControlsWithTextbox), function() {
+      /**
+       * @param {!Array<!MarginControlElement>} controls
+       * @param {number} currentValue Current margin value in pts
+       * @param {string} input String to set in margin textboxes
+       * @param {boolean} invalid Whether the string is invalid
+       * @return {!Promise} Promise that resolves when all controls have been
+       *     tested.
+       */
+      const testAllTextboxes = function(
+          controls, currentValue, input, invalid) {
+        return testControlTextbox(
+                   controls[0], keys[0], currentValue, input, invalid)
+            .then(
+                () => testControlTextbox(
+                    controls[1], keys[1], currentValue, input, invalid))
+            .then(
+                () => testControlTextbox(
+                    controls[2], keys[2], currentValue, input, invalid))
+            .then(
+                () => testControlTextbox(
+                    controls[3], keys[3], currentValue, input, invalid));
+      };
+
       return finishSetup().then(() => {
         const controls = getControls();
+        // Set a shorter delay for testing so the test doesn't take too
+        // long.
+        controls.forEach(c => {
+          c.getInput().setAttribute('data-timeout-delay', 10);
+        });
         model.set(
             'settings.margins.value',
             print_preview.ticket_items.MarginsTypeValue.CUSTOM);
         Polymer.dom.flush();
 
         // Verify entering a new value updates the settings.
-        // Then verify entering an invalid value invalidates the control and
-        // does not update the settings.
-        const newValue = '1.75';  // 1.75 inches
-        const invalidValue = 'abc';
-        const newMarginPts = Math.round(parseFloat(newValue) * pointsPerInch);
-        return testControlTextbox(
-                   controls[0], keys[0], defaultMarginPts, newValue, false)
-            .then(
-                () => testControlTextbox(
-                    controls[1], keys[1], defaultMarginPts, newValue, false))
-            .then(
-                () => testControlTextbox(
-                    controls[2], keys[2], defaultMarginPts, newValue, false))
-            .then(
-                () => testControlTextbox(
-                    controls[3], keys[3], defaultMarginPts, newValue, false))
-            .then(
-                () => testControlTextbox(
-                    controls[0], keys[0], newMarginPts, invalidValue, true))
-            .then(
-                () => testControlTextbox(
-                    controls[1], keys[1], newMarginPts, invalidValue, true))
-            .then(
-                () => testControlTextbox(
-                    controls[2], keys[2], newMarginPts, invalidValue, true))
+        // Then verify entering an invalid value invalidates the control
+        // and does not update the settings.
+        const value1 = '1.75';  // 1.75 inches
+        const newMargin1 = Math.round(parseFloat(value1) * pointsPerInch);
+        const value2 = '.6';
+        const newMargin2 = Math.round(parseFloat(value2) * pointsPerInch);
+        const maximumTopMargin = container.pageSize.height - newMargin2 - 72;
+        return testAllTextboxes(controls, defaultMarginPts, value1, false)
+            .then(() => testAllTextboxes(controls, newMargin1, 'abc', true))
+            .then(() => testAllTextboxes(controls, newMargin1, value2, false))
             .then(
                 () => testControlTextbox(
-                    controls[3], keys[3], newMarginPts, invalidValue, true));
+                    controls[0], keys[0], newMargin2, '1,000', false,
+                    container.pageSize.height - newMargin2 -
+                        72 /* MINIMUM_DISTANCE, see margin_control.js */));
       });
     });
 
@@ -561,7 +586,7 @@ cr.define('custom_margins_test', function() {
             const bottomControl = controls[2];
             const whenEventFired =
                 test_util.eventToPromise('text-focus-position', container);
-            bottomControl.$.textbox.focus();
+            bottomControl.$.input.focus();
             // Workaround for mac so that this does not need to be an
             // interactive test: manually fire the focus event from the control.
             bottomControl.fire('text-focus');