From 61ac2678cf265a659eeb6862d1976f7383d87a34 Mon Sep 17 00:00:00 2001 From: martin Date: Fri, 15 Jan 2016 07:08:56 +0500 Subject: [PATCH 1/2] fix(form): make ngForm $pristine after nested control.$setPristine() When calling $setPristine on the nested form or control, form becomes $pristine of all the nested controls are pristine Closes #13715 --- src/ng/directive/form.js | 36 +++++++++++++-- src/ng/directive/ngModel.js | 1 + test/ng/directive/formSpec.js | 77 +++++++++++++++++++++++++++++++- test/ng/directive/ngModelSpec.js | 8 +++- 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/src/ng/directive/form.js b/src/ng/directive/form.js index 30a932a46347..6c3c2520841d 100644 --- a/src/ng/directive/form.js +++ b/src/ng/directive/form.js @@ -5,6 +5,7 @@ var nullFormCtrl = { $addControl: noop, $$renameControl: nullFormRenameControl, + $$updatePristine: noop, $removeControl: noop, $setValidity: noop, $setDirty: noop, @@ -254,19 +255,46 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { * * This method can be called to remove the 'ng-dirty' class and set the form to its pristine * state (ng-pristine class). This method will also propagate to all the controls contained - * in this form. + * in this form and to the parent form. * * Setting a form back to a pristine state is often useful when we want to 'reuse' a form after * saving or resetting it. */ form.$setPristine = function() { + form.$$setPristineSelf(); + forEach(controls, function(control) { + control.$setPristine(); + }); + }; + + // Private API: Sets the form to its pristine state. + // This method does not affect nested controls. + form.$$setPristineSelf = function() { $animate.setClass(element, PRISTINE_CLASS, DIRTY_CLASS + ' ' + SUBMITTED_CLASS); form.$dirty = false; form.$pristine = true; form.$submitted = false; - forEach(controls, function(control) { - control.$setPristine(); - }); + form.$$parentForm.$$updatePristine(); + }; + + // Private API: update form pristine-ness + form.$$updatePristine = function() { + var isPristine = true, + controlsLength = controls.length, + i; + + for (i = 0; i < controlsLength; i++) { + if (!controls[i].$pristine) { + isPristine = false; + break; + } + } + + if (isPristine) { + // All the nested controls are already pristine. + // Set pristine-ness only for the form itself. + form.$$setPristineSelf(); + } }; /** diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index 4683f4e31c4d..d5fb671f7289 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -383,6 +383,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ ctrl.$pristine = true; $animate.removeClass($element, DIRTY_CLASS); $animate.addClass($element, PRISTINE_CLASS); + ctrl.$$parentForm.$$updatePristine(); }; /** diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 500be66d25d3..4f6e94e7c008 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -714,7 +714,8 @@ describe('form', function() { expect(form.$error.maxlength[0].$name).toBe('childform'); inputController.$setPristine(); - expect(form.$dirty).toBe(true); + // this assertion prevents to propagate prestine to the parent form + // expect(form.$dirty).toBe(true); form.$setPristine(); @@ -1043,6 +1044,80 @@ describe('form', function() { expect(nestedInputCtrl.$pristine).toBe(true); expect(nestedInputCtrl.$dirty).toBe(false); }); + + it('should propagate pristine-ness to the parent form', function() { + doc = $compile( + '
' + + '
' + + '
')(scope); + + var parentForm = doc, + childForm = parentForm.find('div').eq(0), + childFormCtrl = scope.childForm; + + childFormCtrl.$setDirty(); + scope.$apply(); + expect(parentForm).not.toBePristine(); + + childFormCtrl.$setPristine(); + scope.$apply(); + expect(childForm).toBePristine(); + expect(parentForm).toBePristine(); + }); + + it('should be pristine if all the nested controls are pristine', function() { + doc = $compile( + '
' + + '' + + '' + + '
')(scope); + + var form = doc, + formCtrl = scope.form, + input1 = form.find('input').eq(0), + input2 = form.find('input').eq(1), + inputCtrl1 = input1.controller('ngModel'), + inputCtrl2 = input2.controller('ngModel'); + + inputCtrl1.$setDirty(); + inputCtrl2.$setDirty(); + scope.$apply(); + expect(form).not.toBePristine(); + + inputCtrl1.$setPristine(); + scope.$apply(); + expect(form).not.toBePristine(); + + inputCtrl2.$setPristine(); + scope.$apply(); + expect(form).toBePristine(); + }); + + it('should be pristine if all the nested forms are pristine', function() { + doc = $compile( + '
' + + '
' + + '
' + + '
')(scope); + + var form = doc, + formCtrl = scope.form, + childFormCtrl1 = scope.childForm1, + childFormCtrl2 = scope.childForm2; + + childFormCtrl1.$setDirty(); + childFormCtrl2.$setDirty(); + scope.$apply(); + expect(form).not.toBePristine(); + + childFormCtrl1.$setPristine(); + scope.$apply(); + expect(form).not.toBePristine(); + + childFormCtrl2.$setPristine(); + scope.$apply(); + expect(form).toBePristine(); + }); }); describe('$setUntouched', function() { diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index e725d495fe00..56d1476a905b 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -15,7 +15,8 @@ describe('ngModel', function() { $$setPending: jasmine.createSpy('$$setPending'), $setValidity: jasmine.createSpy('$setValidity'), $setDirty: jasmine.createSpy('$setDirty'), - $$clearControlValidity: noop + $$clearControlValidity: noop, + $$updatePristine: jasmine.createSpy('$$updatePristine') }; element = jqLite('
'); @@ -145,6 +146,11 @@ describe('ngModel', function() { expect(ctrl.$dirty).toBe(false); expect(ctrl.$pristine).toBe(true); }); + + it('should propagate pristine to the parent form', function() { + ctrl.$setPristine(); + expect(parentFormCtrl.$$updatePristine).toHaveBeenCalledOnce(); + }); }); describe('setDirty', function() { From 4d8c01c458fdaab57e37e082dadf1b3af89a27db Mon Sep 17 00:00:00 2001 From: martin Date: Sun, 17 Jan 2016 04:30:39 +0500 Subject: [PATCH 2/2] refactor(form): use internal counter instead of recursion --- src/ng/directive/form.js | 83 ++++++++++++---- src/ng/directive/ngModel.js | 22 ++++- test/ng/directive/formSpec.js | 161 +++++++++++++++++++++++++++---- test/ng/directive/ngModelSpec.js | 18 +++- 4 files changed, 244 insertions(+), 40 deletions(-) diff --git a/src/ng/directive/form.js b/src/ng/directive/form.js index 6c3c2520841d..b527dca3f2ee 100644 --- a/src/ng/directive/form.js +++ b/src/ng/directive/form.js @@ -5,6 +5,8 @@ var nullFormCtrl = { $addControl: noop, $$renameControl: nullFormRenameControl, + $$increasePristine: noop, + $$decreasePristine: noop, $$updatePristine: noop, $removeControl: noop, $setValidity: noop, @@ -77,6 +79,7 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { form.$invalid = false; form.$submitted = false; form.$$parentForm = nullFormCtrl; + form.$$pristineChildsCounter = 0; /** * @ngdoc method @@ -144,6 +147,10 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { } control.$$parentForm = form; + // if form was pristine before - it will be after too + if (control.$pristine) { + form.$$pristineChildsCounter++; + } }; // Private API: rename a form control @@ -189,6 +196,10 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { arrayRemove(controls, control); control.$$parentForm = nullFormCtrl; + + if (control.$pristine) { + form.$$pristineChildsCounter--; + } }; @@ -239,6 +250,12 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { * state (ng-dirty class). This method will also propagate to parent forms. */ form.$setDirty = function() { + // see $setPristineBubbling + if (form.$pristine) { + form.$$parentForm.$$decreasePristine(); + } else { + form.$$parentForm.$$updatePristine(); + } $animate.removeClass(element, PRISTINE_CLASS); $animate.addClass(element, DIRTY_CLASS); form.$dirty = true; @@ -261,39 +278,67 @@ function FormController(element, attrs, $scope, $animate, $interpolate) { * saving or resetting it. */ form.$setPristine = function() { - form.$$setPristineSelf(); + form.$$setPristineBubbling(); forEach(controls, function(control) { - control.$setPristine(); + // since we force pristine state, we don't want nested controls to + // change it + control.$$setPristineCapturing(); }); }; // Private API: Sets the form to its pristine state. - // This method does not affect nested controls. + // This method does not affect nested/parent controls. form.$$setPristineSelf = function() { $animate.setClass(element, PRISTINE_CLASS, DIRTY_CLASS + ' ' + SUBMITTED_CLASS); form.$dirty = false; form.$pristine = true; form.$submitted = false; - form.$$parentForm.$$updatePristine(); }; - // Private API: update form pristine-ness - form.$$updatePristine = function() { - var isPristine = true, - controlsLength = controls.length, - i; - - for (i = 0; i < controlsLength; i++) { - if (!controls[i].$pristine) { - isPristine = false; - break; - } + // Private API: Sets the form to its pristine state. + // Propagates pristine-ness to parent form + form.$$setPristineBubbling = function() { + // propagate only if pristine state was actually changed + if (form.$dirty) { + form.$$parentForm.$$increasePristine(); + } else { + // otherwise tell aprent form to calculate current value, + // since it can be changed after adding/removing nested controls. + // The same applies to $setDirty. + form.$$parentForm.$$updatePristine(); } + form.$$setPristineSelf(); + }; - if (isPristine) { - // All the nested controls are already pristine. - // Set pristine-ness only for the form itself. - form.$$setPristineSelf(); + // Private API: Sets the form to its pristine state. + // Propagates pristine-ness to the nested controls + form.$$setPristineCapturing = function() { + form.$$setPristineSelf(); + forEach(controls, function(control) { + control.$$setPristineCapturing(); + }); + }; + + // Pivate API: nested control become pristine + form.$$increasePristine = function() { + form.$$pristineChildsCounter++; + form.$$updatePristine(); + }; + + // Pivate API: nested control become dirty + form.$$decreasePristine = function() { + form.$$pristineChildsCounter--; + form.$$updatePristine(); + }; + + // Private API: update form pristine-ness + form.$$updatePristine = function() { + if (form.$$pristineChildsCounter === controls.length) { + // since we got update from nested controls, we don't want to + // propagate it to them + form.$$setPristineBubbling(); + } else { + form.$setDirty(); } }; diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index d5fb671f7289..9aa169fe0ce9 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -379,11 +379,25 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ * has not been changed from when first compiled. */ this.$setPristine = function() { + // propagate only if pristine state was actually changed + if (ctrl.$dirty) { + ctrl.$$parentForm.$$increasePristine(); + } else { + // otherwise tell aprent form to calculate current value, + // since it can be changed after adding/removing nested controls. + // The same applies to $setDirty. + ctrl.$$parentForm.$$updatePristine(); + } + ctrl.$$setPristineCapturing(); + }; + + // Private API: Sets the control to its pristine state. + // This method does not affect parent form + this.$$setPristineCapturing = function() { ctrl.$dirty = false; ctrl.$pristine = true; $animate.removeClass($element, DIRTY_CLASS); $animate.addClass($element, PRISTINE_CLASS); - ctrl.$$parentForm.$$updatePristine(); }; /** @@ -398,6 +412,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$ * from when first compiled. */ this.$setDirty = function() { + // see $setPristine + if (ctrl.$pristine) { + ctrl.$$parentForm.$$decreasePristine(); + } else { + ctrl.$$parentForm.$$updatePristine(); + } ctrl.$dirty = true; ctrl.$pristine = false; $animate.removeClass($element, PRISTINE_CLASS); diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 4f6e94e7c008..27d0c14c1603 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -714,10 +714,7 @@ describe('form', function() { expect(form.$error.maxlength[0].$name).toBe('childform'); inputController.$setPristine(); - // this assertion prevents to propagate prestine to the parent form - // expect(form.$dirty).toBe(true); - - form.$setPristine(); + expect(form.$dirty).toBe(false); // remove child form form.$removeControl(childformController); @@ -1045,6 +1042,39 @@ describe('form', function() { expect(nestedInputCtrl.$dirty).toBe(false); }); + + it('should be idempotent on freshly created form', function() { + doc = $compile( + '
' + + '' + + '
')(scope); + + var form = doc, + inputCtrl = doc.find('input').eq(0).controller('ngModel'); + + expect(form).toBePristine(); + + // many times + inputCtrl.$setPristine(); + inputCtrl.$setPristine(); + scope.$apply(); + expect(form).toBePristine(); + + inputCtrl.$setDirty(); + scope.$apply(); + expect(form).toBeDirty(); + + // many times + inputCtrl.$setDirty(); + inputCtrl.$setDirty(); + scope.$apply(); + expect(form).toBeDirty(); + + inputCtrl.$setPristine(); + scope.$apply(); + expect(form).toBePristine(); + }); + it('should propagate pristine-ness to the parent form', function() { doc = $compile( '
' + @@ -1057,7 +1087,7 @@ describe('form', function() { childFormCtrl.$setDirty(); scope.$apply(); - expect(parentForm).not.toBePristine(); + expect(parentForm).toBeDirty(); childFormCtrl.$setPristine(); scope.$apply(); @@ -1068,55 +1098,150 @@ describe('form', function() { it('should be pristine if all the nested controls are pristine', function() { doc = $compile( '' + - '' + - '' + + '
' + + '' + + '' + + '
' + '
')(scope); var form = doc, - formCtrl = scope.form, + childForm = form.find('div').eq(0), input1 = form.find('input').eq(0), input2 = form.find('input').eq(1), inputCtrl1 = input1.controller('ngModel'), inputCtrl2 = input2.controller('ngModel'); inputCtrl1.$setDirty(); + inputCtrl1.$setDirty(); + scope.$apply(); + expect(form).toBeDirty(); + expect(childForm).toBeDirty(); + + inputCtrl2.$setDirty(); inputCtrl2.$setDirty(); scope.$apply(); - expect(form).not.toBePristine(); + expect(form).toBeDirty(); + expect(childForm).toBeDirty(); inputCtrl1.$setPristine(); scope.$apply(); - expect(form).not.toBePristine(); + expect(form).toBeDirty(); + expect(childForm).toBeDirty(); inputCtrl2.$setPristine(); scope.$apply(); expect(form).toBePristine(); + expect(childForm).toBePristine(); }); it('should be pristine if all the nested forms are pristine', function() { doc = $compile( - '
' + - '
' + - '
' + + '' + + '
' + + '
' + + '
' + + '
' + '
')(scope); - var form = doc, - formCtrl = scope.form, + var outerForm1 = doc, + outerForm2 = doc.find('div').eq(0), childFormCtrl1 = scope.childForm1, childFormCtrl2 = scope.childForm2; childFormCtrl1.$setDirty(); + scope.$apply(); + expect(outerForm1).toBeDirty(); + expect(outerForm2).toBeDirty(); childFormCtrl2.$setDirty(); scope.$apply(); - expect(form).not.toBePristine(); + expect(outerForm1).toBeDirty(); + expect(outerForm2).toBeDirty(); childFormCtrl1.$setPristine(); scope.$apply(); - expect(form).not.toBePristine(); + expect(outerForm1).toBeDirty(); + expect(outerForm2).toBeDirty(); childFormCtrl2.$setPristine(); scope.$apply(); - expect(form).toBePristine(); + expect(outerForm1).toBePristine(); + expect(outerForm2).toBePristine(); + }); + + it('should properly handle added/removed controls and be idempotent', function() { + + var test = function(input, inputCtrl) { + doc = $compile( + '
' + + '
' + + '
')(scope); + + var outerForm = doc, + innerForm = doc.find('div').eq(0), + innerFormCtrl = innerForm.controller('form'); + + inputCtrl.$setDirty(); + inputCtrl.$setDirty(); + + // just add control does not change form pristine-ness + innerFormCtrl.$addControl(inputCtrl); + scope.$apply(); + expect(innerForm).toBePristine(); + expect(outerForm).toBePristine(); + + // change after adding + inputCtrl.$setDirty(); + inputCtrl.$setDirty(); + scope.$apply(); + expect(innerForm).toBeDirty(); + + innerFormCtrl.$removeControl(inputCtrl); + + // removed control does not affect + inputCtrl.$setPristine(); + scope.$apply(); + expect(innerForm).toBeDirty(); + expect(outerForm).toBeDirty(); + + innerFormCtrl.$addControl(inputCtrl); + scope.$apply(); + expect(innerForm).toBeDirty(); + expect(outerForm).toBeDirty(); + + // single $setPristine after multiple $setDirty + inputCtrl.$setPristine(); + scope.$apply(); + expect(innerForm).toBePristine(); + expect(outerForm).toBePristine(); + + // many times + innerFormCtrl.$removeControl(inputCtrl); + inputCtrl.$setPristine(); + inputCtrl.$setPristine(); + innerFormCtrl.$addControl(inputCtrl); + scope.$apply(); + expect(innerForm).toBePristine(); + expect(outerForm).toBePristine(); + + // single setDirty afterm multiple setPristine + inputCtrl.$setDirty(); + scope.$apply(); + expect(innerForm).toBeDirty(); + expect(outerForm).toBeDirty(); + }; + + var input1 = $compile('')(scope), + inputCtrl1 = input1.controller('ngModel'), + + input2 = $compile('
')(scope), + inputCtrl2 = input2.controller('form'); + + // test for input + test(input1, inputCtrl1); + dealoc(doc); + + // test for ng-form + test(input2, inputCtrl2); }); }); diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 56d1476a905b..7a9788967449 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -16,6 +16,8 @@ describe('ngModel', function() { $setValidity: jasmine.createSpy('$setValidity'), $setDirty: jasmine.createSpy('$setDirty'), $$clearControlValidity: noop, + $$increasePristine: jasmine.createSpy('$$increasePristine'), + $$decreasePristine: jasmine.createSpy('$$decreasePristine'), $$updatePristine: jasmine.createSpy('$$updatePristine') }; @@ -147,9 +149,21 @@ describe('ngModel', function() { expect(ctrl.$pristine).toBe(true); }); - it('should propagate pristine to the parent form', function() { + it('should propagate pristine to the parent form conditionally', function() { ctrl.$setPristine(); - expect(parentFormCtrl.$$updatePristine).toHaveBeenCalledOnce(); + ctrl.$setPristine(); + expect(parentFormCtrl.$$increasePristine).not.toHaveBeenCalled(); + + ctrl.$setDirty(); + expect(parentFormCtrl.$$decreasePristine).toHaveBeenCalledOnce(); + + parentFormCtrl.$$decreasePristine.reset(); + ctrl.$setDirty(); + ctrl.$setDirty(); + expect(parentFormCtrl.$$decreasePristine).not.toHaveBeenCalled(); + + ctrl.$setPristine(); + expect(parentFormCtrl.$$increasePristine).toHaveBeenCalledOnce(); }); });