From 547bfa92efa0f2cbd95de134de49619e24cc665b Mon Sep 17 00:00:00 2001 From: gary-b Date: Wed, 14 Dec 2016 16:34:19 +0000 Subject: [PATCH] fix (forms): clear selected options when model is not an array (#12519) When an invalid model value (eg empty string) was preset ngModel on select[multiple] would throw an error, which is inconsistent with how it works on other user input elements. Setting the model value to null or undefined would also have no effect on what was already selected in the UI. Fix this by clearing selected options when model set to null, undefined or a type other than Array. Closes #11926 --- .../select_multiple_control_value_accessor.ts | 14 ++-- .../forms/test/template_integration_spec.ts | 76 ++++++++++++++++++- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts b/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts index cd7302f72e..9fe3027615 100644 --- a/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts +++ b/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts @@ -66,11 +66,15 @@ export class SelectMultipleControlValueAccessor implements ControlValueAccessor writeValue(value: any): void { this.value = value; - if (value == null) return; - const values: Array = >value; - // convert values to ids - const ids = values.map((v) => this._getOptionId(v)); - this._optionMap.forEach((opt, o) => { opt._setSelected(ids.indexOf(o.toString()) > -1); }); + let optionSelectedStateSetter: (opt: NgSelectMultipleOption, o: any) => void; + if (Array.isArray(value)) { + // convert values to ids + const ids = value.map((v) => this._getOptionId(v)); + optionSelectedStateSetter = (opt, o) => { opt._setSelected(ids.indexOf(o.toString()) > -1); }; + } else { + optionSelectedStateSetter = (opt, o) => { opt._setSelected(false); }; + } + this._optionMap.forEach(optionSelectedStateSetter); } registerOnChange(fn: (value: any) => any): void { diff --git a/modules/@angular/forms/test/template_integration_spec.ts b/modules/@angular/forms/test/template_integration_spec.ts index 48d074f91c..98853e7a9a 100644 --- a/modules/@angular/forms/test/template_integration_spec.ts +++ b/modules/@angular/forms/test/template_integration_spec.ts @@ -7,7 +7,7 @@ */ import {Component, Directive, Input, forwardRef} from '@angular/core'; -import {TestBed, async, fakeAsync, tick} from '@angular/core/testing'; +import {ComponentFixture, TestBed, async, fakeAsync, tick} from '@angular/core/testing'; import {AbstractControl, ControlValueAccessor, FormsModule, NG_ASYNC_VALIDATORS, NG_VALUE_ACCESSOR, NgForm, Validator} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; @@ -23,7 +23,7 @@ export function main() { NgModelRadioForm, NgModelRangeForm, NgModelSelectForm, NgNoFormComp, InvalidNgModelNoName, NgModelOptionsStandalone, NgModelCustomComp, NgModelCustomWrapper, NgModelValidationBindings, NgModelMultipleValidators, NgAsyncValidator, - NgModelAsyncValidation, NgModelSelectWithNullForm + NgModelAsyncValidation, NgModelSelectMultipleForm, NgModelSelectWithNullForm ], imports: [FormsModule] }); @@ -723,6 +723,65 @@ export function main() { })); }); + describe('select multiple controls', () => { + let fixture: ComponentFixture; + let comp: NgModelSelectMultipleForm; + + beforeEach(() => { + fixture = TestBed.createComponent(NgModelSelectMultipleForm); + comp = fixture.componentInstance; + comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}]; + }); + + const setSelectedCities = (selectedCities: any): void => { + comp.selectedCities = selectedCities; + fixture.detectChanges(); + tick(); + }; + + const assertOptionElementSelectedState = (selectedStates: boolean[]): void => { + const options = fixture.debugElement.queryAll(By.css('option')); + if (options.length !== selectedStates.length) { + throw 'the selected state values to assert does not match the number of options'; + } + for (let i = 0; i < selectedStates.length; i++) { + expect(options[i].nativeElement.selected).toBe(selectedStates[i]); + } + }; + + const testNewModelValueUnselectsAllOptions = (modelValue: any): void => { + setSelectedCities([comp.cities[1]]); + assertOptionElementSelectedState([false, true, false]); + + setSelectedCities(modelValue); + + const select = fixture.debugElement.query(By.css('select')); + expect(select.nativeElement.value).toEqual(''); + assertOptionElementSelectedState([false, false, false]); + }; + + it('should support setting value to null and undefined', fakeAsync(() => { + testNewModelValueUnselectsAllOptions(null); + testNewModelValueUnselectsAllOptions(undefined); + })); + + it('should clear all selected option elements when value of wrong type supplied', + fakeAsync(() => { testNewModelValueUnselectsAllOptions(''); })); + + it('should set option elements to selected that are present in model', fakeAsync(() => { + setSelectedCities([comp.cities[0], comp.cities[2]]); + assertOptionElementSelectedState([true, false, true]); + })); + + it('should clear selected option elements since removed from model', fakeAsync(() => { + const selectedCities: [{'name:string': string}] = <[{'name:string': string}]>[]; + selectedCities.push.apply(selectedCities, comp.cities); + setSelectedCities(selectedCities); + setSelectedCities([comp.cities[1]]); + assertOptionElementSelectedState([false, true, false]); + })); + }); + describe('custom value accessors', () => { it('should support standard writing to view and model', async(() => { const fixture = TestBed.createComponent(NgModelCustomWrapper); @@ -1136,6 +1195,19 @@ class NgModelSelectWithNullForm { cities: any[] = []; } +@Component({ + selector: 'ng-model-select-multiple-form', + template: ` + + ` +}) +class NgModelSelectMultipleForm { + selectedCities: any[]; + cities: any[] = []; +} + @Component({ selector: 'ng-model-custom-comp', template: `