Skip to content

Commit 6f1fd1f

Browse files
devversionandrewseguin
authored andcommitted
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.
1 parent 3bfe7f0 commit 6f1fd1f

File tree

2 files changed

+33
-35
lines changed

2 files changed

+33
-35
lines changed

src/lib/slide-toggle/slide-toggle.spec.ts

+13-10
Original file line numberDiff line numberDiff line change
@@ -547,18 +547,21 @@ describe('MdSlideToggle with forms', () => {
547547
expect(slideToggleModel.pristine).toBe(true);
548548
expect(slideToggleModel.touched).toBe(false);
549549

550-
// After changing the value programmatically, the control should
551-
// become dirty (not pristine), but remain untouched.
550+
// After changing the value from the view, the control should
551+
// become dirty (not pristine), but remain untouched if focus is still there.
552552
slideToggle.checked = true;
553-
fixture.detectChanges();
553+
554+
// Dispatch a change event on the input element to fake a user interaction that triggered
555+
// the state change.
556+
dispatchFakeEvent(inputElement, 'change');
554557

555558
expect(slideToggleModel.valid).toBe(true);
556559
expect(slideToggleModel.pristine).toBe(false);
557560
expect(slideToggleModel.touched).toBe(false);
558561

559-
// After a user interaction occurs (such as a click), the control should remain dirty and
560-
// now also be touched.
561-
labelElement.click();
562+
// Once the input element loses focus, the control should remain dirty but should
563+
// also turn touched.
564+
dispatchFakeEvent(inputElement, 'blur');
562565
fixture.detectChanges();
563566

564567
expect(slideToggleModel.valid).toBe(true);
@@ -576,13 +579,13 @@ describe('MdSlideToggle with forms', () => {
576579
expect(slideToggleModel.touched).toBe(false);
577580
expect(slideToggleElement.classList).toContain('mat-checked');
578581

579-
// After a user interaction occurs (such as a click), the control should remain dirty and
580-
// now also be touched.
581-
inputElement.click();
582+
// Once the input element loses focus, the control should remain dirty but should
583+
// also turn touched.
584+
dispatchFakeEvent(inputElement, 'blur');
582585
fixture.detectChanges();
583586

584587
expect(slideToggleModel.touched).toBe(true);
585-
expect(slideToggleElement.classList).not.toContain('mat-checked');
588+
expect(slideToggleElement.classList).toContain('mat-checked');
586589
});
587590

588591
it('should not set the control to touched when changing the model', fakeAsync(() => {

src/lib/slide-toggle/slide-toggle.ts

+20-25
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase implements OnDestroy,
8484
private onTouched = () => {};
8585

8686
private _uniqueId: string = `md-slide-toggle-${++nextUniqueId}`;
87-
private _checked: boolean = false;
8887
private _slideRenderer: SlideToggleRenderer;
8988
private _required: boolean = false;
9089

@@ -103,6 +102,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase implements OnDestroy,
103102
/** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */
104103
@Input() labelPosition: 'before' | 'after' = 'after';
105104

105+
/** Whether the slide-toggle element is checked or not */
106+
@Input() checked: boolean = false;
107+
106108
/** Used to set the aria-label attribute on the underlying input element. */
107109
@Input('aria-label') ariaLabel: string | null = null;
108110

@@ -147,29 +149,30 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase implements OnDestroy,
147149
}
148150

149151
/**
150-
* The onChangeEvent method will be also called on click.
151-
* This is because everything for the slide-toggle is wrapped inside of a label,
152-
* which triggers a onChange event on click.
152+
* This function will called if the underlying input changed its value through user interaction.
153153
*/
154154
_onChangeEvent(event: Event) {
155155
// We always have to stop propagation on the change event.
156156
// Otherwise the change event, from the input element, will bubble up and
157157
// emit its event object to the component's `change` output.
158158
event.stopPropagation();
159159

160-
// Once a drag is currently in progress, we do not want to toggle the slide-toggle on a click.
161-
if (!this.disabled && !this._slideRenderer.dragging) {
162-
this.toggle();
160+
// Sync the value from the underlying input element with the slide-toggle component.
161+
this.checked = this._inputElement.nativeElement.checked;
163162

164-
// Emit our custom change event if the native input emitted one.
165-
// It is important to only emit it, if the native input triggered one, because
166-
// we don't want to trigger a change event, when the `checked` variable changes for example.
167-
this._emitChangeEvent();
168-
}
163+
// Emit our custom change event if the native input emitted one.
164+
// It is important to only emit it, if the native input triggered one, because we don't want
165+
// to trigger a change event, when the `checked` variable changes programmatically.
166+
this._emitChangeEvent();
169167
}
170168

171169
_onInputClick(event: Event) {
172-
this.onTouched();
170+
// In some situations the user will release the mouse on the label element. The label element
171+
// redirects the click to the underlying input element and will result in a value change.
172+
// Prevent the default behavior if dragging, because the value will be set after drag.
173+
if (this._slideRenderer.dragging) {
174+
event.preventDefault();
175+
}
173176

174177
// We have to stop propagation for click events on the visual hidden input element.
175178
// By default, when a user clicks on a label element, a generated click event will be
@@ -207,16 +210,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase implements OnDestroy,
207210
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, 'keyboard');
208211
}
209212

210-
/** Whether the slide-toggle is checked. */
211-
@Input()
212-
get checked() { return !!this._checked; }
213-
set checked(value) {
214-
if (this.checked !== !!value) {
215-
this._checked = value;
216-
this.onChange(this._checked);
217-
}
218-
}
219-
220213
/** Toggles the checked state of the slide-toggle. */
221214
toggle() {
222215
this.checked = !this.checked;
@@ -238,15 +231,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase implements OnDestroy,
238231
}
239232
}
240233

241-
/** Emits the change event to the `change` output EventEmitter */
234+
/**
235+
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
236+
*/
242237
private _emitChangeEvent() {
243238
let event = new MdSlideToggleChange();
244239
event.source = this;
245240
event.checked = this.checked;
246241
this.change.emit(event);
242+
this.onChange(this.checked);
247243
}
248244

249-
250245
_onDragStart() {
251246
if (!this.disabled) {
252247
this._slideRenderer.startThumbDrag(this.checked);

0 commit comments

Comments
 (0)