Settings WebUI: settings-slider, handle keyboard repeat without feedback loop

Bug: 949479
Change-Id: Icace96388ff832fadbf69918c1aa804b07bfc2bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596623
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657790}

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

@@ -50,7 +50,7 @@
       }
 
       #label-end {
-        -webkit-margin-begin: 4px;
+        margin-inline-start: 4px;
       }
     </style>
     <template is="dom-if" if="[[pref.controlledBy]]" restamp>
@@ -59,7 +59,8 @@
     <div class="outer">
       <cr-slider id="slider" disabled$="[[disableSlider_]]" ticks="[[ticks]]"
           on-cr-slider-value-changed="onSliderChanged_" max="[[max]]"
-          min="[[min]]" on-dragging-changed="onSliderChanged_"></cr-slider>
+          min="[[min]]" on-dragging-changed="onSliderChanged_"
+          on-updating-from-key="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

@@ -126,7 +126,8 @@ Polymer({
    * @private
    */
   valueChanged_: function() {
-    if (this.pref == undefined || !this.loaded_) {
+    if (this.pref == undefined || !this.loaded_ || this.$.slider.dragging ||
+        this.$.slider.updatingFromKey) {
       return;
     }
 
@@ -141,18 +142,7 @@ Polymer({
 
     // The preference and slider values are continuous when |ticks| is empty.
     if (numTicks == 0) {
-      // 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;
-      }
+      this.$.slider.value = prefValue * this.scale;
       return;
     }
 
@@ -162,15 +152,6 @@ Polymer({
     this.$.slider.markerCount =
         (this.showMarkers || numTicks <= MAX_TICKS) ? numTicks : 0;
 
-    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).
     const index =

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

@@ -82,6 +82,7 @@ CrSettingsSliderTest.prototype = {
 
   /** @override */
   extraLibraries: CrSettingsBrowserTest.prototype.extraLibraries.concat([
+    'test_util.js',
     'settings_slider_tests.js',
   ]),
 };

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

@@ -27,36 +27,41 @@ suite('SettingsSlider', function() {
     return PolymerTest.flushTasks();
   });
 
+  function press(key) {
+    MockInteractions.keyDownOn(crSlider, null, [], key);
+    MockInteractions.keyUpOn(crSlider, null, [], key);
+  }
+
   function pressArrowRight() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 39, [], 'ArrowRight');
+    press('ArrowRight');
   }
 
   function pressArrowLeft() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 37, [], 'ArrowLeft');
+    press('ArrowLeft');
   }
 
   function pressPageUp() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 33, [], 'PageUp');
+    press('PageUp');
   }
 
   function pressPageDown() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 34, [], 'PageDown');
+    press('PageDown');
   }
 
   function pressArrowUp() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 38, [], 'ArrowUp');
+    press('ArrowUp');
   }
 
   function pressArrowDown() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 40, [], 'ArrowDown');
+    press('ArrowDown');
   }
 
   function pressHome() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 36, [], 'Home');
