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

Commit

Permalink
fix(ngModel): always format the viewValue as a string for text, url a…
Browse files Browse the repository at this point in the history
…nd email types

NgModel will format all scope-based values to string when setting the viewValue for
the associated input element. The formatting, however, only applies to input elements
that contain a text, email, url or blank input type. In the event of a null or undefined
scope or model value, the viewValue will be set to null or undefined instead of being
converted to an empty string.
  • Loading branch information
matsko committed Aug 29, 2014
1 parent 77ce5b8 commit 1eda183
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 4 deletions.
22 changes: 18 additions & 4 deletions src/ng/directive/input.js
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,18 @@ function testFlags(validity, flags) {
return false;
}

function stringBasedInputType(ctrl) {
ctrl.$formatters.push(function(value) {

This comment has been minimized.

Copy link
@pdanpdan

pdanpdan Sep 2, 2014

Shouldn't this be ctrl.$formatters.unshift, to put the cast to string as the last filter?
As it is implemented now, there is no way to bind an object ot the model and display it through a formatter

This comment has been minimized.

Copy link
@chrisirhc

chrisirhc Sep 14, 2014

Contributor

Not really, if you have a directive of higher priority, its post linking function will run after this input's linking function and you can either replace this formatter or add onto it.

return ctrl.$isEmpty(value) ? value : value.toString();

This comment has been minimized.

});
}

function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);
stringBasedInputType(ctrl);
}

function baseInputType(scope, element, attr, ctrl, $sniffer, $browser) {
var validity = element.prop(VALIDITY_STATE_PROPERTY);
var placeholder = element[0].placeholder, noevent = {};
var type = lowercase(element[0].type);
Expand Down Expand Up @@ -1050,7 +1061,7 @@ function createDateParser(regexp, mapping) {
function createDateInputType(type, regexp, parseDate, format) {
return function dynamicDateInputType(scope, element, attr, ctrl, $sniffer, $browser, $filter) {
badInputChecker(scope, element, attr, ctrl);
textInputType(scope, element, attr, ctrl, $sniffer, $browser);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);
var timezone = ctrl && ctrl.$options && ctrl.$options.timezone;

ctrl.$$parserName = type;
Expand Down Expand Up @@ -1100,7 +1111,7 @@ function badInputChecker(scope, element, attr, ctrl) {

function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
badInputChecker(scope, element, attr, ctrl);
textInputType(scope, element, attr, ctrl, $sniffer, $browser);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);

ctrl.$$parserName = 'number';
ctrl.$parsers.push(function(value) {
Expand Down Expand Up @@ -1134,7 +1145,8 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {

function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) {
badInputChecker(scope, element, attr, ctrl);
textInputType(scope, element, attr, ctrl, $sniffer, $browser);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);
stringBasedInputType(ctrl);

ctrl.$$parserName = 'url';
ctrl.$validators.url = function(modelValue, viewValue) {
Expand All @@ -1145,7 +1157,8 @@ function urlInputType(scope, element, attr, ctrl, $sniffer, $browser) {

function emailInputType(scope, element, attr, ctrl, $sniffer, $browser) {
badInputChecker(scope, element, attr, ctrl);
textInputType(scope, element, attr, ctrl, $sniffer, $browser);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);
stringBasedInputType(ctrl);

ctrl.$$parserName = 'email';
ctrl.$validators.email = function(modelValue, viewValue) {
Expand Down Expand Up @@ -2585,6 +2598,7 @@ var minlengthDirective = function() {
var ngListDirective = function() {
return {
restrict: 'A',
priority: 100,
require: 'ngModel',
link: function(scope, element, attr, ctrl) {
// We want to control whitespace trimming so we use this convoluted approach
Expand Down
60 changes: 60 additions & 0 deletions test/ng/directive/inputSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,66 @@ describe('ngModel', function() {
dealoc(element);
}));

it('should always format the viewValue as a string for a blank input type when the value is present',
inject(function($compile, $rootScope, $sniffer) {

var form = $compile('<form name="form"><input name="field" ng-model="val" /></form>')($rootScope);

$rootScope.val = 123;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe('123');

$rootScope.val = null;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe(null);

dealoc(form);
}));

it('should always format the viewValue as a string for a `text` input type when the value is present',
inject(function($compile, $rootScope, $sniffer) {

var form = $compile('<form name="form"><input type="text" name="field" ng-model="val" /></form>')($rootScope);
$rootScope.val = 123;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe('123');

$rootScope.val = null;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe(null);

dealoc(form);
}));

it('should always format the viewValue as a string for an `email` input type when the value is present',
inject(function($compile, $rootScope, $sniffer) {

var form = $compile('<form name="form"><input type="email" name="field" ng-model="val" /></form>')($rootScope);
$rootScope.val = 123;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe('123');

$rootScope.val = null;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe(null);

dealoc(form);
}));

it('should always format the viewValue as a string for a `url` input type when the value is present',
inject(function($compile, $rootScope, $sniffer) {

var form = $compile('<form name="form"><input type="url" name="field" ng-model="val" /></form>')($rootScope);
$rootScope.val = 123;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe('123');

$rootScope.val = null;
$rootScope.$digest();
expect($rootScope.form.field.$viewValue).toBe(null);

dealoc(form);
}));

it('should set the control touched state on "blur" event', inject(function($compile, $rootScope) {
var element = $compile('<form name="myForm">' +
Expand Down

5 comments on commit 1eda183

@wardbell
Copy link

Choose a reason for hiding this comment

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

Just want to observe that this change breaks existing directives which counted on the scope-based value being ... whatever it was. In my case it was integer. In v.1.2.0, value would be an integer (despite being bound to a textbox ... naturally); as of v.1.3.0 it has become a string. I called this out in this issue

Strongly recommend making this behavior crystal clear in the documentation. It is most unexpected to have Angular converting my scope-based value without my permission.

@matsko
Copy link
Contributor Author

@matsko matsko commented on 1eda183 Sep 23, 2014

Choose a reason for hiding this comment

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

@tbosch @caitp I won't be able to handle this anytime soon. Please have a look. Thanks @wardbell for letting us know.

@cstephe
Copy link

Choose a reason for hiding this comment

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

FYI this seems to have an impact when using replace: true in your directive. I can't seem to order my formatters.

@cstephe
Copy link

Choose a reason for hiding this comment

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

code pen showing that a replace option having issues with this change.. http://codepen.io/cstephe/pen/gPqxpa

@cstephe
Copy link

Choose a reason for hiding this comment

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

sorry nm, just realized this is the wrong commit, sorry had 4 windows open.

Also just found the area in the migration guide on how replace is being removed, so double nevermind

due to eec6394, The replace flag for defining directives that replace the element that they are on will be removed in the next major angular version. This feature has difficult semantics (e.g. how attributes are merged) and leads to more problems compared to what it solves. Also, with Web Components it is normal to have custom elements in the DOM.

Please sign in to comment.