Skip to content

Commit 91e0ee2

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 90e6d3c commit 91e0ee2

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
@@ -75,7 +75,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
7575

7676
// A unique id for the slide-toggle. By default the id is auto-generated.
7777
private _uniqueId = `md-slide-toggle-${++nextId}`;
78-
private _checked: boolean = false;
7978
private _slideRenderer: SlideToggleRenderer = null;
8079
private _required: boolean = false;
8180
private _disableRipple: boolean = false;
@@ -95,6 +94,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
9594
/** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */
9695
@Input() labelPosition: 'before' | 'after' = 'after';
9796

97+
/** Whether the slide-toggle element is checked or not */
98+
@Input() checked: boolean = false;
99+
98100
/** Used to set the aria-label attribute on the underlying input element. */
99101
@Input('aria-label') ariaLabel: string = null;
100102

@@ -144,29 +146,30 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
144146
}
145147

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

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

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

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

171174
// We have to stop propagation for click events on the visual hidden input element.
172175
// By default, when a user clicks on a label element, a generated click event will be
@@ -204,16 +207,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
204207
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, 'keyboard');
205208
}
206209

207-
/** Whether the slide-toggle is checked. */
208-
@Input()
209-
get checked() { return !!this._checked; }
210-
set checked(value) {
211-
if (this.checked !== !!value) {
212-
this._checked = value;
213-
this.onChange(this._checked);
214-
}
215-
}
216-
217210
/** Toggles the checked state of the slide-toggle. */
218211
toggle() {
219212
this.checked = !this.checked;
@@ -235,15 +228,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
235228
}
236229
}
237230

238-
/** Emits the change event to the `change` output EventEmitter */
231+
/**
232+
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
233+
*/
239234
private _emitChangeEvent() {
240235
let event = new MdSlideToggleChange();
241236
event.source = this;
242237
event.checked = this.checked;
243238
this.change.emit(event);
239+
this.onChange(this.checked);
244240
}
245241

246-
247242
_onDragStart() {
248243
if (!this.disabled) {
249244
this._slideRenderer.startThumbDrag(this.checked);

0 commit comments

Comments
 (0)