+    press('Home');
   }
 
   function pressEnd() {
-    MockInteractions.pressAndReleaseKeyOn(crSlider, 35, [], 'End');
+    press('End');
   }
 
   function pointerEvent(eventType, ratio) {
@@ -87,6 +92,15 @@ suite('SettingsSlider', function() {
         `expected ${expected} to be close to ${actual}`);
   }
 
+  async function checkSliderValueFromPref(prefValue, sliderValue) {
+    assertNotEquals(sliderValue, crSlider.value);
+    if (crSlider.updatingFromKey) {
+      await test_util.eventToPromise('updating-from-key-changed', crSlider);
+    }
+    slider.set('pref.value', prefValue);
+    assertEquals(sliderValue, crSlider.value);
+  }
+
   test('enforce value', function() {
     // Test that the indicator is not present until after the pref is
     // enforced.
@@ -103,124 +117,110 @@ suite('SettingsSlider', function() {
     assertTrue(!!indicator);
   });
 
-  test('set value', function() {
+  test('set value', async () => {
     slider.ticks = ticks;
-    slider.set('pref.value', 16);
-    Polymer.dom.flush();
-    expectEquals(6, crSlider.max);
-    expectEquals(3, crSlider.value);
+    await checkSliderValueFromPref(8, 2);
+    assertEquals(6, crSlider.max);
 
     // 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);
-    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);
-        });
+    await checkSliderValueFromPref(70, 5);
+    assertEquals(64, slider.pref.value);
+
+    // Setting the value out-of-range should clamp the slider.
+    await checkSliderValueFromPref(-100, 0);
+    assertEquals(2, slider.pref.value);
   });
 
-  test('move slider', function() {
+  test('move slider', async () => {
     slider.ticks = ticks;
-    slider.set('pref.value', 30);
-    expectEquals(4, crSlider.value);
+    await checkSliderValueFromPref(30, 4);
 
     pressArrowRight();
-    expectEquals(5, crSlider.value);
-    expectEquals(64, slider.pref.value);
+    assertEquals(5, crSlider.value);
+    assertEquals(64, slider.pref.value);
 
     pressArrowRight();
-    expectEquals(6, crSlider.value);
-    expectEquals(128, slider.pref.value);
+    assertEquals(6, crSlider.value);
+    assertEquals(128, slider.pref.value);
 
     pressArrowRight();
-    expectEquals(6, crSlider.value);
-    expectEquals(128, slider.pref.value);
+    assertEquals(6, crSlider.value);
+    assertEquals(128, slider.pref.value);
 
     pressArrowLeft();
-    expectEquals(5, crSlider.value);
-    expectEquals(64, slider.pref.value);
+    assertEquals(5, crSlider.value);
+    assertEquals(64, slider.pref.value);
 
     pressPageUp();
-    expectEquals(6, crSlider.value);
-    expectEquals(128, slider.pref.value);
+    assertEquals(6, crSlider.value);
+    assertEquals(128, slider.pref.value);
 
     pressPageDown();
-    expectEquals(5, crSlider.value);
-    expectEquals(64, slider.pref.value);
+    assertEquals(5, crSlider.value);
+    assertEquals(64, slider.pref.value);
 
     pressHome();
-    expectEquals(0, crSlider.value);
-    expectEquals(2, slider.pref.value);
+    assertEquals(0, crSlider.value);
+    assertEquals(2, slider.pref.value);
 
     pressArrowDown();
-    expectEquals(0, crSlider.value);
-    expectEquals(2, slider.pref.value);
+    assertEquals(0, crSlider.value);
+    assertEquals(2, slider.pref.value);
 
     pressArrowUp();
-    expectEquals(1, crSlider.value);
-    expectEquals(4, slider.pref.value);
+    assertEquals(1, crSlider.value);
+    assertEquals(4, slider.pref.value);
 
     pressEnd();
-    expectEquals(6, crSlider.value);
-    expectEquals(128, slider.pref.value);
+    assertEquals(6, crSlider.value);
+    assertEquals(128, slider.pref.value);
   });
 
-  test('scaled slider', function() {
-    slider.set('pref.value', 2);
-    expectEquals(2, crSlider.value);
+  test('scaled slider', async () => {
+    await checkSliderValueFromPref(2, 2);
 
     slider.scale = 10;
     slider.max = 4;
     pressArrowRight();
-    expectEquals(3, crSlider.value);
-    expectEquals(.3, slider.pref.value);
+    assertEquals(3, crSlider.value);
+    assertEquals(.3, slider.pref.value);
 
     pressArrowRight();
-    expectEquals(4, crSlider.value);
-    expectEquals(.4, slider.pref.value);
+    assertEquals(4, crSlider.value);
+    assertEquals(.4, slider.pref.value);
 
     pressArrowRight();
-    expectEquals(4, crSlider.value);
-    expectEquals(.4, slider.pref.value);
+    assertEquals(4, crSlider.value);
+    assertEquals(.4, slider.pref.value);
 
     pressHome();
-    expectEquals(0, crSlider.value);
-    expectEquals(0, slider.pref.value);
+    assertEquals(0, crSlider.value);
+    assertEquals(0, slider.pref.value);
 
     pressEnd();
-    expectEquals(4, crSlider.value);
-    expectEquals(.4, slider.pref.value);
+    assertEquals(4, crSlider.value);
+    assertEquals(.4, slider.pref.value);
 
-    slider.set('pref.value', .25);
-    expectEquals(2.5, crSlider.value);
-    expectEquals(.25, slider.pref.value);
+    await checkSliderValueFromPref(.25, 2.5);
+    assertEquals(.25, slider.pref.value);
 
     pressPageUp();
-    expectEquals(3.5, crSlider.value);
-    expectEquals(.35, slider.pref.value);
+    assertEquals(3.5, crSlider.value);
+    assertEquals(.35, slider.pref.value);
 
     pressPageUp();
-    expectEquals(4, crSlider.value);
-    expectEquals(.4, slider.pref.value);
+    assertEquals(4, crSlider.value);
+    assertEquals(.4, slider.pref.value);
   });
 
-  test('update value instantly both off and on with ticks', () => {
+  test('update value instantly both off and on with ticks', async () => {
     slider.ticks = ticks;
-    slider.set('pref.value', 2);
+    await checkSliderValueFromPref(4, 1);
     slider.updateValueInstantly = false;
-    assertEquals(0, crSlider.value);
     pointerDown(3 / crSlider.max);
     assertEquals(3, crSlider.value);
-    assertEquals(2, slider.pref.value);
+    assertEquals(4, slider.pref.value);
     pointerUp();
     assertEquals(3, crSlider.value);
     assertEquals(16, slider.pref.value);
@@ -244,11 +244,10 @@ suite('SettingsSlider', function() {
     assertEquals(8, slider.pref.value);
   });
 
-  test('update value instantly both off and on', () => {
+  test('update value instantly both off and on', async () => {
     slider.scale = 10;
-    slider.set('pref.value', 2);
+    await checkSliderValueFromPref(2, 20);
     slider.updateValueInstantly = false;
-    assertCloseTo(20, crSlider.value);
     pointerDown(.3);
     assertCloseTo(30, crSlider.value);
     assertEquals(2, slider.pref.value);

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

@@ -86,6 +86,12 @@ cr_slider.SliderTick;
         reflectToAttribute: true,
       },
 
+      updatingFromKey: {
+        type: Boolean,
+        value: false,
+        notify: true,
+      },
+
       markerCount: {
         type: Number,
         value: 0,
@@ -181,6 +187,7 @@ cr_slider.SliderTick;
       focus: 'onFocus_',
       blur: 'onBlur_',
       keydown: 'onKeyDown_',
+      keyup: 'onKeyUp_',
       pointerdown: 'onPointerDown_',
     },
 
@@ -314,6 +321,7 @@ cr_slider.SliderTick;
         return;
       }
 
+      this.updatingFromKey = true;
       if (this.updateValue_(newValue)) {
         this.fire('cr-slider-value-changed');
       }
@@ -324,6 +332,19 @@ cr_slider.SliderTick;
       });
     },
 
+    /**
+     * @param {!Event} event
+     * @private
+     */
+    onKeyUp_: function(event) {
+      if (event.key == 'Home' || event.key == 'End' ||
+          this.deltaKeyMap_.has(event.key)) {
+        setTimeout(() => {
+          this.updatingFromKey = false;
+        });
+      }
+    },
+
     /**
      * This code is taken from cr-input. See https://crbug.com/832177#c31 for
      * the CL that handles escaping focus from the shadow DOM.
@@ -379,7 +400,8 @@ cr_slider.SliderTick;
       this.draggingEventTracker_.add(this, 'pointerdown', stopDragging);
       this.draggingEventTracker_.add(this, 'pointerup', stopDragging);
       this.draggingEventTracker_.add(this, 'keydown', e => {
-        if (e.key == 'Escape' || e.key == 'Tab') {
+        if (e.key == 'Escape' || e.key == 'Tab' || e.key == 'Home' ||
+            e.key == 'End' || this.deltaKeyMap_.has(e.key)) {
           stopDragging();
         }
       });