fix(ngModel): remove reference to parentForm from removed control #12418

Narretz · 2015-07-23T14:30:57Z

Otherwise, once the removed control's validity changes,
it will continue to be reflected on its former parent form.

@gkalpak can you take a look?

gkalpak · 2015-07-28T08:00:18Z

src/ng/directive/form.js

@@ -168,6 +168,7 @@ function FormController(element, attrs, $scope, $animate, $interpolate) {
    });

    arrayRemove(controls, control);
+    control.$$parentForm = nullFormCtrl;


I think a similar change is needed in $addControl(): control.$$parentForm = form

seems right

gkalpak · 2015-07-28T08:23:56Z

Since nested ngForms are also treated as controls to parent-forms (i.e. can be added/removed via $add/removeControl()), similar changes need to be made in formDirective (i.e. referencing form.$$parentForm instead of the private parentForm variable.

Note, that this issue does not only affect validity states, but other properties or forms/controls, such as $dirty, $submitted etc.

It would also be nice to have more thorough testing (testing those other properties - not only $error).

But other than the comments above, it LGTM 😄

gkalpak · 2015-07-28T08:25:25Z

(And of course, there is the fail on Travis, but it's an easy fix 😛)

Narretz · 2015-08-04T16:20:06Z

@gkalpak Thanks for the review! Do you remember what failure Travis recorded? It's such a pain to look through the logs.

caitp · 2015-08-04T17:39:01Z

@gkalpak Thanks for the review! Do you remember what failure Travis recorded? It's such a pain to look through the logs.

Running "jshint:ng" (jshint) task
   src/ng/directive/ngModel.js
    241 |  var parentForm = this.$$parentForm = $element.inheritedData('$formController') || nullFormCtrl
                                                                                                         ^ Missing semicolon.
>> 1 error in 70 files
Warning: Task "jshint:ng" failed.� Use --force to continue.
Aborted due to warnings.
The command "./scripts/travis/build.sh" exited with 3.
cache.2
store build cache
1.74snothing changed, not updating cache
$ ./scripts/travis/print_logs.sh

Narretz · 2015-08-05T13:21:55Z

test/ng/directive/ngModelSpec.js

@@ -19,7 +19,6 @@ describe('ngModel', function() {
      };

      element = jqLite('<form><input></form>');
-      element.data('$formController', parentFormCtrl);


Since the parentFormCtrl is now extracted via regular require, this mechanism doesn't work anymore. This might break people's tests if they relied on the same technique. Are we okay with this?

Narretz · 2015-08-05T13:27:49Z

Thanks @caitp
This turned out to be a bit more complex, since the adding of controls is handled differently in forms / ngModel. I refactored this now, so that the initial adding of controls happens in the preLink function for both form and ngModel (this actually saves one instance of accessing the parent form controller in ngModel). The logic of assigning the parentForm to the control is moved to the $addControl.
When a control is added, its validity, dirty and submit states will be reflected automatically on the parent form.
I'll have to test if these changes impact performance.

Narretz · 2015-08-24T14:55:59Z

@gkalpak / @caitp can you please take another look?

caitp · 2015-08-26T16:01:52Z

src/ng/directive/form.js

+
+    control.$$parentForm = form;
+
+    forEach(control.$pending, function(value, name) {


this may not be a big deal in most cases, but 3x forEach() calls seems like a perf hit, which would be most noticeable in forms with visibility controlled by ng-if or ng-switch. I'm not saying it's a huge problem, but yeah

caitp · 2015-08-26T16:02:21Z

apart from 1 comment, 3rd commit looks ok to me

gkalpak · 2015-08-27T12:13:05Z

src/ng/directive/form.js

+    }
+
+    if (control.$submitted && !form.$submitted) {
+      form.$setSubmitted();


Do we really want this ?
It is really a corner case, but if we already have unsubmitted form (with more fields/child-forms), then adding a submitted child-form shouldn't necessarily mark the form as submitted.
(The same holds for a submitted form and unsubmitted child-form being added.)

Neither way seems intuitive (so maybe it's better to leave the property untouched).

I guess this is in accordance to what happens in $setSubmitted() (where the parent form is also marked as submitted), but it feels unintuitive.

Anyway, since this is how things are, let's leave it as is.

gkalpak · 2015-08-27T13:06:05Z

src/ng/directive/form.js

@@ -204,7 +224,7 @@ function FormController(element, attrs, $scope, $animate, $interpolate) {
        delete object[property];
      }
    },
-    parentForm: parentForm,
+    parentForm: form.$$parentForm,


This property is not used, since you are accessing the parent form via ctrl.$$parentForm inside addSetValidity().

Narretz · 2015-09-01T19:00:55Z

Thanks for the review @gkalpak, I have fixed the cases where the wrong parent was referenced.
I've also reverted the behavior where calling addControl would try to set the current state (error, success, pending, dirty, submitted) on the parentForm. This way, we match the previous behavior, and we avoid possible performance penalties. I've added documentation about this.

gkalpak · 2015-09-02T11:59:17Z

test/ng/directive/formSpec.js

+    changeInputValue(input, 'a');
+
+    expect(form.$error.maxlength).toBeFalsy();
+  });


In this test you are not actually testing that the form has reacted to changes in the added control.
After you add the control you only check form.$error.maxlength (which hasn't changed since the last time you checked).

gkalpak · 2015-09-02T12:10:37Z

Apart for a couple of nitpicks in tests, it LGTM 👍

…forms This delegates setting the control's parentForm to the parentForm's $addControl method. This way, the model controller saves one instance of looking up the parentForm controller. The form controller keeps two lookups (one for its own ctrl, one for the optional parent). This also fixes adding the parentForm in the following case: - a control is removed from a parent, but its corresponding DOM element is not destroyed - the control is then re-added to the form Before the fix, the control's parentForm was only set once during controller initialization, so the the parentForm would not be set on the control in that specific case.

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

Test that re-added controls propagate validity changes to the parent form. Ensure that when a form / control that was removed and then attached to a different parent, is renamed / deleted, the new parent will be notified of the change. Document that dynamic adding / removing of controls may require manually propagating the current state of the control to the parent form.

Narretz · 2015-09-04T09:59:12Z

Thx again for the reviews! Landed as f8a07dd^..aa11dfc

Narretz added type: bug component: forms branch: 1.3.x severity: broken expected use labels Jul 23, 2015

Narretz added this to the 1.4.5 milestone Jul 23, 2015

googlebot added the cla: yes label Jul 23, 2015

gkalpak reviewed Jul 28, 2015
View reviewed changes

Narretz self-assigned this Jul 31, 2015

Narretz reviewed Aug 5, 2015
View reviewed changes

Narretz force-pushed the fix-parent-form branch from 27897bb to 441ac05 Compare August 13, 2015 14:25

Narretz removed their assignment Aug 23, 2015

Narretz force-pushed the fix-parent-form branch 4 times, most recently from efffd1f to 8f32876 Compare August 24, 2015 14:55

caitp reviewed Aug 26, 2015
View reviewed changes

gkalpak reviewed Aug 27, 2015
View reviewed changes

Narretz modified the milestones: 1.4.5, 1.4.6 Aug 31, 2015

Narretz force-pushed the fix-parent-form branch 4 times, most recently from 95afb91 to c2be9f7 Compare September 1, 2015 18:55

gkalpak reviewed Sep 2, 2015
View reviewed changes

Narretz force-pushed the fix-parent-form branch 2 times, most recently from 3b80047 to 3a1c446 Compare September 3, 2015 22:17

Narretz added 4 commits September 4, 2015 00:31

style(formSpec): fix indentation

cc55ce8

Narretz force-pushed the fix-parent-form branch from 3a1c446 to cc55ce8 Compare September 3, 2015 22:32

Narretz closed this Sep 4, 2015

Narretz deleted the fix-parent-form branch September 4, 2015 09:59


		control.$$parentForm = form;

		forEach(control.$pending, function(value, name) {

fix(ngModel): remove reference to parentForm from removed control #12418

fix(ngModel): remove reference to parentForm from removed control #12418

Uh oh!

Conversation

Narretz commented Jul 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jul 28, 2015

Uh oh!

gkalpak commented Jul 28, 2015

Uh oh!

Narretz commented Aug 4, 2015

Uh oh!

caitp commented Aug 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Aug 5, 2015

Uh oh!

Narretz commented Aug 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caitp commented Aug 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Sep 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Sep 2, 2015

Uh oh!

Narretz commented Sep 4, 2015

Uh oh!

Uh oh!