fix(form, ngModel): correctly notify parent form when children are added

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.
angular · Sep 4, 2015 · c6110e8 · c6110e8
1 parent 290b504
commit c6110e8
Show file tree

Hide file tree

Showing 3 changed files with 218 additions and 6 deletions.
diff --git a/src/ng/directive/form.js b/src/ng/directive/form.js
@@ -114,11 +114,23 @@ function FormController(element, attrs, $scope, $animate, $interpolate) {
   /**
    * @ngdoc method
    * @name form.FormController#$addControl
+   * @param {object} control control object, either a {@link form.FormController} or an
+   * {@link ngModel.NgModelController}
    *
    * @description
-   * Register a control with the form.
+   * Register a control with the form. Input elements using ngModelController do this automatically
+   * when they are linked.
    *
-   * Input elements using ngModelController do this automatically when they are linked.
+   * Note that the current state of the control will not be reflected on the new parent form. This
+   * is not an issue with normal use, as freshly compiled and linked controls are in a `$pristine`
+   * state.
+   *
+   * However, if the method is used programmatically, for example by adding dynamically created controls,
+   * or controls that have been previously removed without destroying their corresponding DOM element,
+   * it's the developers responsiblity to make sure the current state propagates to the parent form.
+   *
+   * For example, if an input control is added that is already `$dirty` and has `$error` properties,
+   * calling `$setDirty()` and `$validate()` afterwards will propagate the state to the parent form.
    */
   form.$addControl = function(control) {
     // Breaking change - before, inputs whose name was "hasOwnProperty" were quietly ignored
@@ -147,11 +159,18 @@ function FormController(element, attrs, $scope, $animate, $interpolate) {
   /**
    * @ngdoc method
    * @name form.FormController#$removeControl
+   * @param {object} control control object, either a {@link form.FormController} or an
+   * {@link ngModel.NgModelController}
    *
    * @description
    * Deregister a control from the form.
    *
    * Input elements using ngModelController do this automatically when they are destroyed.
+   *
+   * Note that only the removed control's validation state (`$errors`etc.) will be removed from the
+   * form. `$dirty`, `$submitted` states will not be changed, because the expected behavior can be
+   * different from case to case. For example, removing the only `$dirty` control from a form may or
+   * may not mean that the form is still `$dirty`.
    */
   form.$removeControl = function(control) {
     if (control.$name && form[control.$name] === control) {
@@ -503,13 +522,13 @@ var formDirectiveFactory = function(isNgForm) {
               attr.$observe(nameAttr, function(newValue) {
                 if (controller.$name === newValue) return;
                 setter(scope, undefined);
-                parentFormCtrl.$$renameControl(controller, newValue);
+                controller.$$parentForm.$$renameControl(controller, newValue);
                 setter = getSetter(controller.$name);
                 setter(scope, controller);
               });
             }
             formElement.on('$destroy', function() {
-              parentFormCtrl.$removeControl(controller);
+              controller.$$parentForm.$removeControl(controller);
               setter(scope, undefined);
               extend(controller, nullFormCtrl); //stop propagating child destruction handlers upwards
             });

diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js
@@ -1038,12 +1038,12 @@ var ngModelDirective = ['$rootScope', function($rootScope) {
 
           attr.$observe('name', function(newValue) {
             if (modelCtrl.$name !== newValue) {
-              formCtrl.$$renameControl(modelCtrl, newValue);
+              modelCtrl.$$parentForm.$$renameControl(modelCtrl, newValue);
             }
           });
 
           scope.$on('$destroy', function() {
-            formCtrl.$removeControl(modelCtrl);
+            modelCtrl.$$parentForm.$removeControl(modelCtrl);
           });
         },
         post: function ngModelPostLink(scope, element, attr, ctrls) {

diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js
@@ -95,6 +95,82 @@ describe('form', function() {
   });
 
 
+  it('should react to validation changes in manually added controls', function() {
+    doc = $compile(
+      '<form name="myForm">' +
+        '<input name="control" ng-maxlength="1" ng-model="value" store-model-ctrl/>' +
+      '</form>')(scope);
+
+      scope.$digest();
+
+    var form = scope.myForm;
+
+    var input = doc.find('input').eq(0);
+
+    // remove control and invalidate it
+    form.$removeControl(control);
+    expect(form.control).toBeUndefined();
+
+    changeInputValue(input, 'abc');
+    expect(control.$error.maxlength).toBe(true);
+    expect(control.$dirty).toBe(true);
+    expect(form.$error.maxlength).toBeFalsy();
+    expect(form.$dirty).toBe(false);
+
+    // re-add the control; its current validation state is not propagated
+    form.$addControl(control);
+    expect(form.control).toBe(control);
+    expect(form.$error.maxlength).toBeFalsy();
+    expect(form.$dirty).toBe(false);
+
+    // Only when the input changes again its validation state is propagated
+    changeInputValue(input, 'abcd');
+    expect(form.$error.maxlength[0]).toBe(control);
+    expect(form.$dirty).toBe(false);
+  });
+
+
+  it('should use the correct parent when renaming and removing dynamically added controls', function() {
+    scope.controlName = 'childControl';
+    scope.hasChildControl = true;
+
+    doc = $compile(
+      '<form name="myForm">' +
+        '<div ng-if="hasChildControl">' +
+          '<input name="{{controlName}}" ng-maxlength="1" ng-model="value"/>' +
+        '</div>' +
+      '</form>' +
+      '<form name="otherForm"></form>')(scope);
+
+    scope.$digest();
+
+    var form = scope.myForm;
+    var otherForm = scope.otherForm;
+    var childControl = form.childControl;
+
+    // remove child form and add it to another form
+    form.$removeControl(childControl);
+    otherForm.$addControl(childControl);
+
+    expect(form.childControl).toBeUndefined();
+    expect(otherForm.childControl).toBe(childControl);
+
+    // rename the childControl
+    scope.controlName = 'childControlMoved';
+    scope.$digest();
+
+    expect(form.childControlMoved).toBeUndefined();
+    expect(otherForm.childControl).toBeUndefined();
+    expect(otherForm.childControlMoved).toBe(childControl);
+
+    scope.hasChildControl = false;
+    scope.$digest();
+
+    expect(form.childControlMoved).toBeUndefined();
+    expect(otherForm.childControlMoved).toBeUndefined();
+  });
+
+
   it('should remove scope reference when form with no parent form is removed from the DOM', function() {
     var formController;
     scope.ctrl = {};
@@ -614,6 +690,123 @@ describe('form', function() {
     expect(child.$error.required).toEqual([inputB]);
   });
 
+
+    it('should ignore changes in manually removed child forms', function() {
+      doc = $compile(
+        '<form name="myForm">' +
+          '<ng-form name="childform">' +
+            '<input name="childformcontrol" ng-maxlength="1" ng-model="value"/>' +
+          '</ng-form>' +
+        '</form>')(scope);
+
+      var form = scope.myForm;
+      var childformController = doc.find('ng-form').eq(0).controller('form');
+
+      var input = doc.find('input').eq(0);
+      var inputController = input.controller('ngModel');
+
+      changeInputValue(input, 'ab');
+      scope.$apply();
+
+      expect(form.$dirty).toBe(true);
+      expect(form.$error.maxlength).toBeTruthy();
+      expect(form.$error.maxlength[0].$name).toBe('childform');
+
+      inputController.$setPristine();
+      expect(form.$dirty).toBe(true);
+
+      form.$setPristine();
+
+      // remove child form
+      form.$removeControl(childformController);
+      expect(form.childform).toBeUndefined();
+      expect(form.$error.maxlength).toBeFalsy();
+
+      changeInputValue(input, 'abc');
+      scope.$apply();
+
+      expect(form.$error.maxlength).toBeFalsy();
+      expect(form.$dirty).toBe(false);
+    });
+
+
+    it('should react to changes in manually added child forms', function() {
+      doc = $compile(
+        '<form name="myForm">' +
+          '<ng-form name="childForm">' +
+            '<input name="childformcontrol" ng-maxlength="1" ng-model="value" />' +
+          '</ng-form>' +
+        '</form>')(scope);
+
+      var form = scope.myForm;
+      var childFormController = doc.find('ng-form').eq(0).controller('form');
+
+      var input = doc.find('input').eq(0);
+
+      // remove child form so we can add it manually
+      form.$removeControl(childFormController);
+      changeInputValue(input, 'ab');
+
+      expect(form.childForm).toBeUndefined();
+      expect(form.$dirty).toBe(false);
+      expect(form.$error.maxlength).toBeFalsy();
+
+      // re-add the child form; its current validation state is not propagated
+      form.$addControl(childFormController);
+      expect(form.childForm).toBe(childFormController);
+      expect(form.$error.maxlength).toBeFalsy();
+      expect(form.$dirty).toBe(false);
+
+      // Only when the input inside the child form changes, the validation state is propagated
+      changeInputValue(input, 'abc');
+      expect(form.$error.maxlength[0]).toBe(childFormController);
+      expect(form.$dirty).toBe(false);
+    });
+
+
+    it('should use the correct parent when renaming and removing dynamically added forms', function() {
+      scope.formName = 'childForm';
+      scope.hasChildForm = true;
+
+      doc = $compile(
+        '<form name="myForm">' +
+          '<div ng-if="hasChildForm">' +
+            '<ng-form name="{{formName}}">' +
+              '<input name="childformcontrol" ng-maxlength="1" ng-model="value"/>' +
+            '</ng-form>' +
+          '</div>' +
+        '</form>' +
+        '<form name="otherForm"></form>')(scope);
+
+      scope.$digest();
+
+      var form = scope.myForm;
+      var otherForm = scope.otherForm;
+      var childForm = form.childForm;
+
+      // remove child form and add it to another form
+      form.$removeControl(childForm);
+      otherForm.$addControl(childForm);
+
+      expect(form.childForm).toBeUndefined();
+      expect(otherForm.childForm).toBe(childForm);
+
+      // rename the childForm
+      scope.formName = 'childFormMoved';
+      scope.$digest();
+
+      expect(form.childFormMoved).toBeUndefined();
+      expect(otherForm.childForm).toBeUndefined();
+      expect(otherForm.childFormMoved).toBe(childForm);
+
+      scope.hasChildForm = false;
+      scope.$digest();
+
+      expect(form.childFormMoved).toBeUndefined();
+      expect(otherForm.childFormMoved).toBeUndefined();
+    });
+
+
     it('should chain nested forms in repeater', function() {
       doc = jqLite(
          '<ng:form name=parent>' +