Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(input): interpolates form input name attrs for ngModel #4791

Closed
wants to merge 1 commit into from
Closed

feat(input): interpolates form input name attrs for ngModel #4791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions src/ng/directive/form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/
var nullFormCtrl = {
$addControl: noop,
$$renameControl: nullFormRenameControl,
$removeControl: noop,
$setValidity: noop,
$$setPending: noop,
Expand All @@ -14,6 +15,10 @@ var nullFormCtrl = {
},
SUBMITTED_CLASS = 'ng-submitted';

function nullFormRenameControl(control, name) {
control.$name = name;
}

/**
* @ngdoc type
* @name form.FormController
Expand Down Expand Up @@ -51,17 +56,18 @@ SUBMITTED_CLASS = 'ng-submitted';
*
*/
//asks for $scope to fool the BC controller module
FormController.$inject = ['$element', '$attrs', '$scope', '$animate'];
function FormController(element, attrs, $scope, $animate) {
FormController.$inject = ['$element', '$attrs', '$scope', '$animate', '$interpolate'];
function FormController(element, attrs, $scope, $animate, $interpolate) {
var form = this,
parentForm = element.parent().controller('form') || nullFormCtrl,
controls = [];

var parentForm = form.$$parentForm = element.parent().controller('form') || nullFormCtrl;

// init state
form.$error = {};
form.$$success = {};
form.$pending = undefined;
form.$name = attrs.name || attrs.ngForm;
form.$name = $interpolate(attrs.name || attrs.ngForm || '')($scope);
form.$dirty = false;
form.$pristine = true;
form.$valid = true;
Expand Down Expand Up @@ -127,6 +133,17 @@ function FormController(element, attrs, $scope, $animate) {
}
};

// Private API: rename a form control
form.$$renameControl = function(control, newName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to:

var oldName = control.$name;
if (form[oldName] === control) {
  delete form[oldName];
}
form[newName] = control;
control.$name = newName;

var oldName = control.$name;

if (form[oldName] === control) {
delete form[oldName];
}
form[newName] = control;
control.$name = newName;
};

/**
* @ngdoc method
* @name form.FormController#$removeControl
Expand Down Expand Up @@ -466,13 +483,20 @@ var formDirectiveFactory = function(isNgForm) {
});
}

var parentFormCtrl = formElement.parent().controller('form'),
alias = attr.name || attr.ngForm;
var parentFormCtrl = controller.$$parentForm,
alias = controller.$name;

if (alias) {
setter(scope, alias, controller, alias);
attr.$observe(attr.name ? 'name' : 'ngForm', function(newValue) {
if (alias === newValue) return;
setter(scope, alias, undefined, alias);
alias = newValue;
setter(scope, alias, controller, alias);
parentFormCtrl.$$renameControl(controller, alias);
});
}
if (parentFormCtrl) {
if (parentFormCtrl !== nullFormCtrl) {
formElement.on('$destroy', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should invoke unobserveName() here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to since all watchers are removed when the scope is destroyed. Internally attr.$observe uses $scope.$watch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly --- the observers collection stays alive afaik. The attribute interpolation watches are removed automatically though, this is true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably better to unobserve manually rather than accidentally risk keeping a reference alive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whatever makes you guys happy :>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add unobserve here, as we don't have it at similar places in the code neither and it confuses the reader (ask myself: "why here and not at the other places"...)

parentFormCtrl.$removeControl(controller);
if (alias) {
Expand Down
12 changes: 9 additions & 3 deletions src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1657,8 +1657,8 @@ var VALID_CLASS = 'ng-valid',
*
*
*/
var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$parse', '$animate', '$timeout', '$rootScope', '$q',
function($scope, $exceptionHandler, $attr, $element, $parse, $animate, $timeout, $rootScope, $q) {
var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$parse', '$animate', '$timeout', '$rootScope', '$q', '$interpolate',
function($scope, $exceptionHandler, $attr, $element, $parse, $animate, $timeout, $rootScope, $q, $interpolate) {
this.$viewValue = Number.NaN;
this.$modelValue = Number.NaN;
this.$validators = {};
Expand All @@ -1675,7 +1675,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
this.$error = {}; // keep invalid keys here
this.$$success = {}; // keep valid keys here
this.$pending = undefined; // keep pending keys here
this.$name = $attr.name;
this.$name = $interpolate($attr.name || '', false)($scope);


var parsedNgModel = $parse($attr.ngModel),
Expand Down Expand Up @@ -2387,6 +2387,12 @@ var ngModelDirective = function() {
// notify others, especially parent forms
formCtrl.$addControl(modelCtrl);

attr.$observe('name', function(newValue) {
if (modelCtrl.$name !== newValue) {
formCtrl.$$renameControl(modelCtrl, newValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the handling when there is no parent form (similar to the handling in formController):

if (formCtrl !== nullFormCtrl) {
  formCtrl.$$renamveControl...
} else {
  this.$name = newValue;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a test for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to have the nullFormCtrl version of the function just update the name of the control. This way there is less conditional execution. Also added tests for controls/forms with no form parents

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

}
});

scope.$on('$destroy', function() {
formCtrl.$removeControl(modelCtrl);
});
Expand Down
51 changes: 51 additions & 0 deletions test/ng/directive/formSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,57 @@ describe('form', function() {
});
});


it('should rename nested form controls when interpolated name changes', function() {
scope.idA = 'A';
scope.idB = 'X';

doc = $compile(
'<form name="form">' +
'<div ng-form="nested{{idA}}">' +
'<div ng-form name="nested{{idB}}"' +
'</div>' +
'</div>' +
'</form'
)(scope);

scope.$digest();
var formA = scope.form.nestedA;
expect(formA).toBeDefined();
expect(formA.$name).toBe('nestedA');

var formX = formA.nestedX;
expect(formX).toBeDefined();
expect(formX.$name).toBe('nestedX');

scope.idA = 'B';
scope.idB = 'Y';
scope.$digest();

expect(scope.form.nestedA).toBeUndefined();
expect(scope.form.nestedB).toBe(formA);
expect(formA.nestedX).toBeUndefined();
expect(formA.nestedY).toBe(formX);
});


it('should rename forms with no parent when interpolated name changes', function() {
var element = $compile('<form name="name{{nameID}}"></form>')(scope);
var element2 = $compile('<div ng-form="name{{nameID}}"></div>')(scope);
scope.nameID = "A";
scope.$digest();
var form = element.controller('form');
var form2 = element2.controller('form');
expect(form.$name).toBe('nameA');
expect(form2.$name).toBe('nameA');

scope.nameID = "B";
scope.$digest();
expect(form.$name).toBe('nameB');
expect(form2.$name).toBe('nameB');
});


describe('$setSubmitted', function() {
beforeEach(function() {
doc = $compile(
Expand Down
36 changes: 36 additions & 0 deletions test/ng/directive/inputSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,42 @@ describe('input', function() {
}
}));


it('should interpolate input names', function() {
scope.nameID = '47';
compileInput('<input type="text" ng-model="name" name="name{{nameID}}" />');
expect(scope.form.name47.$pristine).toBeTruthy();
changeInputValueTo('caitp');
expect(scope.form.name47.$dirty).toBeTruthy();
});


it('should rename form controls in form when interpolated name changes', function() {
scope.nameID = "A";
compileInput('<input type="text" ng-model="name" name="name{{nameID}}" />');
expect(scope.form.nameA.$name).toBe('nameA');
var oldModel = scope.form.nameA;
scope.nameID = "B";
scope.$digest();
expect(scope.form.nameA).toBeUndefined();
expect(scope.form.nameB).toBe(oldModel);
expect(scope.form.nameB.$name).toBe('nameB');
});


it('should rename form controls in null form when interpolated name changes', function() {
var element = $compile('<input type="text" ng-model="name" name="name{{nameID}}" />')(scope);
scope.nameID = "A";
scope.$digest();
var model = element.controller('ngModel');
expect(model.$name).toBe('nameA');

scope.nameID = "B";
scope.$digest();
expect(model.$name).toBe('nameB');
});


describe('"change" event', function() {
function assertBrowserSupportsChangeEvent(inputEventSupported) {
// Force browser to report a lack of an 'input' event
Expand Down
27 changes: 27 additions & 0 deletions test/ng/directive/selectSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,33 @@ describe('select', function() {
});


it('should interpolate select names', function() {
scope.robots = ['c3p0', 'r2d2'];
scope.name = 'r2d2';
scope.nameID = 47;
compile('<select ng-model="name" name="name{{nameID}}">' +
'<option ng-repeat="r in robots">{{r}}</option>' +
'</select>');
expect(scope.form.name47.$pristine).toBeTruthy();
browserTrigger(element.find('option').eq(0));
expect(scope.form.name47.$dirty).toBeTruthy();
expect(scope.name).toBe('c3p0');
});


it('should rename select controls in form when interpolated name changes', function() {
scope.nameID = "A";
compile('<select ng-model="name" name="name{{nameID}}"></select>');
expect(scope.form.nameA.$name).toBe('nameA');
var oldModel = scope.form.nameA;
scope.nameID = "B";
scope.$digest();
expect(scope.form.nameA).toBeUndefined();
expect(scope.form.nameB).toBe(oldModel);
expect(scope.form.nameB.$name).toBe('nameB');
});


describe('empty option', function() {

it('should select the empty option when model is undefined', function() {
Expand Down