From 9af7a1f3179cae78df4c292372929509e08b989a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 18 Apr 2017 20:24:08 +0200 Subject: [PATCH] fix(slide-toggle): invalid model change event While initially looking into #4124, there were a few more issues inside of the slide-toggle. * ngModelChange event is dispatched at initialization. * Checked state isn't synchronized when state changes through drag. New state after dragging got overwritten by click event on label. * Removes unnecessary logic inside of `change` listener. Change event doesn't fire if underlying checkbox is disabled. Fixes #4124. --- src/lib/slide-toggle/slide-toggle.spec.ts | 25 +++++++------ src/lib/slide-toggle/slide-toggle.ts | 44 +++++++++++------------ 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/lib/slide-toggle/slide-toggle.spec.ts b/src/lib/slide-toggle/slide-toggle.spec.ts index 7f623a11a12a..4245c8968d72 100644 --- a/src/lib/slide-toggle/slide-toggle.spec.ts +++ b/src/lib/slide-toggle/slide-toggle.spec.ts @@ -295,18 +295,23 @@ describe('MdSlideToggle', () => { expect(slideToggleControl.pristine).toBe(true); expect(slideToggleControl.touched).toBe(false); - // After changing the value programmatically, the control should - // become dirty (not pristine), but remain untouched. + // After changing the value from the view, the control should + // become dirty (not pristine), but remain untouched if focus is still there. slideToggle.checked = true; + + // Dispatch a change event on the input element to fake a user interaction that triggered + // the state change. + dispatchFakeEvent(inputElement, 'change'); + fixture.detectChanges(); expect(slideToggleControl.valid).toBe(true); expect(slideToggleControl.pristine).toBe(false); expect(slideToggleControl.touched).toBe(false); - // After a user interaction occurs (such as a click), the control should remain dirty and - // now also be touched. - labelElement.click(); + // Once the input element looses focus, the control should remain dirty but should + // also turn touched. + dispatchFakeEvent(inputElement, 'blur'); fixture.detectChanges(); expect(slideToggleControl.valid).toBe(true); @@ -324,13 +329,13 @@ describe('MdSlideToggle', () => { expect(slideToggleControl.touched).toBe(false); expect(slideToggleElement.classList).toContain('mat-checked'); - // After a user interaction occurs (such as a click), the control should remain dirty and - // now also be touched. - inputElement.click(); + // Once the input element looses focus, the control should remain dirty but should + // also turn touched. + dispatchFakeEvent(inputElement, 'blur'); fixture.detectChanges(); expect(slideToggleControl.touched).toBe(true); - expect(slideToggleElement.classList).not.toContain('mat-checked'); + expect(slideToggleElement.classList).toContain('mat-checked'); }); // TODO(kara): update when core/testing adds fix @@ -434,7 +439,6 @@ describe('MdSlideToggle', () => { })); it('should prevent the form from submit when being required', () => { - if ('reportValidity' in inputElement === false) { // If the browser does not report the validity then the tests will break. // e.g Safari 8 on Mobile. @@ -442,7 +446,6 @@ describe('MdSlideToggle', () => { } testComponent.isRequired = true; - fixture.detectChanges(); buttonElement.click(); diff --git a/src/lib/slide-toggle/slide-toggle.ts b/src/lib/slide-toggle/slide-toggle.ts index 21927436e996..6edf1d748783 100644 --- a/src/lib/slide-toggle/slide-toggle.ts +++ b/src/lib/slide-toggle/slide-toggle.ts @@ -65,7 +65,6 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA // A unique id for the slide-toggle. By default the id is auto-generated. private _uniqueId = `md-slide-toggle-${++nextId}`; - private _checked: boolean = false; private _color: string; private _isMousedown: boolean = false; private _slideRenderer: SlideToggleRenderer = null; @@ -88,6 +87,9 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA /** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */ @Input() labelPosition: 'before' | 'after' = 'after'; + /** Whether the slide-toggle element is checked or not */ + @Input() checked: boolean = false; + /** Used to set the aria-label attribute on the underlying input element. */ @Input('aria-label') ariaLabel: string = null; @@ -139,9 +141,7 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA } /** - * The onChangeEvent method will be also called on click. - * This is because everything for the slide-toggle is wrapped inside of a label, - * which triggers a onChange event on click. + * This function will called if the underlying input changed its value through user interaction. */ _onChangeEvent(event: Event) { // We always have to stop propagation on the change event. @@ -149,19 +149,22 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA // emit its event object to the component's `change` output. event.stopPropagation(); - // Once a drag is currently in progress, we do not want to toggle the slide-toggle on a click. - if (!this.disabled && !this._slideRenderer.dragging) { - this.toggle(); + // Sync the value from the underlying input element with the slide-toggle component. + this.checked = this._inputElement.nativeElement.checked; - // Emit our custom change event if the native input emitted one. - // It is important to only emit it, if the native input triggered one, because - // we don't want to trigger a change event, when the `checked` variable changes for example. - this._emitChangeEvent(); - } + // Emit our custom change event if the native input emitted one. + // It is important to only emit it, if the native input triggered one, because we don't want + // to trigger a change event, when the `checked` variable changes programmatically. + this._emitChangeEvent(); } _onInputClick(event: Event) { - this.onTouched(); + // In some situations the user will release the mouse on the label element. The label element + // redirects the click to the underlying input element and will result in a value change. + // Prevent the default behavior if dragging, because the value will be set after drag. + if (this._slideRenderer.dragging) { + event.preventDefault(); + } // We have to stop propagation for click events on the visual hidden input element. // By default, when a user clicks on a label element, a generated click event will be @@ -207,16 +210,6 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard'); } - /** Whether the slide-toggle is checked. */ - @Input() - get checked() { return !!this._checked; } - set checked(value) { - if (this.checked !== !!value) { - this._checked = value; - this.onChange(this._checked); - } - } - /** The color of the slide-toggle. Can be primary, accent, or warn. */ @Input() get color(): string { return this._color; } @@ -257,12 +250,15 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA } } - /** Emits the change event to the `change` output EventEmitter */ + /** + * Emits a change event on the `change` output. Also notifies the FormControl about the change. + */ private _emitChangeEvent() { let event = new MdSlideToggleChange(); event.source = this; event.checked = this.checked; this._change.emit(event); + this.onChange(this.checked); }