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

gary-b · 2016-11-01T23:14:01Z

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?

[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?
Closes #12527. The original repro had quite a lot going on, here is a more distilled version Select a few options, then push or pop from the options list.

If you bound an array to select[multiple] via ngModel and subsequently changed the options to select from, the UI would drop any selections that the user had made since.

After options change SelectMultipleControlValueAccessor will read the model again and set the state of the options. The problem was that it didn't keep a reference to the latest model.

What is the new behavior?
SelectMultipleControlValueAccessor maintains an up to date reference to the model. You can now add and remove items from the options and currently selected items will remain selected. Items in the model array that didn't have matching options in the select list will also cause their corresponding options to become selected once added.

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

Yes
No

Whoever looks at this could look at #12519 again too. Ill need to do a little bit of refactoring to the tests when the last one is committed, if they both are accepted that is :P

kara

Thanks for the PR! Looks fine, but requesting minor changes.

kara · 2016-11-10T22:08:53Z

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

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


The @input decorator is unnecessary - can you remove?

kara · 2016-11-10T22:12:04Z

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

+           comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}];
+           comp.selectedCities = [];
+           detectChangesAndTick();
+           const select = fixture.debugElement.queryAll(By.css('select'))[0];


queryAll doesn't seem necessary here, given that there is only one select element in the test component. Can you switch to query? Then you won't have to look for [0].

kara · 2016-11-10T22:14:06Z

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

+           const select = fixture.debugElement.queryAll(By.css('select'))[0];
+           select.nativeElement.value = '1: Object';
+           dispatchEvent(select.nativeElement, 'change');
+           detectChangesAndTick();


After this change detection run, can you check that the options are selected as you expect? Will make the intent of the test clearer.

kara · 2016-11-10T22:16:02Z

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

+           detectChangesAndTick();
+           assertOptionElementSelectedState([false, true, false, false]);
+         }));
+    });


Can you add a test for the 'pop' case?

kara · 2016-11-10T22:18:59Z

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

+           const comp = fixture.componentInstance;
+           comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}];
+           comp.selectedCities = [];
+           detectChangesAndTick();


The test is a bit harder to read because there are no line breaks. Can you add a line break after each logical section? So perhaps here, after the selection is made, and after the new value is pushed?

vicb · 2016-12-09T23:24:05Z

@gary-b do you have plan to update this PR ?

gary-b · 2016-12-11T14:14:36Z

Yes, ill work on this and #12519 tomorrow

If you bound an array to select[multiple] via ngModel and subsequently changed the options to select from, the UI would drop any selections made since by the user. This was due to SelectMultipleControlValueAccessor not keeping a reference to the new model arrays it generated when users interacted with the select control. Update code to keep the reference. Closes angular#12527

gary-b · 2016-12-12T20:38:09Z

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

+      let fixture: ComponentFixture<NgModelSelectMultipleForm>;
+      let comp: NgModelSelectMultipleForm;
+
+      beforeEach(() => {


Used by other tests in #12519

gary-b · 2016-12-12T20:38:20Z

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

+      };
+
+      const setSelectedCities = (selectedCities: any): void => {
+        comp.selectedCities = selectedCities;


Used by other tests in #12519

gary-b · 2016-12-12T20:49:40Z

As before, I may need to rebase this again after #12519 is merged as the tests for both introduce the same describe(). They also use the same helper functions.

kara

LGTM

If you bound an array to select[multiple] via ngModel and subsequently changed the options to select from, the UI would drop any selections made since by the user. This was due to SelectMultipleControlValueAccessor not keeping a reference to the new model arrays it generated when users interacted with the select control. Update code to keep the reference. Closes #12527 Closes #12654

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

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 Nov 1, 2016

vicb added the area: forms label Nov 2, 2016

kara requested changes Nov 10, 2016

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 10, 2016

gary-b force-pushed the select-multiple-looses-selection branch from c638a5f to af4328f Compare December 12, 2016 20:36

gary-b commented Dec 12, 2016

View reviewed changes

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 closed this in 821b8f0 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): ensure select[multiple] retains selections #12654

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

gary-b commented Nov 1, 2016 •

edited

Loading

kara left a comment

kara Nov 10, 2016

kara Nov 10, 2016

kara Nov 10, 2016

kara Nov 10, 2016

kara Nov 10, 2016

vicb commented Dec 9, 2016

gary-b commented Dec 11, 2016

gary-b Dec 12, 2016

gary-b Dec 12, 2016

gary-b commented Dec 12, 2016

kara left a comment

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

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

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

Conversation

gary-b commented Nov 1, 2016 • edited Loading

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

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

vicb commented Dec 9, 2016

gary-b commented Dec 11, 2016

gary-b Dec 12, 2016

Choose a reason for hiding this comment

gary-b Dec 12, 2016

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 Nov 1, 2016 •

edited

Loading