Skip to content

Commit bbd36d5

Browse files
committed
fix(slide-toggle): invalid model change event (angular#4140)" (angular#4218)
While initially looking into angular#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 angular#4124.
1 parent c20bec8 commit bbd36d5

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
@@ -81,7 +81,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
8181
private onTouched = () => {};
8282

8383
private _uniqueId: string = `md-slide-toggle-${++nextUniqueId}`;
84-
private _checked: boolean = false;
8584
private _slideRenderer: SlideToggleRenderer;
8685
private _required: boolean = false;
8786
private _disableRipple: boolean = false;
@@ -101,6 +100,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
101100
/** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */
102101
@Input() labelPosition: 'before' | 'after' = 'after';
103102

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

@@ -150,29 +152,30 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
150152
}
151153

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

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

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

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

177180
// We have to stop propagation for click events on the visual hidden input element.
178181
// By default, when a user clicks on a label element, a generated click event will be
@@ -210,16 +213,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
210213
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, 'keyboard');
211214
}
212215

213-
/** Whether the slide-toggle is checked. */
214-
@Input()
215-
get checked() { return !!this._checked; }
216-
set checked(value) {
217-
if (this.checked !== !!value) {
218-
this._checked = value;
219-
this.onChange(this._checked);
220-
}
221-
}
222-
223216
/** Toggles the checked state of the slide-toggle. */
224217
toggle() {
225218
this.checked = !this.checked;
@@ -241,15 +234,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
241234
}
242235
}
243236

244-
/** Emits the change event to the `change` output EventEmitter */
237+
/**
238+
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
239+
*/
245240
private _emitChangeEvent() {
246241
let event = new MdSlideToggleChange();
247242
event.source = this;
248243
event.checked = this.checked;
249244
this.change.emit(event);
245+
this.onChange(this.checked);
250246
}
251247

252-
253248
_onDragStart() {
254249
if (!this.disabled) {
255250
this._slideRenderer.startThumbDrag(this.checked);

0 commit comments

Comments
 (0)