Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Input validity reflected on form after $removeControl #12263

Closed
TheSharpieOne opened this issue Jul 2, 2015 · 6 comments
Closed

Input validity reflected on form after $removeControl #12263

TheSharpieOne opened this issue Jul 2, 2015 · 6 comments

Comments

@TheSharpieOne
Copy link

After removing a field from a form using $removeControl, if you change the field's value [to be invalid] it will affect the validity of the form it was removed from.
Not too sure if this is working as expected or not, but seems to be undesired.
Affected versions [at least] 1.3.x, 1.4.x

Steps to reproduce (given the following plnkr)
http://plnkr.co/edit/NZJn0zwWdKxvrIlQxUmn?p=preview

  1. Add an invalid value into the field (it accepts numbers).
  2. Note the validity of the form and field. (Form: invalid; Field: invalid)
  3. Click the button to remove the control via $removeControl(...)
  4. Note the validity of the form and field. (Form: valid; Field: [not on formCtrl])
  5. Edit the field to be invalid.
  6. Note the validity of the form and field. (Form: invalid; Field: [not on formCtrl])

For step 6 I would expect the form to no longer be affected by the field, especially since step 4 set the form to valid when the invalid field was removed.

@Narretz
Copy link
Contributor

Narretz commented Jul 2, 2015

It's definitely unexpected and broken from this perspective. That said, removing the control without actually removing the input is not something that's best practive. Can you share the scenario in which you need this?

@TheSharpieOne
Copy link
Author

My particular scenario is that I need to have the field not be validated in any way under certain circumstances. I have a directive that evaluates an expression and determines to whether to add or remove the control from the form. It seemed easier than messing with the validate pipeline (and possibly parsers pipeline.) I couldn't find a way to not validate fields besides actually removing the field from the DOM, which wasn't an option.

@FesterCluck
Copy link

The best solution I've found so far is to change the 'type' of the input to an unsupported value. This should preclude it from constraint validation, yet still leave it as submit-able. The 'hidden' type works like this, but with the addition of preventing render. It's standard behavior for an input of an unknown type to render as a text input. Since willValidate() isn't called until the submit happens, it should be fine to change the type value all you need before submitting.

HTML5 Constraints Validation API
https://peszko.pl/html5/forms.html#the-constraint-validation-api
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/HTML5/Constraint_validation

@TheSharpieOne @Narretz

Example:
http://plnkr.co/edit/itlZECyyj8tEVk5CGfNj

@gkalpak
Copy link
Member

gkalpak commented Jul 3, 2015

FWIW, I believe removing the control is a valid usecase (albeit uncommon) and should be supported.

@Narretz (since you are an ngModel/forms guru 😃) do you think the fix would be complicated ?

@gkalpak
Copy link
Member

gkalpak commented Jul 3, 2015

After looking into it a bit, we don't seem to properly support adding or removing controls after the initialization phase, although we provide $addControl() and $removeControl() methods.

Basically, form.$add/removeControl() does handle the stuff that needs to be taken care of as far as the form is concerned, but nobody hadles the stuff that needs to be taken care of at the control level.
(I.e. we never unset the controler parentForm, which results in subsequent changes on the control affecting the (old) parent form. This does not only affect validity state, but dirty-ness as well (and maybe other things).)

Essentially, there doesn't seem to be provision for re-assigning parentForm (e.g. removing the current parentForm and/or assigning a different parentForm).

IMO, it would make more sense to (also) provide add/remove methods on the control itself. They would take care of internal stuff and also call the parentForm.$add/removeControl().
Taking a step further, I might consider making form.$add/removeControl() private, as there doesn't seem to be much value in them outside of the context of ngModel/ngForm.

@Narretz
Copy link
Contributor

Narretz commented Jul 5, 2015

@gkalpak I think you are quite right. Basically, $removeControl assumes that the removed control is actually completely removed from the UI (like when it's called from the $destroy event), so the cleanup doesn't have to deal with the child control.
The reference to the parentForm currently is not a property of the ngModel controller lives inside the addSetValidityMethod closure, so we might have to get it out there to fix this. Unfortunately, this part of the code is already pretty messy, so we shouldn't rush this.

Narretz added a commit to Narretz/angular.js that referenced this issue Jul 23, 2015
Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Aug 13, 2015
Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Aug 24, 2015
Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Aug 24, 2015
Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Aug 24, 2015
Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Aug 24, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Sep 1, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Sep 3, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Sep 3, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes angular#12263
Narretz added a commit to Narretz/angular.js that referenced this issue Sep 3, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes angular#12263
@Narretz Narretz closed this as completed Sep 4, 2015
Narretz added a commit that referenced this issue Sep 4, 2015
This fixes cases where the control gets removed, but the control's
element stays in the DOM.
Previously, if the removed control's validity changed, a reference to
it would again be stored on its former parent form.

Fixes #12263
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants