Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactive Form change through ControlValueAccessor doesn't update view #13792

Closed
tiagoroldao opened this issue Jan 5, 2017 · 59 comments
Closed
Labels
area: forms forms: ControlValueAccessor freq3: high needs reproduction This issue needs a reproduction in order for the team to investigate further P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent type: bug/fix
Milestone

Comments

@tiagoroldao
Copy link

tiagoroldao commented Jan 5, 2017

I'm submitting a ... (check one with "x")

[x] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
When updating a value through a form field that uses the formControl directive, the view isn't updated. When, say, two fields hold the same form control, changing one does NOT change the other.

This does happen when using NgModel, and does happen when calling formControl.setValue(newVal)

Expected behavior
Behaviour is expected to be the same in NgModel as when using Reactive FormControls

Minimal reproduction of the problem with instructions
http://plnkr.co/edit/LXyo6uHdbEpP3feudYoo?p=preview

What is the motivation / use case for changing the behavior?
The contract custom form fields have when using the ControlValueAccessor interface is broken. A good example is http://valor-software.com/ng2-bootstrap/#/buttons#radio - which works currently only with NgModel, as it requires updating the view on all relevant buttons on value change.

When using a set of buttons with reactive forms, currently, a (click) based hack needs to be put in place:

<div class="btn-group">
    <label 
        class="btn btn-primary" 
        formControlName="privateApplication"
        (click)="form.get('privateApplication').setValue(false)"
        [btnRadio]="false">
            {{'LABEL_PUBLIC' | translate}}
    </label>
    <label 
        class="btn btn-primary" 
        formControlName="privateApplication" 
        (click)="form.get('privateApplication').setValue(true)"
        [btnRadio]="true">
            {{'LABEL_PRIVATE' | translate}}
    </label>
</div>

Please tell us about your environment:
OS Not relevant

  • Angular version: 2.4.1

  • Browser: Chrome 55

  • Language: TypeScript 2

@Toxicable
Copy link

Can you recheck this on the latest version, all appears to be working fine for me

@tiagoroldao
Copy link
Author

The plunker is up to date (2.4.9) and still displays issue - are you saying you don't see issue in the plunker?

@Toxicable
Copy link

Toxicable commented Mar 3, 2017

@tiagoroldao what is meant to happen? I click the ngmodel button and the 2 inputs + 1 label update.
I click the Update Field button and it's 2 inputs change along with it's label, without moving my mouse or causing any other forms of CD

@tiagoroldao
Copy link
Author

@Toxicable summing it up:
Top (correct) form using [(ngModel)]:

  • changing any field updates the model and the other field, which shares value
  • clicking the button changes model and both fields

Bottom (buggy) form using formControlName:

  • changing any field updates the model but not the other field, which should share value
  • clicking the button changes model and both fields

@Toxicable
Copy link

Sorry, ignore the above. I reread your issue see whats going on here, I don't know the inner workings overly well but it looks to me like a limitation of Reactive Forms currently not necessarily a bug. Maybe opening a Feature request for this after checking if there isn't one already

@tiagoroldao
Copy link
Author

