fix(slider): don't emit change events on mousedown (#20240)

Currently when the user presses down somewhere on the slider we emit a `change` event, as well as when they release. While we don't emit the same value twice, the timing seems to be unexpected by consumers, because one happens at the begging of the sequence and another one at the end.

These changes make it so that we only emit the change event once the user has released the slider.

Fixes #14363.

Author: crisbeto

src/material/slider/slider.spec.ts

@@ -763,11 +763,12 @@ describe('MatSlider', () => {
       sliderNativeElement = sliderDebugElement.nativeElement;
     });
 
-    it('should emit change on mousedown', () => {
+    it('should emit change on mouseup', () => {
       expect(testComponent.onChange).not.toHaveBeenCalled();
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.2);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.2);
 
       expect(testComponent.onChange).toHaveBeenCalledTimes(1);
     });
@@ -799,12 +800,15 @@ describe('MatSlider', () => {
       dispatchMousedownEventSequence(sliderNativeElement, 0.2);
       fixture.detectChanges();
 
-      expect(testComponent.onChange).toHaveBeenCalledTimes(1);
+      expect(testComponent.onChange).not.toHaveBeenCalled();
       expect(testComponent.onInput).toHaveBeenCalledTimes(1);
 
       dispatchSlideEndEvent(sliderNativeElement, 0.2);
       fixture.detectChanges();
 
+      expect(testComponent.onChange).toHaveBeenCalledTimes(1);
+      expect(testComponent.onInput).toHaveBeenCalledTimes(1);
+
       testComponent.slider.value = 0;
       fixture.detectChanges();
 
@@ -813,6 +817,7 @@ describe('MatSlider', () => {
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.2);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.2);
 
       expect(testComponent.onChange).toHaveBeenCalledTimes(2);
       expect(testComponent.onInput).toHaveBeenCalledTimes(2);
@@ -857,8 +862,8 @@ describe('MatSlider', () => {
       expect(testComponent.onChange).not.toHaveBeenCalled();
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.75);
-
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.75);
 
       // The `onInput` event should be emitted once due to a single click.
       expect(testComponent.onInput).toHaveBeenCalledTimes(1);
@@ -1270,11 +1275,12 @@ describe('MatSlider', () => {
       sliderNativeElement = sliderDebugElement.nativeElement;
     });
 
-    it('should update the model on mousedown', () => {
+    it('should update the model on mouseup', () => {
       expect(testComponent.val).toBe(0);
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.76);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.76);
 
       expect(testComponent.val).toBe(76);
     });
@@ -1342,11 +1348,12 @@ describe('MatSlider', () => {
       expect(testComponent.control.value).toBe(0);
     });
 
-    it('should update the control on mousedown', () => {
+    it('should update the control on mouseup', () => {
       expect(testComponent.control.value).toBe(0);
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.76);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.76);
 
       expect(testComponent.control.value).toBe(76);
     });
@@ -1399,6 +1406,7 @@ describe('MatSlider', () => {
       // but remain untouched.
       dispatchMousedownEventSequence(sliderNativeElement, 0.5);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.5);
 
       expect(sliderControl.valid).toBe(true);
       expect(sliderControl.pristine).toBe(false);
@@ -1435,6 +1443,7 @@ describe('MatSlider', () => {
 
       dispatchMousedownEventSequence(sliderNativeElement, 0.1);
       fixture.detectChanges();
+      dispatchSlideEndEvent(sliderNativeElement, 0.1);
 
       expect(testComponent.value).toBe(10);
       expect(testComponent.slider.value).toBe(10);

src/material/slider/slider.ts

@@ -470,9 +470,6 @@ export class MatSlider extends _MatSliderMixinBase
   /** The value of the slider when the slide start event fires. */
   private _valueOnSlideStart: number | null;
 
-  /** Position of the pointer when the dragging started. */
-  private _pointerPositionOnStart: {x: number, y: number} | null;
-
   /** Reference to the inner slider wrapper element. */
   @ViewChild('sliderWrapper') private _sliderWrapper: ElementRef;
 
@@ -639,13 +636,11 @@ export class MatSlider extends _MatSliderMixinBase
       this._bindGlobalEvents(event);
       this._focusHostElement();
       this._updateValueFromPosition(pointerPosition);
-      this._valueOnSlideStart = this.value;
-      this._pointerPositionOnStart = pointerPosition;
+      this._valueOnSlideStart = oldValue;
 
       // Emit a change and input event if the value changed.
       if (oldValue != this.value) {
         this._emitInputEvent();
-        this._emitChangeEvent();
       }
     });
   }
@@ -672,19 +667,15 @@ export class MatSlider extends _MatSliderMixinBase
   /** Called when the user has lifted their pointer. Bound on the document level. */
   private _pointerUp = (event: TouchEvent | MouseEvent) => {
     if (this._isSliding) {
-      const pointerPositionOnStart = this._pointerPositionOnStart;
-      const currentPointerPosition = getPointerPositionOnPage(event);
-
       event.preventDefault();
       this._removeGlobalEvents();
-      this._valueOnSlideStart = this._pointerPositionOnStart = this._lastPointerEvent = null;
       this._isSliding = false;
 
-      if (this._valueOnSlideStart != this.value && !this.disabled &&
-          pointerPositionOnStart && (pointerPositionOnStart.x !== currentPointerPosition.x ||
-          pointerPositionOnStart.y !== currentPointerPosition.y)) {
+      if (this._valueOnSlideStart != this.value && !this.disabled) {
         this._emitChangeEvent();
       }
+
+      this._valueOnSlideStart = this._lastPointerEvent = null;
     }
   }