fix (forms): clear selected options when model is not an array #12519

gary-b · 2016-10-25T16:09:55Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

For a select[multiple] with NgModel, the expected model type is an Array. Currently if you set it to a different type of object (ignoring null/undefined for the moment), you get an error. NgModel does not throw such an error when you use an invalid model value with other input controls, eg input[type=number].
The repro in Issue SelectMultipleControlValueAccessor is a string and fails with values.map does not exist #11926 includes the use of ngModel on a select element, where ngModel is not explicitly bound. This result in an initial model value for the select of an empty string. (I'm going to raise a separate issue about this ngModel behavior, which I suspect is related to one time string initialization.)
When you set the model value to null or undefined, the currently selected option elements stay selected. NgModel used with other input controls clears the selection when these values are passed in.

What is the new behavior?

If a select[multiple] is bound to an invalid value, the currently selected option elements are cleared and no error is raised.
If you set the model to null or undefined, the currently selected option elements are cleared.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No - (Well, changes what happens when the model is set to null / undefined - but I doubt anyone desires the old behavior.)

gary-b · 2016-10-25T18:28:32Z

I fixed a lint error in my code. 2 of the travis-ci build steps are still failing though:
Fail 3 looks like infrastructure problems.
Fail 4 relates to a failing test in a completely different area.

vicb · 2016-10-26T01:55:17Z

modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts

-    this._optionMap.forEach((opt, o) => { opt._setSelected(ids.indexOf(o.toString()) > -1); });
+    let optionSelectedStateSetter: (opt: NgSelectMultipleOption, o: any) => void;
+    if (Array.isArray(value)) {
+      let values: Array<any> = <Array<any>>value;


const values: any[] = <any[]>values;

but using value directly should also work because of Array.isArray(value)

vicb · 2016-10-26T01:56:15Z

modules/@angular/forms/test/template_integration_spec.ts

@@ -701,6 +701,66 @@ export function main() {
         }));
    });

+    describe('select multiple controls', () => {
+      var fixture: ComponentFixture<NgModelSelectMultipleForm>;


var -> let (or const where applicable)

kara

Minor comments, but looks good

kara · 2016-11-10T21:41:59Z

modules/@angular/forms/test/template_integration_spec.ts

+  `
+})
+class NgModelSelectMultipleForm {
+  @Input() selectedCities: any[];


The @input decorator here isn't necessary - can you remove?

kara · 2016-11-10T21:43:51Z

modules/@angular/forms/test/template_integration_spec.ts

+        comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}];
+      });
+
+      const setSelectedCitiesDetectChangesAndTick = (selectedCities: any): void => {


Can you rename to setSelectedCities? The name is a bit of a handful to read.

kara · 2016-11-10T21:47:15Z

modules/@angular/forms/test/template_integration_spec.ts

+      const assertOptionElementSelectedState = (selectedStates: boolean[]): void => {
+        const options = fixture.debugElement.queryAll(By.css('option'));
+        if (options.length !== selectedStates.length) {
+          throw `there are ${options.length} option elements but ${selectedStates.length}` +


I'd wrap the whole error message in backticks so you don't have to concat the string. Or if you're trying to avoid the line break, I'd just make it shorter.

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 angular#11926

gary-b · 2016-12-12T20:52:18Z

PR updated.

kara

LGTM

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

angular-automatic-lock-bot · 2019-09-10T11:21:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Oct 25, 2016

gary-b force-pushed the first-typescript-bug branch from 8eae1a4 to 6f05c93 Compare October 25, 2016 17:12

vicb added the area: forms label Oct 26, 2016

vicb reviewed Oct 26, 2016

View reviewed changes

vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 26, 2016

gary-b force-pushed the first-typescript-bug branch from 6f05c93 to 0cab245 Compare October 26, 2016 16:19

gary-b mentioned this pull request Nov 1, 2016

fix(forms): ensure select[multiple] retains selections #12654

Closed

5 tasks

kara reviewed Nov 10, 2016

View reviewed changes

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 10, 2016

gary-b force-pushed the first-typescript-bug branch from 0cab245 to e17bf59 Compare December 12, 2016 15:00

gary-b force-pushed the first-typescript-bug branch from e17bf59 to a24ae95 Compare December 12, 2016 18:41

kara approved these changes Dec 14, 2016

View reviewed changes

kara added action: merge The PR is ready for merge by the caretaker pr_state: LGTM and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 14, 2016

vicb merged commit 7b0a867 into angular:master Dec 14, 2016

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix (forms): clear selected options when model is not an array #12519

fix (forms): clear selected options when model is not an array #12519

gary-b commented Oct 25, 2016 •

edited

gary-b commented Oct 25, 2016

vicb Oct 26, 2016

vicb Oct 26, 2016

kara left a comment

kara Nov 10, 2016

kara Nov 10, 2016

kara Nov 10, 2016 •

edited

gary-b commented Dec 12, 2016

kara left a comment

angular-automatic-lock-bot bot commented Sep 10, 2019

fix (forms): clear selected options when model is not an array #12519

fix (forms): clear selected options when model is not an array #12519

Conversation

gary-b commented Oct 25, 2016 • edited

gary-b commented Oct 25, 2016

vicb Oct 26, 2016

Choose a reason for hiding this comment

vicb Oct 26, 2016

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

kara Nov 10, 2016

Choose a reason for hiding this comment

kara Nov 10, 2016

Choose a reason for hiding this comment

kara Nov 10, 2016 • edited

Choose a reason for hiding this comment

gary-b commented Dec 12, 2016

kara left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 10, 2019

gary-b commented Oct 25, 2016 •

edited

kara Nov 10, 2016 •

edited