Settings WebUI: fix flaky test, add a wait to setup before running test cases

Patchset 1 is the revert. Patchset 2 is the fix.

Bug: 916888
Change-Id: I16e2fc111c8ca6e3477414f4ed893bd4da0e2087
Reviewed-on: https://chromium-review.googlesource.com/c/1387386Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618366}

chrome/browser/resources/settings/controls/settings_slider.html

@@ -53,9 +53,8 @@
     </template>
     <div class="outer">
       <cr-slider id="slider" disabled$="[[disableSlider_]]" ticks="[[ticks]]"
-          on-value-changed="onSliderChanged_" max="[[max]]" min="[[min]]"
-          on-dragging-changed="onSliderChanged_">
-      </cr-slider>
+          on-cr-slider-value-changed="onSliderChanged_" max="[[max]]"
+          min="[[min]]" on-dragging-changed="onSliderChanged_"></cr-slider>
       <div id="labels" disabled$="[[disableSlider_]]">
         <div id="label-begin">[[labelMin]]</div>
         <div id="label-end">[[labelMax]]</div>

chrome/browser/resources/settings/controls/settings_slider.js

@@ -134,9 +134,20 @@ Polymer({
     }
 
     const prefValue = /** @type {number} */ (this.pref.value);
-    // If |ticks| is empty, simply set current value to the slider.
+
+    // The preference and slider values are continuous when |ticks| is empty.
     if (numTicks == 0) {
-      this.$.slider.value = prefValue * this.scale;
+      // This method is handling a preference value change. If the slider is,
+      // in a dragging state, that change is discarded and the the preference
+      // value is updated based on the slider value.
+      if (this.$.slider.dragging) {
+        const prefValueFromSlider = this.$.slider.value / this.scale;
+        if (this.updateValueInstantly && prefValue != prefValueFromSlider)
+          this.set('pref.value', prefValueFromSlider);
+      } else {
+        // When not dragging, simply update the slider value.
+        this.$.slider.value = prefValue * this.scale;
+      }
       return;
     }
 
@@ -146,28 +157,27 @@ Polymer({
     this.$.slider.markerCount =
         (this.showMarkers || numTicks <= MAX_TICKS) ? numTicks : 0;
 
-    const tickValue = this.getTickValueAtIndex_(this.$.slider.value);
-    if (this.$.slider.dragging && this.pref.value != tickValue) {
-      if (!this.updateValueInstantly)
-        return;
-
-      // The value changed outside settings-slider but we're still holding the
-      // knob, so set the value back to where the knob was.
-      // Async so we don't confuse Polymer's data binding.
-      this.async(() => {
+    if (this.$.slider.dragging) {
+      const tickValue = this.getTickValueAtIndex_(this.$.slider.value);
+      if (this.updateValueInstantly && this.pref.value != tickValue)
         this.set('pref.value', tickValue);
-      });
+
       return;
     }
 
     // Convert from the public |value| to the slider index (where the knob
     // should be positioned on the slider).
-    this.$.slider.value =
+    const index =
         this.ticks.map(tick => Math.abs(this.getTickValue_(tick) - prefValue))
             .reduce(
                 (acc, diff, index) => diff < acc.diff ? {index, diff} : acc,
                 {index: -1, diff: Number.MAX_VALUE})
             .index;
-    assert(this.$.slider.value != -1);
+    assert(index != -1);
+    if (this.$.slider.value != index)
+      this.$.slider.value = index;
+    const tickValue = this.getTickValueAtIndex_(index);
+    if (this.pref.value != tickValue)
+      this.set('pref.value', tickValue);
   },
 });

chrome/browser/resources/settings/device_page/display.html

@@ -165,7 +165,7 @@
             disabled="[[isDisplayScaleMandatory_(
                 selectedDisplay,
                 prefs.cros.device_display_resolution)]]"
-            on-value-changed="onDisplaySizeSliderDrag_">
+            on-cr-slider-value-changed="onDisplaySizeSliderDrag_">
         </settings-slider>
       </div>
 

chrome/test/data/webui/cr_elements/cr_slider_test.js

@@ -10,6 +10,7 @@ suite('cr-slider', function() {
     document.body.innerHTML = '<cr-slider min="0" max="100"></cr-slider>';
 
     crSlider = document.body.querySelector('cr-slider');
+    return PolymerTest.flushTasks();
   });
 
   /** @param {boolean} expected */
