Print Preview: Move all setSetting('margins') to margins-settings

This fixes a Polymer timing issue where setting the margins in the
margin-control-container was causing the observer in margins-settings
that updated the UI to not fire.

Also moving the margins setter in pages_per_sheet to margins for
consistency.

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

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

@@ -194,13 +194,8 @@ Polymer({
     }
 
     this.resetMargins_ = true;
-    const marginsSetting = this.getSetting('margins');
-    if (marginsSetting.value === MarginsType.CUSTOM) {
-      // Set the margins value to default first.
-      this.setSetting('margins', MarginsType.DEFAULT);
-    }
-    // Reset custom margins so that the sticky value is not restored for the
-    // new paper size.
+    // Reset custom margins so that the sticky value is not restored for the new
+    // paper size.
     this.setSetting('customMargins', {});
   },
 

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

@@ -4,8 +4,7 @@
   <span id="margins-label" slot="title">$i18n{marginsLabel}</span>
   <div slot="controls">
     <select class="md-select" aria-labelledby="margins-label"
-        disabled$="[[getMarginsSettingsDisabled_(disabled,
-                         settings.pagesPerSheet.value)]]"
+        disabled$="[[marginsDisabled_]]"
         value="{{selectedValue::change}}">
       <!-- The order of these options must match the natural order of their
       values, which come from MarginsType. -->

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

@@ -9,6 +9,7 @@ import './settings_section.js';
 import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
 import {MarginsType} from '../data/margins.js';
+import {State} from '../data/state.js';
 
 import {SelectBehavior} from './select_behavior.js';
 import {SettingsBehavior} from './settings_behavior.js';