I do see it as a bug - when updating the reactive formControl through the directive, it specifically updates it without updating views (I'm thinking it assumes it isn't needed as it came from the view, but it doesn't account for multiple uses of the same formControl)

The culprit is here:

control.setValue(newValue, {emitModelToViewChange: false});

Although I don't know the full performance implications of removing emitModelToViewChange: false

@tiagoroldao
Copy link
Author

By the way, the problem is fixed by removing the assumption that the views do not need to be updated when a change comes from view to model. Fixed here:
http://plnkr.co/edit/b7u3XWg1tFIsWTpAvQdH?p=preview

Still I can't presume to know the implications of this. @DzmitryShylovich who could verify this?

@Ledzz
Copy link

Ledzz commented Mar 31, 2017

For those who came from google with this issue like me, there's a workaround that saved me:

control.valueChanges.subscribe(e => {
    control.setValue(e, {emitEvent: false});
});

@mlb6
Copy link

mlb6 commented Jul 12, 2017

Facing the same issue. Thank you @Ledzz for the workaround.

@mmmichl
Copy link

mmmichl commented Nov 2, 2017

@Ledzz solution works, but be aware of the jumping cursor in IE11:
ie11-typing
Try it: https://plnkr.co/edit/ilRx7V7VfNbzgB2lqC1v?p=preview

I need a input field and a select to work on the same control. Following workaround worked for me:

<form [formGroup]="form">
  <input type="text" formControlName="bewarage">
  <select formControlName="bewarage" #s
      [value]="form.get('bewarage').value"
      (change)="form.get('bewarage').setValue(s.value, {emitEvent: false})">
    <option *ngFor="let b of bewarages" value="{{b}}">{{b}}</option>
  </select>
</form>

try it: https://plnkr.co/edit/WaLHOlcNAVbSAH8m0a7o?p=preview

On topic: In my opinion this is an unexpected behaviour, so I see it as a bug not a missing feature.

@johannesjo
Copy link

johannesjo commented Nov 19, 2017

No news in this one? I've the same issue with ng4 and ng5 as well.

@FabienInan
Copy link

Thanks for the solution. But i hope this bug fixed soon.

@orasestravis
Copy link

I'm having the same problem. The workaround from @Ledzz does not work for me. I also tried calling ChangeDetectorRef.detectChanges() in my controller, but that doesn't help either.

@seth-git
Copy link

I'm having the same problem. When I do this from my controller:

this.myform.setValue({
	someLabel: somevalue
});

With this template:

<form [formGroup]="myform" novalidate>
   <custom-component formControlName="someLabel"></custom-component>
</form>

My custom component that implements ControlValueAccessor, never gets the new value passed to it's writeValue method. There appear to be no workarounds.

@simeyla
Copy link

simeyla commented Jan 20, 2018

If you only have a subset of form fields you know to be 'duplicated' you're slightly better off using patchValue instead.

 this.addressForm.valueChanges.subscribe((data) => {
        this.addressForm.patchValue({ firstName: data.firstName, lastName: data.lastName }, 
                                    { emitEvent: false });
  });

This will probably mitigate issues with browser controls quirks such as the above mentioned IE11 issue.

Still strikes me as bizarre this is the 'by design' behavior. I suppose it is a performance issue. Everything in angular magically binds - except for the simplest forms :-/

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018
@jayordway
Copy link

I have the same problem as well. Both setValue doesn't cause a change nor is valueChanges firing when values are set.

@mlc-mlapis
Copy link
Contributor

... the problem is still valid for Angular 5.2.4 https://stackblitz.com/edit/mlc-two-inputs-same-formcontrol?file=app%2Fapp.component.html

@mohamed-gara
Copy link

There are two points to note about the following code: http://plnkr.co/edit/LXyo6uHdbEpP3feudYoo?p=preview

First, it's worth to say that in this example: there is a subtle difference between the template driven and reactive versions:

  • In the reactive version, there is a single FormControl object.
  • But in the template driven version there are two FormControl objects.

We can avoid this difference by wrapping the inputs in a form.

Second, the two versions (template driven and reactive) are not really equivalent. And the problem (if it's really a problem), exists also in the template driven forms as in the reactive ones. Here http://plnkr.co/edit/mPDLG7MZlp6KRvkYBN89?p=preview I give the equivalent reactive version of the template driven one and the template driven version equivalent to the reactive one.

@HelloPb
Copy link

HelloPb commented May 27, 2018

This problem still exist. No fix from angular yet. I am facing the same issue for custom radio button control using reactiveforms

@sayax
Copy link

sayax commented Jun 4, 2018

If you have any solution please share, i have the same issue with custom checkbox, currently using formcontrolname with input to update which is not the best practice

@HelloPb
Copy link

HelloPb commented Jun 4, 2018

I am doing this way to solve the issue. Its working

  1. radio.component.html

`<div class="d-inline-flex" (click)="toggle()">
put your radio input here

`
  1. radio.component.ts

`import { Component, OnInit, Input, forwardRef, Self, Optional } from '@angular/core';
import { NG_VALUE_ACCESSOR, NG_VALIDATORS, ControlValueAccessor, NgControl, FormControl, Validator } from '@angular/forms';

@component({
selector: 'abc-radio',
templateUrl: './radio.component.html',
styleUrls: ['./radio.component.scss']
})
export class RadioComponent implements OnInit, ControlValueAccessor {

@input() label = '';
@input() value: any;

public selected = false;
public disabled = false;

constructor(public ngControl: NgControl) {
if (this.ngControl != null) { this.ngControl.valueAccessor = this; }
}

private propagateChange = (_: any) => { };

public toggle(): void {
if (!this.disabled) {
this.selected = true;
this.propagateChange(this.value);
this.ngControl.control.setValue(this.value);
}
}

private update(value: any): void {
if (value === this.value) {
this.selected = true;
} else {
this.selected = false;
}
}

public writeValue(value: any): void {
this.update(value);
}

public registerOnChange(fn: any): void {
this.propagateChange = fn;
}

public registerOnTouched(fn: any): void {
}

public setDisabledState(isDisabled: boolean): void {
this.disabled = isDisabled;
}

public ngOnInit(): void {
this.ngControl.control.valueChanges.subscribe(v => {
this.update(v);
});
}
}`

@sayax
Copy link

sayax commented Jun 4, 2018

Well i have a working solution but it requires some input directives, however i wanna do it without the help of inputs only using formcontrolname, its kinda essential in my projects

@seth-git
Copy link

seth-git commented Jun 9, 2018

This worked for me for both a reactive form and a non-reactive one:

import {ChangeDetectionStrategy, ChangeDetectorRef, Component, forwardRef, Input} from "@angular/core";
import {Observable} from "rxjs/Observable";
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from "@angular/forms";
...

@Component({
	selector: 'my-select',
	changeDetection: ChangeDetectionStrategy.OnPush,
	templateUrl: './outcome-select.template.html',
	styleUrls: ['./outcome-select.scss'],
	providers: [
		{
			provide: NG_VALUE_ACCESSOR,
			useExisting: forwardRef(() => OutcomeSelectComponent),
			multi: true
		}
	]
})
export class OutcomeSelectComponent implements ControlValueAccessor {
	// For more on the style of this component, see: https://alligator.io/angular/custom-form-control/
	@Input() isRequired: boolean;
	@Input() noSelectionText: string;
	@Input('value') _value: number; // outcome id
	onChange: any = () => { };
	onTouched: any = () => { };

	options$: Observable<OutcomeOption[]>;

	get value(): number {
		return this._value;
	}

	set value(val: number) {
		this._value = val;
		this.onChange(val);
		this.onTouched();
		this.cd.detectChanges();
	}

	constructor(
		private cd: ChangeDetectorRef
	) {
		this.options$ = Observable.of([
			{
				name:'Option 1',
				id: 1
			}, {
				name: 'Option 2',
				id: 2
			}
		]);
	}

	registerOnChange(fn) {
		this.onChange = fn;
	}

	registerOnTouched(fn) {
		this.onTouched = fn;
	}

	writeValue(value: number) {
		this.value = value;
	}
}

The template looks like this:

<select
		[required]="isRequired"
		[(ngModel)]="value">
	<option value="" [disabled]="isRequired">{{ noSelectionText }}</option>
	<option [ngValue]="opt.id" *ngFor="let opt of (options$ | async)">{{opt.name}}</option>
</select>

@slavafomin
Copy link

@chrisv2 Have you tried setting it this way?

<input [formControl]="myFormCtrl" type="radio" name="variant" [value]="1">

@nguyenbathanh
Copy link

This worked for me for both a reactive form and a non-reactive one:

import {ChangeDetectionStrategy, ChangeDetectorRef, Component, forwardRef, Input} from "@angular/core";
import {Observable} from "rxjs/Observable";
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from "@angular/forms";
...

@Component({
	selector: 'my-select',
	changeDetection: ChangeDetectionStrategy.OnPush,
	templateUrl: './outcome-select.template.html',
	styleUrls: ['./outcome-select.scss'],
	providers: [
		{
			provide: NG_VALUE_ACCESSOR,
			useExisting: forwardRef(() => OutcomeSelectComponent),
			multi: true
		}
	]
})
export class OutcomeSelectComponent implements ControlValueAccessor {
	// For more on the style of this component, see: https://alligator.io/angular/custom-form-control/
	@Input() isRequired: boolean;
	@Input() noSelectionText: string;
	@Input('value') _value: number; // outcome id
	onChange: any = () => { };
	onTouched: any = () => { };

	options$: Observable<OutcomeOption[]>;

	get value(): number {
		return this._value;
	}

	set value(val: number) {
		this._value = val;
		this.onChange(val);
		this.onTouched();
		this.cd.detectChanges();
	}

	constructor(
		private cd: ChangeDetectorRef
	) {
		this.options$ = Observable.of([
			{
				name:'Option 1',
				id: 1
			}, {
				name: 'Option 2',
				id: 2
			}
		]);
	}

	registerOnChange(fn) {
		this.onChange = fn;
	}

	registerOnTouched(fn) {
		this.onTouched = fn;
	}

	writeValue(value: number) {
		this.value = value;
	}
}

The template looks like this:

<select
		[required]="isRequired"
		[(ngModel)]="value">
	<option value="" [disabled]="isRequired">{{ noSelectionText }}</option>
	<option [ngValue]="opt.id" *ngFor="let opt of (options$ | async)">{{opt.name}}</option>
</select>

Only this solution works for me, simple and straightforward.

@alexfeds
Copy link

alexfeds commented Jun 11, 2020

Another solution that worked for me, using timeout, bit hacky but does a job, just wrap the patchValue or setValue with timeout

  setTimeout(() => {
        this.complexSearchForm.patchValue({
          complexSearchQueryCtrl: ruleSet,
          filterTagsCtl: {
            filterTags: obj.tags ? true : false,
            selectedTagTypes: obj.tags && obj.tags.required ? obj.tags.required : [],
            excludedTagTypes: obj.tags && obj.tags.excluded ? obj.tags.excluded : []
          },
        });
      }, 0);

@stefanomiccoli
Copy link

Got stuck with this issue in a scenario pretty close to the @maxime1992 colorpicker. Very disappointed when I saw this issue open for years... If not recognised as an issue it would be nice to hear the reasons from someone in angular core team.

in our cases, only solution was to trigger a this.form.patchValue(changes, { emitEvent: false }). Without emitEvent to false it would generate a loop

@JoepKockelkorn
Copy link

Another solution that worked for me, using timeout, bit hacky but does a job, just wrap the patchValue or setValue with timeout

  setTimeout(() => {
        this.complexSearchForm.patchValue({
          complexSearchQueryCtrl: ruleSet,
          filterTagsCtl: {
            filterTags: obj.tags ? true : false,
            selectedTagTypes: obj.tags && obj.tags.required ? obj.tags.required : [],
            excludedTagTypes: obj.tags && obj.tags.excluded ? obj.tags.excluded : []
          },
        });
      }, 0);

While this works definitely, it's not optimal as Angular needs another ChangeDetection cycle to pick up your changes. Did you try to call changeDetectorRef.detectChanges() after the patchValue?

@alexfeds
Copy link

alexfeds commented Jul 16, 2020

Another solution that worked for me, using timeout, bit hacky but does a job, just wrap the patchValue or setValue with timeout

  setTimeout(() => {
        this.complexSearchForm.patchValue({
          complexSearchQueryCtrl: ruleSet,
          filterTagsCtl: {
            filterTags: obj.tags ? true : false,
            selectedTagTypes: obj.tags && obj.tags.required ? obj.tags.required : [],
            excludedTagTypes: obj.tags && obj.tags.excluded ? obj.tags.excluded : []
          },
        });
      }, 0);

While this works definitely, it's not optimal as Angular needs another ChangeDetection cycle to pick up your changes. Did you try to call changeDetectorRef.detectChanges() after the patchValue?

I did try the change detection, but did nothing. What I finally did, I removed the [ngModel], and only used the reactive approach. In case you have [ngModel] and reactive form together, another solution to update the view via setting the models with values, like this.filterTags = someValue. after the patching, in this case it updates the view

@hudzenko
Copy link

hudzenko commented Sep 8, 2020

Almost 4 years for this bug.. 👎

I have a stepper in my app and last step is a summary of previous forms. Well I have to use same form groups at the last step, but as it is formcontrols duplication summary is not updated anytime

@jelbourn jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed severity3: broken labels Oct 1, 2020
@guilhermetod
Copy link

guilhermetod commented Mar 7, 2021

I'm not sure if this is a bug actually. The point of the ControlValueAccessor is to act as a bridge between the control and the native element, as stated in the docs again. If you override the default provider, you need your own implementantion, includeing the native element update.

I have a currency control, where I want to keep the input display value masked, but record it as a numeric value in my formControl, so I can submit it straight away. If the Control Value Accessor keeps syncing the formControl to the native element, this will turn impossible. It seems reasonable for me that we should use renderer to set the native element input.

I think the confusion is due to the fact that this seems not to be needed in the template form, because the model is updated with the simple property change, but only because that's how ngModel behaves. The flow behind the reactive forms is different.

That being said, it only seems reasonable after a very long time that I spent not being able to understand what was happenning. I think the docs could get some attention not only for this issue, but for the ControlValueAccessor in general. I found really hard to understand the correct implementation of it at first, specially for simple cases, because most articles I found online were either template forms and the simple property update would be enough to update the view, or form groups with child controls, which, in the end, delegated the view update to the child controls.

A simple mask input example would be better to understand the purpose and how the ControlValueAccessor works.

Again, for the forms docs, I found the (very cool) data flow diagrams a bit misleading, because they lead you to think that the view to model and vice-versa flow is done through the FormControlDirective, when in fact it seems to be trough the ControlValueAccessor.

@harshalagrawal03
Copy link

I am also facing this issue.
Same form control is used at two places in view and when one is changed by user the other view is not changing.
However, using ngModel resolves the issue. However ngModel and formControl's simultaneous usage is deprecated now.

@ko1ebayev
Copy link

ko1ebayev commented Jan 9, 2024

I guess triggering change detection could help in case of writing value from outside
but logic tells me that it must be the framework work if it allows us to implement controlValueAccessor

@JeanMeche
Copy link
Member

JeanMeche commented May 9, 2024

Same as @guilhermetod, I'm not sure every usecase given here is a bug.

I took the stackblitz example given earlier and fixed it: https://stackblitz.com/edit/angular-urxsu8?file=src%2Fmain.ts

In that particuliar case, the inner model was not updated meaning the on the subsequent writeValue wrote in the same value hence not triggering the change.

I'll close this for now. If someone can provide a stackblitz repro of this issue after taking my remarks into account, I'll reopen.

@JeanMeche JeanMeche closed this as not planned Won't fix, can't repro, duplicate, stale May 9, 2024
@JeanMeche JeanMeche added needs reproduction This issue needs a reproduction in order for the team to investigate further and removed state: confirmed labels May 9, 2024
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: forms forms: ControlValueAccessor freq3: high needs reproduction This issue needs a reproduction in order for the team to investigate further P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent type: bug/fix
Projects
None yet
Development

No branches or pull requests