fix: ensure the value attribute of option elements is properly set via [value] bindings when there is no associated form view adapter

Author: MrWolfZ

CHANGELOG.md

@@ -1,5 +1,12 @@
 ## ngrx-forms Changelog
 
+<a name="2.3.3"></a>
+### 2.3.3
+
+#### Bugfixes
+
+* ensure the `value` attribute of `option` elements is properly set via `[value]` bindings when there is no associated form view adapter (closes [#67](https://github.com/MrWolfZ/ngrx-forms/issues/67), thanks @kmiskiewicz for helping me find this)
+
 <a name="2.3.2"></a>
 ### 2.3.2
 

src/control/e2e.spec/select.spec.ts

@@ -23,6 +23,15 @@ export class SelectComponent {
   options = SELECT_OPTIONS;
 }
 
+@Component({
+  // tslint:disable-next-line:component-selector
+  selector: 'select-test-fallback',
+  template: '<select><option *ngFor="let o of options" [value]="o">{{o}} Label</option></select>',
+})
+export class SelectFallbackComponent {
+  options = SELECT_OPTIONS;
+}
+
 describe(SelectComponent.name, () => {
   let component: SelectComponent;
   let fixture: ComponentFixture<SelectComponent>;
@@ -43,7 +52,7 @@ describe(SelectComponent.name, () => {
   beforeEach(async(() => {
     TestBed.configureTestingModule({
       imports: [NgrxFormsModule],
-      declarations: [SelectComponent],
+      declarations: [SelectComponent, SelectFallbackComponent],
       providers: [{ provide: ActionsSubject, useValue: actionsSubject }],
     }).compileComponents();
   }));
@@ -83,6 +92,17 @@ describe(SelectComponent.name, () => {
     element.selectedIndex = 0;
     element.dispatchEvent(new Event('change'));
   });
+
+  it('should set the value attribute for options without associated form state', () => {
+    const fallbackFixture = TestBed.createComponent(SelectFallbackComponent);
+    fallbackFixture.detectChanges();
+    const nativeElement = fallbackFixture.nativeElement as HTMLElement;
+    element = nativeElement.querySelector('select') as HTMLSelectElement;
+    option1 = nativeElement.querySelectorAll('option')[0] as HTMLOptionElement;
+    option2 = nativeElement.querySelectorAll('option')[1] as HTMLOptionElement;
+    expect(option1.value).toBe(SELECT_OPTIONS[0]);
+    expect(option2.value).toBe(SELECT_OPTIONS[1]);
+  });
 });
 
 const SELECT_NUMBER_OPTIONS = [1, 2];

src/module.ts

@@ -5,6 +5,7 @@ import { NgrxFormDirective } from './group/directive';
 import { NgrxCheckboxViewAdapter } from './view-adapter/checkbox';
 import { NgrxDefaultViewAdapter } from './view-adapter/default';
 import { NgrxNumberViewAdapter } from './view-adapter/number';
+import { NgrxFallbackSelectOption } from './view-adapter/option';
 import { NgrxRadioViewAdapter } from './view-adapter/radio';
 import { NgrxRangeViewAdapter } from './view-adapter/range';
 import { NgrxSelectOption, NgrxSelectViewAdapter } from './view-adapter/select';
@@ -23,6 +24,7 @@ const exportsAndDeclarations = [
   NgrxSelectMultipleViewAdapter,
   NgrxSelectOption,
   NgrxSelectViewAdapter,
+  NgrxFallbackSelectOption,
   NgrxStatusCssClassesDirective,
 ];
 

src/ngrx-forms.spec.ts

@@ -32,6 +32,7 @@ import {
   NgrxSelectMultipleViewAdapter,
   NgrxSelectOption,
   NgrxSelectViewAdapter,
+  NgrxFallbackSelectOption,
   NgrxStatusCssClassesDirective,
   NgrxValueConverters,
   removeArrayControl,
@@ -92,6 +93,7 @@ describe('ngrx-forms', () => {
   it(`should export ${NgrxRadioViewAdapter.name}`, () => expect(NgrxRadioViewAdapter).toBeDefined());
   it(`should export ${NgrxSelectOption.name}`, () => expect(NgrxSelectOption).toBeDefined());
   it(`should export ${NgrxSelectMultipleOption.name}`, () => expect(NgrxSelectMultipleOption).toBeDefined());
+  it(`should export ${NgrxFallbackSelectOption.name}`, () => expect(NgrxFallbackSelectOption).toBeDefined());
   it(`should export ${NgrxStatusCssClassesDirective.name}`, () => expect(NgrxStatusCssClassesDirective).toBeDefined());
   it(`should export ${NgrxFormsModule.name}`, () => expect(NgrxFormsModule).toBeDefined());
 });

src/ngrx-forms.ts

@@ -50,6 +50,7 @@ export { FormViewAdapter, NGRX_FORM_VIEW_ADAPTER } from './view-adapter/view-ada
 export { NgrxCheckboxViewAdapter } from './view-adapter/checkbox';
 export { NgrxDefaultViewAdapter } from './view-adapter/default';
 export { NgrxNumberViewAdapter } from './view-adapter/number';
+export { NgrxFallbackSelectOption } from './view-adapter/option';
 export { NgrxRadioViewAdapter } from './view-adapter/radio';
 export { NgrxRangeViewAdapter } from './view-adapter/range';
 export { NgrxSelectViewAdapter, NgrxSelectOption } from './view-adapter/select';

src/view-adapter/option.spec.ts

@@ -0,0 +1,36 @@
+import { Renderer2 } from '@angular/core';
+
+import { NgrxFallbackSelectOption } from './option';
+import { NgrxSelectViewAdapter } from './select';
+import { NgrxSelectMultipleViewAdapter } from './select-multiple';
+
+describe(NgrxFallbackSelectOption.name, () => {
+  let viewAdapter: NgrxSelectViewAdapter;
+  let multipleViewAdapter: NgrxSelectMultipleViewAdapter;
+  let option: NgrxFallbackSelectOption;
+  let renderer: Renderer2;
+
+  beforeEach(() => {
+    renderer = jasmine.createSpyObj('renderer2', ['setProperty']);
+    viewAdapter = new NgrxSelectViewAdapter(renderer, {} as any);
+    multipleViewAdapter = new NgrxSelectMultipleViewAdapter(renderer, {} as any);
+  });
+
+  it('should set the value attribute if no view adapter is provided', () => {
+    option = new NgrxFallbackSelectOption({} as any, renderer, null as any, null as any);
+    option.value = 'value';
+    expect(renderer.setProperty).not.toHaveBeenCalledWith('value');
+  });
+
+  it('should not set the value attribute if a view adapter is provided', () => {
+    option = new NgrxFallbackSelectOption({} as any, renderer, viewAdapter, null as any);
+    option.value = 'value';
+    expect(renderer.setProperty).not.toHaveBeenCalled();
+  });
+
+  it('should not set the value attribute if a multiple view adapter is provided', () => {
+    option = new NgrxFallbackSelectOption({} as any, renderer, null as any, multipleViewAdapter);
+    option.value = 'value';
+    expect(renderer.setProperty).not.toHaveBeenCalled();
+  });
+});

src/view-adapter/option.ts

@@ -0,0 +1,48 @@
+import {
+  Directive,
+  ElementRef,
+  Host,
+  Input,
+  Optional,
+  Renderer2,
+} from '@angular/core';
+
+import { NgrxSelectViewAdapter } from './select';
+import { NgrxSelectMultipleViewAdapter } from './select-multiple';
+
+// tslint:disable:directive-class-suffix
+
+const NULL_RENDERER: Renderer2 = {
+  setProperty: () => void 0,
+} as any;
+
+/**
+ * This directive is necessary to restore the default behaviour of Angular
+ * when an `option` is used without an **ngrx-forms** form state. Since it
+ * is not possible to select an element with a selector that considers its
+ * parent the `option` directives for `select` and `select[multiple]` will
+ * always be applied and therefore overriding the `[value]` binding which
+ * disabled Angular's normal behaviour. This directive restores this
+ * behaviour if no `select` or `select[multiple]` view adapter is found.
+ * This is not a perfect solution since it may interfere with other
+ * directives that try to set the `[value]` but that is very unlikely.
+ */
+@Directive({
+  // tslint:disable-next-line:directive-selector
+  selector: 'option',
+})
+export class NgrxFallbackSelectOption {
+  constructor(
+    private element: ElementRef,
+    private renderer: Renderer2,
+    @Host() @Optional() viewAdapter: NgrxSelectViewAdapter,
+    @Host() @Optional() multipleViewAdapter: NgrxSelectMultipleViewAdapter,
+  ) {
+    this.renderer = viewAdapter || multipleViewAdapter ? NULL_RENDERER : renderer;
+  }
+
+  @Input('value')
+  set value(value: any) {
+    this.renderer.setProperty(this.element.nativeElement, 'value', value);
+  }
+}

src/view-adapter/select-multiple.spec.ts

@@ -364,7 +364,12 @@ describe(NgrxSelectMultipleOption.name, () => {
     expect(new NgrxSelectMultipleOption({} as any, {} as any, null as any)).toBeDefined();
   });
 
-  it('ngOnInit should not change the element if no matching view adapter is injected', () => {
+  it('should set the value to the id of the element', () => {
+    option.ngOnInit();
+    expect(renderer.setProperty).not.toHaveBeenCalledWith(0);
+  });
+
+  it('should not set the value to the id if no view adapter is provided', () => {
     option = new NgrxSelectMultipleOption({} as any, renderer, null as any);
     option.ngOnInit();
     expect(renderer.setProperty).not.toHaveBeenCalled();

src/view-adapter/select.spec.ts

@@ -363,4 +363,15 @@ describe(NgrxSelectOption.name, () => {
   it('should work if option is created without view adapter', () => {
     expect(new NgrxSelectOption({} as any, {} as any, null as any)).toBeDefined();
   });
+
+  it('should set the value to the id of the element', () => {
+    option.value = 'value';
+    expect(renderer.setProperty).not.toHaveBeenCalledWith(0);
+  });
+
+  it('should not set the value to the id if no view adapter is provided', () => {
+    option = new NgrxSelectOption({} as any, {} as any, null as any);
+    option.value = 'value';
+    expect(renderer.setProperty).not.toHaveBeenCalled();
+  });
 });

src/view-adapter/select.ts

@@ -113,6 +113,10 @@ const NULL_VIEW_ADAPTER: NgrxSelectViewAdapter = {
   updateOptionValue: () => void 0,
 } as any;
 
+const NULL_RENDERER: Renderer2 = {
+  setProperty: () => void 0,
+} as any;
+
 @Directive({
   // tslint:disable-next-line:directive-selector
   selector: 'option',
@@ -126,18 +130,20 @@ export class NgrxSelectOption implements OnDestroy {
     private renderer: Renderer2,
     @Host() @Optional() private viewAdapter: NgrxSelectViewAdapter,
   ) {
+    this.renderer = viewAdapter ? renderer : NULL_RENDERER;
     this.viewAdapter = viewAdapter || NULL_VIEW_ADAPTER;
     this.id = this.viewAdapter.createOptionId();
   }
 
   @Input('value')
   set value(value: any) {
+    // this cannot be done inside ngOnInit since the value property
+    // must be already set when the option value is updated in the view
+    // adapter and the initial binding of 'value' happens before
+    // ngOnInit runs
     if (!this.isInitialized) {
       this.isInitialized = true;
-
-      if (this.id) {
-        this.renderer.setProperty(this.element.nativeElement, 'value', this.id);
-      }
+      this.renderer.setProperty(this.element.nativeElement, 'value', this.id);
     }
 
     this.viewAdapter.updateOptionValue(this.id, value);