From e74f20cb5802f67720ad5f64977713376ec4638e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 23 Apr 2017 00:56:10 +0200 Subject: [PATCH] fix(slide-toggle): invalid model change event (#4140)" (#4218) 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 | 45 ++++++++++------------- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/lib/slide-toggle/slide-toggle.spec.ts b/src/lib/slide-toggle/slide-toggle.spec.ts index 8b9270c1fa8f..2a54f0921f6a 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(slideToggleModel.pristine).toBe(true); expect(slideToggleModel.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(slideToggleModel.valid).toBe(true); expect(slideToggleModel.pristine).toBe(false); expect(slideToggleModel.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(slideToggleModel.valid).toBe(true); @@ -324,13 +329,13 @@ describe('MdSlideToggle', () => { expect(slideToggleModel.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(slideToggleModel.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 67087a9afad8..3a81545fe195 100644 --- a/src/lib/slide-toggle/slide-toggle.ts +++ b/src/lib/slide-toggle/slide-toggle.ts @@ -83,7 +83,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase // 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 _slideRenderer: SlideToggleRenderer = null; private _required: boolean = false; private _disableRipple: boolean = false; @@ -103,6 +102,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase /** 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; @@ -152,9 +154,7 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase } /** - * 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. @@ -162,19 +162,22 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase // 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 @@ -212,16 +215,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, '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); - } - } - /** Toggles the checked state of the slide-toggle. */ toggle() { this.checked = !this.checked; @@ -243,15 +236,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase } } - /** 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); } - _onDragStart() { if (!this.disabled) { this._slideRenderer.startThumbDrag(this.checked);