Skip to content

Commit f93649c

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 bfdef8e commit f93649c

File tree

2 files changed

+34
-35
lines changed

2 files changed

+34
-35
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(slideToggleControl.pristine).toBe(true);
296296
expect(slideToggleControl.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(slideToggleControl.valid).toBe(true);
304309
expect(slideToggleControl.pristine).toBe(false);
305310
expect(slideToggleControl.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(slideToggleControl.valid).toBe(true);
@@ -324,13 +329,13 @@ describe('MdSlideToggle', () => {
324329
expect(slideToggleControl.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(slideToggleControl.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-24
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA
6464

6565
// A unique id for the slide-toggle. By default the id is auto-generated.
6666
private _uniqueId = `md-slide-toggle-${++nextId}`;
67-
private _checked: boolean = false;
6867
private _color: string;
6968
private _slideRenderer: SlideToggleRenderer = null;
7069
private _disabled: boolean = false;
@@ -86,6 +85,9 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA
8685
/** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */
8786
@Input() labelPosition: 'before' | 'after' = 'after';
8887

88+
/** Whether the slide-toggle element is checked or not */
89+
@Input() checked: boolean = false;
90+
8991
/** Used to set the aria-label attribute on the underlying input element. */
9092
@Input('aria-label') ariaLabel: string = null;
9193

@@ -136,29 +138,30 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA
136138
}
137139

138140
/**
139-
* The onChangeEvent method will be also called on click.
140-
* This is because everything for the slide-toggle is wrapped inside of a label,
141-
* which triggers a onChange event on click.
141+
* This function will called if the underlying input changed its value through user interaction.
142142
*/
143143
_onChangeEvent(event: Event) {
144144
// We always have to stop propagation on the change event.
145145
// Otherwise the change event, from the input element, will bubble up and
146146
// emit its event object to the component's `change` output.
147147
event.stopPropagation();
148148

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

153-
// Emit our custom change event if the native input emitted one.
154-
// It is important to only emit it, if the native input triggered one, because
155-
// we don't want to trigger a change event, when the `checked` variable changes for example.
156-
this._emitChangeEvent();
157-
}
152+
// Emit our custom change event if the native input emitted one.
153+
// It is important to only emit it, if the native input triggered one, because we don't want
154+
// to trigger a change event, when the `checked` variable changes programmatically.
155+
this._emitChangeEvent();
158156
}
159157

160158
_onInputClick(event: Event) {
161-
this.onTouched();
159+
// In some situations the user will release the mouse on the label element. The label element
160+
// redirects the click to the underlying input element and will result in a value change.
161+
// Prevent the default behavior if dragging, because the value will be set after drag.
162+
if (this._slideRenderer.dragging) {
163+
event.preventDefault();
164+
}
162165

163166
// We have to stop propagation for click events on the visual hidden input element.
164167
// By default, when a user clicks on a label element, a generated click event will be
@@ -195,16 +198,6 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA
195198
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard');
196199
}
197200

198-
/** Whether the slide-toggle is checked. */
199-
@Input()
200-
get checked() { return !!this._checked; }
201-
set checked(value) {
202-
if (this.checked !== !!value) {
203-
this._checked = value;
204-
this.onChange(this._checked);
205-
}
206-
}
207-
208201
/** The color of the slide-toggle. Can be primary, accent, or warn. */
209202
@Input()
210203
get color(): string { return this._color; }
@@ -245,12 +238,15 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA
245238
}
246239
}
247240

248-
/** Emits the change event to the `change` output EventEmitter */
241+
/**
242+
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
243+
*/
249244
private _emitChangeEvent() {
250245
let event = new MdSlideToggleChange();
251246
event.source = this;
252247
event.checked = this.checked;
253248
this.change.emit(event);
249+
this.onChange(this.checked);
254250
}
255251

256252

0 commit comments

Comments
 (0)