@@ -76,14 +77,6 @@ suite('cr-slider', function() {
     pointerEvent('pointerup', 0);
   }
 
-  // Ensure that value-changed event bubbles, since users of cr-slider rely on
-  // such event.
-  test('value-changed bubbles', function() {
-    const whenFired = test_util.eventToPromise('value-changed', crSlider);
-    crSlider.value = 50;
-    return whenFired;
-  });
-
   test('key events', () => {
     crSlider.value = 0;
     pressArrowRight();
@@ -193,7 +186,7 @@ suite('cr-slider', function() {
     assertEquals('4', crSlider.getAttribute('aria-valuenow'));
     assertEquals('', crSlider.$.label.innerHTML.trim());
     assertEquals(2, crSlider.value);
-    crSlider.value = 100;
+    pressArrowRight();
     assertEquals(3, crSlider.value);
     assertEquals('8', crSlider.getAttribute('aria-valuetext'));
     assertEquals('8', crSlider.getAttribute('aria-valuenow'));
@@ -219,7 +212,7 @@ suite('cr-slider', function() {
     assertEquals('Third', crSlider.getAttribute('aria-valuetext'));
     assertEquals('Third', crSlider.$.label.innerHTML.trim());
     assertEquals('3', crSlider.getAttribute('aria-valuenow'));
-    crSlider.value = 1;
+    pressArrowLeft();
     assertEquals('Second', crSlider.getAttribute('aria-valuetext'));
     assertEquals('20', crSlider.getAttribute('aria-valuenow'));
     assertEquals('Second', crSlider.$.label.innerHTML.trim());
@@ -357,16 +350,14 @@ suite('cr-slider', function() {
     assertEquals(.5, crSlider.getRatio());
   });
 
-  test('cr-slider-value-changed-from-ui event when mouse clicked', () => {
-    const wait =
-        test_util.eventToPromise('cr-slider-value-changed-from-ui', crSlider);
-    pointerDown(0);
+  test('cr-slider-value-changed event when mouse clicked', () => {
+    const wait = test_util.eventToPromise('cr-slider-value-changed', crSlider);
+    pointerDown(.1);
     return wait;
   });
 
-  test('cr-slider-value-changed-from-ui event when key pressed', () => {
-    const wait =
-        test_util.eventToPromise('cr-slider-value-changed-from-ui', crSlider);
+  test('cr-slider-value-changed event when key pressed', () => {
+    const wait = test_util.eventToPromise('cr-slider-value-changed', crSlider);
     pressArrowRight();
     return wait;
   });

chrome/test/data/webui/settings/device_page_tests.js

@@ -590,9 +590,15 @@ cr.define('device_page_tests', function() {
     });
 
     test(assert(TestNames.Keyboard), function() {
+      const name = k => `prefs.settings.language.${k}.value`;
+      const get = k => devicePage.get(name(k));
+      const set = (k, v) => devicePage.set(name(k), v);
+      let keyboardPage;
+      let collapse;
       // Open the keyboard subpage.
       return showAndGetDeviceSubpage('keyboard', settings.routes.KEYBOARD)
-          .then(function(keyboardPage) {
+          .then(function(page) {
+            keyboardPage = page;
             // Initially, the optional keys are hidden.
             expectFalse(!!keyboardPage.$$('#capsLockKey'));
             expectFalse(!!keyboardPage.$$('#diamondKey'));
@@ -653,7 +659,7 @@ cr.define('device_page_tests', function() {
             expectTrue(!!keyboardPage.$$('#externalMetaKey'));
             expectTrue(!!keyboardPage.$$('#externalCommandKey'));
 
-            const collapse = keyboardPage.$$('iron-collapse');
+            collapse = keyboardPage.$$('iron-collapse');
             assertTrue(!!collapse);
             expectTrue(collapse.opened);
 
@@ -668,31 +674,28 @@ cr.define('device_page_tests', function() {
             MockInteractions.pressAndReleaseKeyOn(
                 keyboardPage.$$('#repeatRateSlider').$$('cr-slider'), 39, [],
                 'ArrowRight');
-            const language = devicePage.prefs.settings.language;
-            expectEquals(1000, language.xkb_auto_repeat_delay_r2.value);
-            expectEquals(300, language.xkb_auto_repeat_interval_r2.value);
+            expectEquals(1000, get('xkb_auto_repeat_delay_r2'));
+            expectEquals(300, get('xkb_auto_repeat_interval_r2'));
 
             // Test sliders change when prefs change.
-            devicePage.set(
-                'prefs.settings.language.xkb_auto_repeat_delay_r2.value', 1500);
+            set('xkb_auto_repeat_delay_r2', 1500);
             expectEquals(1500, keyboardPage.$$('#delaySlider').pref.value);
-            devicePage.set(
-                'prefs.settings.language.xkb_auto_repeat_interval_r2.value',
-                2000);
+            set('xkb_auto_repeat_interval_r2', 2000);
             expectEquals(2000, keyboardPage.$$('#repeatRateSlider').pref.value);
 
             // Test sliders round to nearest value when prefs change.
-            devicePage.set(
-                'prefs.settings.language.xkb_auto_repeat_delay_r2.value', 600);
+            set('xkb_auto_repeat_delay_r2', 600);
+            return PolymerTest.flushTasks();
+          })
+          .then(() => {
             expectEquals(500, keyboardPage.$$('#delaySlider').pref.value);
-            devicePage.set(
-                'prefs.settings.language.xkb_auto_repeat_interval_r2.value',
-                45);
+            set('xkb_auto_repeat_interval_r2', 45);
+            return PolymerTest.flushTasks();
+          })
+          .then(() => {
             expectEquals(50, keyboardPage.$$('#repeatRateSlider').pref.value);
 
-            devicePage.set(
-                'prefs.settings.language.xkb_auto_repeat_enabled_r2.value',
-                false);
+            set('xkb_auto_repeat_enabled_r2', false);
             expectFalse(collapse.opened);
 
             // Test keyboard shortcut viewer button.

chrome/test/data/webui/settings/settings_slider_tests.js

@@ -81,6 +81,12 @@ suite('SettingsSlider', function() {
     pointerEvent('pointerup', 0);
   }
 
+  function assertCloseTo(actual, expected) {
+    assertTrue(
+        Math.abs(1 - actual / expected) <= Number.EPSILON,
+        `expected ${expected} to be close to ${actual}`);
+  }
+
   test('enforce value', function() {
     // Test that the indicator is not present until after the pref is
     // enforced.
@@ -107,13 +113,19 @@ suite('SettingsSlider', function() {
     // settings-slider only supports snapping to a range of tick values.
     // Setting to an in-between value should snap to an indexed value.
     slider.set('pref.value', 70);
-    expectEquals(5, crSlider.value);
-    expectEquals(64, slider.pref.value);
-
-    // Setting the value out-of-range should clamp the slider.
-    slider.set('pref.value', -100);
-    expectEquals(0, crSlider.value);
-    expectEquals(2, slider.pref.value);
+    return PolymerTest.flushTasks()
+        .then(() => {
+          expectEquals(5, crSlider.value);
+          expectEquals(64, slider.pref.value);
+
+          // Setting the value out-of-range should clamp the slider.
+          slider.set('pref.value', -100);
+          return PolymerTest.flushTasks();
+        })
+        .then(() => {
+          expectEquals(0, crSlider.value);
+          expectEquals(2, slider.pref.value);
+        });
   });
 
   test('move slider', function() {
@@ -201,7 +213,7 @@ suite('SettingsSlider', function() {
     expectEquals(.4, slider.pref.value);
   });
 
-  test('update value instantly both off and on', () => {
+  test('update value instantly both off and on with ticks', () => {
     slider.ticks = ticks;
     slider.set('pref.value', 2);
     slider.updateValueInstantly = false;
@@ -231,4 +243,35 @@ suite('SettingsSlider', function() {
     assertEquals(2, crSlider.value);
     assertEquals(8, slider.pref.value);
   });
+
+  test('update value instantly both off and on', () => {
+    slider.scale = 10;
+    slider.set('pref.value', 2);
+    slider.updateValueInstantly = false;
+    assertCloseTo(20, crSlider.value);
+    pointerDown(.3);
+    assertCloseTo(30, crSlider.value);
+    assertEquals(2, slider.pref.value);
+    pointerUp();
+    assertCloseTo(30, crSlider.value);
+    assertCloseTo(3, slider.pref.value);
+
+    // Once |updateValueInstantly| is turned on, |value| should start updating
+    // again during drag.
+    pointerDown(0);
+    assertEquals(0, crSlider.value);
+    assertCloseTo(3, slider.pref.value);
+    slider.updateValueInstantly = true;
+    assertEquals(0, slider.pref.value);
+    pointerMove(.1);
+    assertCloseTo(10, crSlider.value);
+    assertCloseTo(1, slider.pref.value);
+    slider.updateValueInstantly = false;
+    pointerMove(.2);
+    assertCloseTo(20, crSlider.value);
+    assertCloseTo(1, slider.pref.value);
+    pointerUp();
+    assertCloseTo(20, crSlider.value);
+    assertCloseTo(2, slider.pref.value);
+  });
 });

ui/file_manager/audio_player/elements/control_panel.js

@@ -139,14 +139,14 @@ var AriaLabels;
      */
     ready: function() {
       var timeSlider = /** @type {!CrSliderElement} */ (this.$.timeSlider);
-      timeSlider.addEventListener('value-changed', function() {
+      timeSlider.addEventListener('cr-slider-value-changed', () => {
         this.time = timeSlider.value;
-      }.bind(this));
+      });
 
       var volumeSlider = /** @type {!CrSliderElement} */ (this.$.volumeSlider);
-      volumeSlider.addEventListener('value-changed', function() {
+      volumeSlider.addEventListener('cr-slider-value-changed', () => {
         this.volume = volumeSlider.value;
-      }.bind(this));
+      });
     },
 
     /**

ui/file_manager/gallery/js/image_editor/image_editor_toolbar.js

@@ -282,16 +282,16 @@ ImageEditorToolbar.prototype.addRange = function(
   slider.min = Math.ceil(min * scale);
   slider.max = Math.floor(max * scale);
   slider.value = value * scale;
-  slider.addEventListener('value-changed', () => {
+  const handler = () => {
     if (this.updateCallback_)
       this.updateCallback_(this.getOptions());
-  });
+  };
+  slider.addEventListener('cr-slider-value-changed', handler);
+  setTimeout(handler);
   range.appendChild(slider);
 
   range.name = name;
-  range.getValue = function(slider, scale) {
-    return slider.value / scale;
-  }.bind(this, slider, scale);
+  range.getValue = () => slider.value / scale;
 
   // Swallow the left and right keys, so they are not handled by other
   // listeners.

ui/file_manager/video_player/js/media_controls.js

@@ -382,11 +382,10 @@ MediaControls.prototype.initTimeControls = function(opt_parent) {
   this.progressSlider_.addEventListener('dragging-changed', event => {
     this.setSeeking_(event.detail.value);
   });
-  this.progressSlider_.addEventListener(
-      'cr-slider-value-changed-from-ui', () => {
-        this.onProgressChange_();
-        this.updateTimeFromSlider_();
-      });
+  this.progressSlider_.addEventListener('cr-slider-value-changed', () => {
+    this.onProgressChange_();
+    this.updateTimeFromSlider_();
+  });
   timeControls.appendChild(this.progressSlider_);
 };
 
@@ -575,7 +574,7 @@ MediaControls.prototype.initVolumeControls = function(opt_parent) {
   this.volume_.setAttribute('aria-label',
       str('MEDIA_PLAYER_VOLUME_SLIDER_LABEL'));
   this.volume_.addEventListener(
-      'cr-slider-value-changed-from-ui', this.onVolumeChange_.bind(this));
+      'cr-slider-value-changed', this.onVolumeChange_.bind(this));
   this.loadVolumeControlState();
   volumeControls.appendChild(this.volume_);
 };

ui/file_manager/video_player/js/video_player.js

@@ -96,7 +96,7 @@ function FullWindowVideoControls(
   document.addEventListener('keypress', function(e) {
     this.inactivityWatcher_.kick();
   }.wrap(this));
-  controlsContainer.addEventListener('cr-slider-value-changed-from-ui', () => {
+  controlsContainer.addEventListener('cr-slider-value-changed', () => {
     this.inactivityWatcher_.kick();
   });
 

ui/webui/resources/cr_elements/cr_slider/cr_slider.js

@@ -37,9 +37,8 @@ cr_slider.SliderTick;
   /**
    * The following are the events emitted from cr-slider.
    *
-   * cr-slider-value-changed-from-ui: fired when updating slider via the UI.
+   * cr-slider-value-changed: fired when updating slider via the UI.
    * dragging-changed: fired on pointer down and on pointer up.
-   * value-changed: fired anytime |value| is changed, manually or via the UI.
    */
   Polymer({
     is: 'cr-slider',
@@ -116,8 +115,6 @@ cr_slider.SliderTick;
       value: {
         type: Number,
         value: 0,
-        notify: true,
-        observer: 'onValueChanged_',
       },
 
       /** @private */
@@ -233,15 +230,6 @@ cr_slider.SliderTick;
       return (this.value - this.min) / (this.max - this.min);
     },
 
-    /** @private */
-    ensureValidValue_: function() {
-      if (this.value == undefined)
-        return;
-      let validValue = clamp(this.min, this.max, this.value);
-      validValue = this.snaps ? Math.round(validValue) : validValue;
-      this.value = validValue;
-    },
-
     /**
      * Removes all event listeners related to dragging, and cancels ripple.
      * @param {number} pointerId
@@ -295,26 +283,21 @@ cr_slider.SliderTick;
       if (event.metaKey || event.shiftKey || event.altKey || event.ctrlKey)
         return;
 
-      let handled = true;
       if (event.key == 'Home') {
-        this.value = this.min;
+        this.updateValue_(this.min);
       } else if (event.key == 'End') {
-        this.value = this.max;
+        this.updateValue_(this.max);
       } else if (this.deltaKeyMap_.has(event.key)) {
-        const newValue = this.value + this.deltaKeyMap_.get(event.key);
-        this.value = clamp(this.min, this.max, newValue);
+        this.updateValue_(this.value + this.deltaKeyMap_.get(event.key));
       } else {
-        handled = false;
+        return;
       }
 
-      if (handled) {
-        this.fire('cr-slider-value-changed-from-ui');
-        event.preventDefault();
-        event.stopPropagation();
-        setTimeout(() => {
-          this.holdDown_ = true;
-        });
-      }
+      event.preventDefault();
+      event.stopPropagation();
+      setTimeout(() => {
+        this.holdDown_ = true;
+      });
     },
 
     /**
@@ -388,17 +371,7 @@ cr_slider.SliderTick;
         this.max = this.ticks.length - 1;
         this.min = 0;
       }
-      this.ensureValidValue_();
-      this.updateLabelAndAria_();
-    },
-
-    /**
-     * Update |value| which is used for rendering when |value| is
-     * updated either programmatically or from a keyboard input or a mouse drag.
-     * @private
-     */
-    onValueChanged_: function() {
-      this.ensureValidValue_();
+      this.updateValue_(this.value);
     },
 
     /** @private */
@@ -424,7 +397,7 @@ cr_slider.SliderTick;
       this.label_ = Number.isFinite(tick) ? '' : tick.label;
 
       // Update label location after it has been rendered.
-      this.async(() => {
+      setTimeout(() => {
         const label = this.$.label;
         const parentWidth = label.parentElement.offsetWidth;
         const labelWidth = label.offsetWidth;
@@ -432,11 +405,7 @@ cr_slider.SliderTick;
         const margin = 16;
         const knobLocation = parentWidth * this.getRatio() + margin;
         const offsetStart = knobLocation - (labelWidth / 2);
-        // The label should be centered over the knob. Clamping the offset to a
-        // min and max value prevents the label from being cutoff.
-        const max = parentWidth + 2 * margin - labelWidth;
-        label.style.marginInlineStart =
-            `${Math.round(clamp(0, max, offsetStart))}px`;
+        label.style.marginInlineStart = `${Math.round(offsetStart)}px`;
       });
 
       const ariaValues = [tick, ticks[0], ticks[ticks.length - 1]].map(t => {
@@ -452,6 +421,19 @@ cr_slider.SliderTick;
       this.setAttribute('aria-valuemax', ariaValues[2]);
     },
 
+    /**
+     * @param {number} value
+     * @private
+     */
+    updateValue_: function(value) {
+      let validValue = clamp(this.min, this.max, value);
+      validValue = this.snaps ? Math.round(validValue) : validValue;
+      if (this.value != validValue) {
+        this.value = validValue;
+        this.fire('cr-slider-value-changed');
+      }
+    },
+
     /**
      * @param {number} clientX
      * @private
@@ -461,8 +443,7 @@ cr_slider.SliderTick;
       let ratio = (clientX - rect.left) / rect.width;
       if (this.isRtl_)
         ratio = 1 - ratio;
-      this.value = ratio * (this.max - this.min) + this.min;
-      this.fire('cr-slider-value-changed-from-ui');
+      this.updateValue_(ratio * (this.max - this.min) + this.min);
     },
 
     _createRipple: function() {