From ce723693d6bd02f8214ea69aee35916d91f7c2ee Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 14 Aug 2020 17:16:50 +0200 Subject: [PATCH] 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. --- src/material/slider/slider.spec.ts | 19 ++++++++++++++----- src/material/slider/slider.ts | 17 ++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/material/slider/slider.spec.ts b/src/material/slider/slider.spec.ts index 9cbbd3a24c21..cb064373776b 100644 --- a/src/material/slider/slider.spec.ts +++ b/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); diff --git a/src/material/slider/slider.ts b/src/material/slider/slider.ts index 75afd8a6ef6b..d06c9b77241c 100644 --- a/src/material/slider/slider.ts +++ b/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; } }