From 32ff21c16bf1833d2da9f8e2ec8536f7a13f92de Mon Sep 17 00:00:00 2001
From: Kara Erickson <karakara@google.com>
Date: Mon, 14 Aug 2017 14:33:54 -0700
Subject: [PATCH] fix(forms): re-assigning options should not clear select

Fixes #18330
---
 .../select_control_value_accessor.ts          |  4 +-
 .../test/value_accessor_integration_spec.ts   | 58 ++++++++++++++++++-
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/packages/forms/src/directives/select_control_value_accessor.ts b/packages/forms/src/directives/select_control_value_accessor.ts
index 43e6322ae3..127470237b 100644
--- a/packages/forms/src/directives/select_control_value_accessor.ts
+++ b/packages/forms/src/directives/select_control_value_accessor.ts
@@ -129,8 +129,8 @@ export class SelectControlValueAccessor implements ControlValueAccessor {
 
   registerOnChange(fn: (value: any) => any): void {
     this.onChange = (valueString: string) => {
-      this.value = valueString;
-      fn(this._getOptionValue(valueString));
+      this.value = this._getOptionValue(valueString);
+      fn(this.value);
     };
   }
   registerOnTouched(fn: () => any): void { this.onTouched = fn; }
diff --git a/packages/forms/test/value_accessor_integration_spec.ts b/packages/forms/test/value_accessor_integration_spec.ts
index b708121882..11ad0271d6 100644
--- a/packages/forms/test/value_accessor_integration_spec.ts
+++ b/packages/forms/test/value_accessor_integration_spec.ts
@@ -8,7 +8,7 @@
 
 import {Component, Directive, EventEmitter, Input, Output, Type} from '@angular/core';
 import {ComponentFixture, TestBed, async, fakeAsync, tick} from '@angular/core/testing';
-import {AbstractControl, ControlValueAccessor, FormControl, FormGroup, FormsModule, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, NgForm, ReactiveFormsModule, Validators} from '@angular/forms';
+import {AbstractControl, ControlValueAccessor, FormControl, FormGroup, FormsModule, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, NgForm, NgModel, ReactiveFormsModule, Validators} from '@angular/forms';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util';
 
@@ -210,6 +210,31 @@ export function main() {
           expect(sfOption.nativeElement.selected).toBe(true);
         });
 
+        it('should support re-assigning the options array with compareWith', () => {
+          const fixture = initTest(FormControlSelectWithCompareFn);
+          fixture.detectChanges();
+
+          // Option IDs start out as 0 and 1, so setting the select value to "1: Object"
+          // will select the second option (NY).
+          const select = fixture.debugElement.query(By.css('select'));
+          select.nativeElement.value = '1: Object';
+          dispatchEvent(select.nativeElement, 'change');
+          fixture.detectChanges();
+
+          expect(fixture.componentInstance.form.value).toEqual({city: {id: 2, name: 'NY'}});
+
+          fixture.componentInstance.cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}];
+          fixture.detectChanges();
+
+          // Now that the options array has been re-assigned, new option instances will
+          // be created by ngFor. These instances will have different option IDs, subsequent
+          // to the first: 2 and 3. For the second option to stay selected, the select
+          // value will need to have the ID of the current second option: 3.
+          const nyOption = fixture.debugElement.queryAll(By.css('option'))[1];
+          expect(select.nativeElement.value).toEqual('3: Object');
+          expect(nyOption.nativeElement.selected).toBe(true);
+        });
+
       });
 
       describe('in template-driven forms', () => {
@@ -335,6 +360,37 @@ export function main() {
              expect(sfOption.nativeElement.selected).toBe(true);
            }));
 
+        it('should support re-assigning the options array with compareWith', fakeAsync(() => {
+             const fixture = initTest(NgModelSelectWithCustomCompareFnForm);
+             fixture.componentInstance.selectedCity = {id: 1, name: 'SF'};
+             fixture.componentInstance.cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}];
+             fixture.detectChanges();
+             tick();
+
+             // Option IDs start out as 0 and 1, so setting the select value to "1: Object"
+             // will select the second option (NY).
+             const select = fixture.debugElement.query(By.css('select'));
+             select.nativeElement.value = '1: Object';
+             dispatchEvent(select.nativeElement, 'change');
+             fixture.detectChanges();
+
+             const model = fixture.debugElement.children[0].injector.get(NgModel);
+             expect(model.value).toEqual({id: 2, name: 'NY'});
+
+             fixture.componentInstance.cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}];
+             fixture.detectChanges();
+             tick();
+
+             // Now that the options array has been re-assigned, new option instances will
+             // be created by ngFor. These instances will have different option IDs, subsequent
+             // to the first: 2 and 3. For the second option to stay selected, the select
+             // value will need to have the ID of the current second option: 3.
+             const nyOption = fixture.debugElement.queryAll(By.css('option'))[1];
+             expect(select.nativeElement.value).toEqual('3: Object');
+             expect(nyOption.nativeElement.selected).toBe(true);
+           }));
+
+
       });
 
     });