Skip to content

Commit

Permalink
fix(slide-toggle): invalid model change event (#4140)" (#4218)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
devversion authored and mmalerba committed Jul 6, 2017
1 parent ac3e21a commit a96cc0a
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 36 deletions.
25 changes: 14 additions & 11 deletions src/lib/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,23 @@ describe('MdSlideToggle', () => {
expect(slideToggleModel.pristine).toBe(true);
expect(slideToggleModel.touched).toBe(false);

// After changing the value programmatically, the control should
// become dirty (not pristine), but remain untouched.
// After changing the value from the view, the control should
// become dirty (not pristine), but remain untouched if focus is still there.
slideToggle.checked = true;

// Dispatch a change event on the input element to fake a user interaction that triggered
// the state change.
dispatchFakeEvent(inputElement, 'change');

fixture.detectChanges();

expect(slideToggleModel.valid).toBe(true);
expect(slideToggleModel.pristine).toBe(false);
expect(slideToggleModel.touched).toBe(false);

// After a user interaction occurs (such as a click), the control should remain dirty and
// now also be touched.
labelElement.click();
// Once the input element looses focus, the control should remain dirty but should
// also turn touched.
dispatchFakeEvent(inputElement, 'blur');
fixture.detectChanges();

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

// After a user interaction occurs (such as a click), the control should remain dirty and
// now also be touched.
inputElement.click();
// Once the input element looses focus, the control should remain dirty but should
// also turn touched.
dispatchFakeEvent(inputElement, 'blur');
fixture.detectChanges();

expect(slideToggleModel.touched).toBe(true);
expect(slideToggleElement.classList).not.toContain('mat-checked');
expect(slideToggleElement.classList).toContain('mat-checked');
});

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

it('should prevent the form from submit when being required', () => {

if ('reportValidity' in inputElement === false) {
// If the browser does not report the validity then the tests will break.
// e.g Safari 8 on Mobile.
return;
}

testComponent.isRequired = true;

fixture.detectChanges();

buttonElement.click();
Expand Down
45 changes: 20 additions & 25 deletions src/lib/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase

// A unique id for the slide-toggle. By default the id is auto-generated.
private _uniqueId = `md-slide-toggle-${++nextId}`;
private _checked: boolean = false;
private _slideRenderer: SlideToggleRenderer;
private _required: boolean = false;
private _disableRipple: boolean = false;
Expand All @@ -104,6 +103,9 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
/** Whether the label should appear after or before the slide-toggle. Defaults to 'after' */
@Input() labelPosition: 'before' | 'after' = 'after';

/** Whether the slide-toggle element is checked or not */
@Input() checked: boolean = false;

/** Used to set the aria-label attribute on the underlying input element. */
@Input('aria-label') ariaLabel: string | null = null;

Expand Down Expand Up @@ -153,29 +155,30 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
}

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

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

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

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

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

/** Whether the slide-toggle is checked. */
@Input()
get checked() { return !!this._checked; }
set checked(value) {
if (this.checked !== !!value) {
this._checked = value;
this.onChange(this._checked);
}
}

/** Toggles the checked state of the slide-toggle. */
toggle() {
this.checked = !this.checked;
Expand All @@ -244,15 +237,17 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
}
}

/** Emits the change event to the `change` output EventEmitter */
/**
* Emits a change event on the `change` output. Also notifies the FormControl about the change.
*/
private _emitChangeEvent() {
let event = new MdSlideToggleChange();
event.source = this;
event.checked = this.checked;
this.change.emit(event);
this.onChange(this.checked);
}


_onDragStart() {
if (!this.disabled) {
this._slideRenderer.startThumbDrag(this.checked);
Expand Down

0 comments on commit a96cc0a

Please sign in to comment.