Skip to content

Commit e74f20c

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 154f54e commit e74f20c

File tree

2 files changed

+34
-36
lines changed

2 files changed

+34
-36
lines changed

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

+14-11
Original file line numberDiff line numberDiff line change
@@ -295,18 +295,23 @@ describe('MdSlideToggle', () => {
295295
expect(slideToggleModel.pristine).toBe(true);
296296
expect(slideToggleModel.touched).toBe(false);
297297

298-
// After changing the value programmatically, the control should
299-
// become dirty (not pristine), but remain untouched.
298+
// After changing the value from the view, the control should
299+
// become dirty (not pristine), but remain untouched if focus is still there.
300300
slideToggle.checked = true;
301+
302+
// Dispatch a change event on the input element to fake a user interaction that triggered
303+
// the state change.
304+
dispatchFakeEvent(inputElement, 'change');
305+
301306
fixture.detectChanges();
302307

303308
expect(slideToggleModel.valid).toBe(true);
304309
expect(slideToggleModel.pristine).toBe(false);
305310
expect(slideToggleModel.touched).toBe(false);
306311

307-
// After a user interaction occurs (such as a click), the control should remain dirty and
308-
// now also be touched.
309-
labelElement.click();
312+
// Once the input element looses focus, the control should remain dirty but should
313+
// also turn touched.
314+
dispatchFakeEvent(inputElement, 'blur');
310315
fixture.detectChanges();
311316

312317
expect(slideToggleModel.valid).toBe(true);
@@ -324,13 +329,13 @@ describe('MdSlideToggle', () => {
324329
expect(slideToggleModel.touched).toBe(false);
325330
expect(slideToggleElement.classList).toContain('mat-checked');
326331

327-
// After a user interaction occurs (such as a click), the control should remain dirty and
328-
// now also be touched.
329-
inputElement.click();
332+
// Once the input element looses focus, the control should remain dirty but should
333+
// also turn touched.
334+
dispatchFakeEvent(inputElement, 'blur');
330335
fixture.detectChanges();
331336

332337
expect(slideToggleModel.touched).toBe(true);
333-
expect(slideToggleElement.classList).not.toContain('mat-checked');
338+
expect(slideToggleElement.classList).toContain('mat-checked');
334339
});
335340

336341
// TODO(kara): update when core/testing adds fix
@@ -434,15 +439,13 @@ describe('MdSlideToggle', () => {
434439
}));
435440

436441
it('should prevent the form from submit when being required', () => {
437-
438442
if ('reportValidity' in inputElement === false) {
439443
// If the browser does not report the validity then the tests will break.
440444
// e.g Safari 8 on Mobile.
441445
return;
442446
}
443447

444448
testComponent.isRequired = true;
445-
446449
fixture.detectChanges();
447450

448451
buttonElement.click();

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

+20-25
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
8383

8484
// A unique id for the slide-toggle. By default the id is auto-generated.
8585
private _uniqueId = `md-slide-toggle-${++nextId}`;
86-
private _checked: boolean = false;
8786
private _slideRenderer: SlideToggleRenderer = null;
8887
private _required: boolean = false;
8988
private _disableRipple: boolean = false;
@@ -103,6 +102,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
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;
108110

@@ -152,29 +154,30 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
152154
}
153155

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

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

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

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

179182
// We have to stop propagation for click events on the visual hidden input element.
180183
// 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
212215
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, 'keyboard');
213216
}
214217

215-
/** Whether the slide-toggle is checked. */
216-
@Input()
217-
get checked() { return !!this._checked; }
218-
set checked(value) {
219-
if (this.checked !== !!value) {
220-
this._checked = value;
221-
this.onChange(this._checked);
222-
}
223-
}
224-
225218
/** Toggles the checked state of the slide-toggle. */
226219
toggle() {
227220
this.checked = !this.checked;
@@ -243,15 +236,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
243236
}
244237
}
245238

246-
/** Emits the change event to the `change` output EventEmitter */
239+
/**
240+
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
241+
*/
247242
private _emitChangeEvent() {
248243
let event = new MdSlideToggleChange();
249244
event.source = this;
250245
event.checked = this.checked;
251246
this.change.emit(event);
247+
this.onChange(this.checked);
252248
}
253249

254-
255250
_onDragStart() {
256251
if (!this.disabled) {
257252
this._slideRenderer.startThumbDrag(this.checked);

0 commit comments

Comments
 (0)