@@ -21,23 +22,66 @@ Polymer({
   behaviors: [SettingsBehavior, SelectBehavior],
 
   properties: {
-    disabled: Boolean,
+    disabled: {
+      type: Boolean,
+      observer: 'updateMarginsDisabled_',
+    },
+
+    /** @type {!State} */
+    state: {
+      type: Number,
+      observer: 'onStateChange_',
+    },
+
+    /** @private */
+    marginsDisabled_: Boolean,
 
     /** Mirroring the enum so that it can be used from HTML bindings. */
     MarginsTypeEnum: Object,
   },
 
-  observers: ['onMarginsSettingChange_(settings.margins.value)'],
+  observers: [
+    'onMarginsSettingChange_(settings.margins.value)',
+    'onMediaSizeOrLayoutChange_(' +
+        'settings.mediaSize.value, settings.layout.value)',
+    'onPagesPerSheetSettingChange_(settings.pagesPerSheet.value)'
+  ],
+
+  /** @private {boolean} */
+  loaded_: false,
 
   /** @override */
   ready() {
     this.MarginsTypeEnum = MarginsType;
   },
 
+  /** @private */
+  onStateChange_() {
+    if (this.state === State.READY) {
+      this.loaded_ = true;
+    }
+  },
+
+  /** @private */
+  onMediaSizeOrLayoutChange_() {
+    if (this.loaded_ &&
+        this.getSetting('margins').value === MarginsType.CUSTOM) {
+      this.setSetting('margins', MarginsType.DEFAULT);
+    }
+  },
+
   /**
-   * @param {*} newValue The new value of the margins setting.
+   * @param {number} newValue The new value of the pages per sheet setting.
    * @private
    */
+  onPagesPerSheetSettingChange_(newValue) {
+    if (newValue > 1) {
+      this.setSetting('margins', MarginsType.DEFAULT);
+    }
+    this.updateMarginsDisabled_();
+  },
+
+  /** @param {*} newValue The new value of the margins setting. */
   onMarginsSettingChange_(newValue) {
     this.selectedValue =
         /** @type {!MarginsType} */ (newValue).toString();
@@ -48,13 +92,10 @@ Polymer({
     this.setSetting('margins', parseInt(value, 10));
   },
 
-  /**
-   * @param {boolean} globallyDisabled Value of the |disabled| property.
-   * @param {number} pagesPerSheet Number of pages per sheet.
-   * @return {boolean} Whether the margins settings button should be disabled.
-   * @private
-   */
-  getMarginsSettingsDisabled_(globallyDisabled, pagesPerSheet) {
-    return globallyDisabled || pagesPerSheet > 1;
+  /** @private */
+  updateMarginsDisabled_() {
+    this.marginsDisabled_ =
+        /** @type {number} */ (this.getSettingValue('pagesPerSheet')) > 1 ||
+        this.disabled;
   },
 });

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

@@ -32,7 +32,6 @@ Polymer({
    */
   onPagesPerSheetSettingChange_(newValue) {
     this.selectedValue = /** @type {number} */ (newValue).toString();
-    this.setSetting('margins', MarginsType.DEFAULT);
   },
 
   /** @param {string} value The new select value. */

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

@@ -90,7 +90,7 @@
         hidden$="[[!settings.pagesPerSheet.available]]"
         class="settings-section">
     </print-preview-pages-per-sheet-settings>
-    <print-preview-margins-settings settings="[[settings]]"
+    <print-preview-margins-settings settings="[[settings]]" state="[[state]]"
         disabled="[[controlsDisabled_]]"
         hidden$="[[!settings.margins.available]]"
         class="settings-section">

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

@@ -246,11 +246,11 @@ suite(custom_margins_test.suiteName, function() {
       container.setSetting(settingName, newValue);
       container.previewLoaded = false;
 
-      // Margins should be reset to default and custom margins values should
-      // be cleared.
-      expectEquals(MarginsType.DEFAULT, container.getSettingValue('margins'));
+      // Custom margins values should be cleared.
       expectEquals(
           '{}', JSON.stringify(container.getSettingValue('customMargins')));
+      // The margins-settings element will also set the margins type to DEFAULT.
+      model.set('settings.margins.value', MarginsType.DEFAULT);
 
       // When preview loads, custom margins should still be empty, since
       // custom margins are not selected. We do not want to set the sticky

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

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {MarginsType} from 'chrome://print/print_preview.js';
+import {MarginsType, State} from 'chrome://print/print_preview.js';
 import {assert} from 'chrome://resources/js/assert.m.js';
 import {selectOption} from 'chrome://test/print_preview/print_preview_test_utils.js';
 import {eventToPromise, fakeDataBind} from 'chrome://test/test_util.m.js';
@@ -12,16 +12,19 @@ suite('MarginsSettingsTest', function() {
 
   let marginsTypeEnum = null;
 
+  let model = null;
+
   /** @override */
   setup(function() {
     PolymerTest.clearBody();
-    const model = document.createElement('print-preview-model');
+    model = document.createElement('print-preview-model');
     document.body.appendChild(model);
 
     marginsSection = document.createElement('print-preview-margins-settings');
     document.body.appendChild(marginsSection);
     marginsSection.settings = model.settings;
     marginsSection.disabled = false;
+    marginsSection.state = State.READY;
     fakeDataBind(model, marginsSection, 'settings');
     marginsTypeEnum = MarginsType;
   });
@@ -54,15 +57,53 @@ suite('MarginsSettingsTest', function() {
   });
 
   // This test verifies that changing pages per sheet to N > 1 disables the
-  // margins dropdown.
-  test('disabled by pages per sheet', function() {
+  // margins dropdown and changes the value to DEFAULT.
+  test('disabled by pages per sheet', async () => {
     const select = marginsSection.$$('select');
+    await selectOption(marginsSection, marginsTypeEnum.MINIMUM.toString());
+    assertEquals(
+        marginsTypeEnum.MINIMUM, marginsSection.getSettingValue('margins'));
     assertFalse(select.disabled);
 
-    marginsSection.setSetting('pagesPerSheet', 2);
+    model.set('settings.pagesPerSheet.value', 2);
+    await eventToPromise('process-select-change', marginsSection);
+    assertEquals(
+        marginsTypeEnum.DEFAULT, marginsSection.getSettingValue('margins'));
+    assertEquals(marginsTypeEnum.DEFAULT.toString(), select.value);
     assertTrue(select.disabled);
 
-    marginsSection.setSetting('pagesPerSheet', 1);
+    model.set('settings.pagesPerSheet.value', 1);
+    assertEquals(
+        marginsTypeEnum.DEFAULT, marginsSection.getSettingValue('margins'));
     assertFalse(select.disabled);
   });
+
+  // Test that changing the layout or media size setting clears a custom
+  // margins setting.
+  test('custom margins cleared by layout and media size', async () => {
+    const select = marginsSection.$$('select');
+    await selectOption(marginsSection, marginsTypeEnum.CUSTOM.toString());
+    assertEquals(
+        marginsTypeEnum.CUSTOM, marginsSection.getSettingValue('margins'));
+
+    // Changing layout clears custom margins.
+    model.set('settings.layout.value', true);
+    await eventToPromise('process-select-change', marginsSection);
+    assertEquals(
+        marginsTypeEnum.DEFAULT, marginsSection.getSettingValue('margins'));
+    assertEquals(marginsTypeEnum.DEFAULT.toString(), select.value);
+
+    await selectOption(marginsSection, marginsTypeEnum.CUSTOM.toString());
+    assertEquals(
+        marginsTypeEnum.CUSTOM, marginsSection.getSettingValue('margins'));
+
+    // Changing media size clears custom margins.
+    model.set(
+        'settings.mediaSize.value',
+        '{height_microns: 400, width_microns: 300}');
+    await eventToPromise('process-select-change', marginsSection);
+    assertEquals(
+        marginsTypeEnum.DEFAULT, marginsSection.getSettingValue('margins'));
+    assertEquals(marginsTypeEnum.DEFAULT.toString(), select.value);
+  });
 });

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

@@ -34,17 +34,6 @@ suite('PagesPerSheetSettingsTest', function() {
     assertEquals('4', select.value);
   });
 
-  // Tests that setting the pages per sheet setting resets margins to DEFAULT.
-  test('resets margins setting', async () => {
-    pagesPerSheetSection.setSetting('margins', MarginsType.NO_MARGINS);
-    assertEquals(1, pagesPerSheetSection.getSettingValue('pagesPerSheet'));
-    pagesPerSheetSection.setSetting('pagesPerSheet', 4);
-    await eventToPromise('process-select-change', pagesPerSheetSection);
-    assertEquals(4, pagesPerSheetSection.getSettingValue('pagesPerSheet'));
-    assertEquals(
-        MarginsType.DEFAULT, pagesPerSheetSection.getSettingValue('margins'));
-  });
-
   // Tests that selecting a new option in the dropdown updates the setting.
   test('select option', async () => {
     // Verify that the selected option and names are as expected.