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

input[type=date] borks model on binding if ngRequired is falsey #9996

Closed
EverPresent opened this issue Nov 11, 2014 · 14 comments · Fixed by #10048
Closed

input[type=date] borks model on binding if ngRequired is falsey #9996

EverPresent opened this issue Nov 11, 2014 · 14 comments · Fixed by #10048

Comments

@EverPresent
Copy link

So this wasn't a problem in the previous version of angular that I was running (1.2.*?) but as soon as I upgraded to 1.3.1 all of my required date input types would set my model value to null upon binding (which would fire my $watches prematurely and cause havok).

It seems there have been a lot of input controller related issues arise in 1.3 but looking through the open issues I don't think this one has any related.

Check out this plunk to verify the issue:
http://plnkr.co/edit/dgsiLXD4OTDt19FF6pmi?p=info

I tracked down the problem to the default formatter that is attached to the input in input.js:createDateInputType. It is returning an empty string if the value evaluates to isEmpty. Looking at other formatters the rule of thumb seems to be to return the original value that evaluated to falsey (not change it to empty string). I made this one line change and if you click the "see fixed version" at the bottom of the plunk you will see the exact same page using my fixed version of the angular.js file.

@Narretz
Copy link
Contributor

Narretz commented Nov 11, 2014

I think the reason for returning empty string is to have equality between $viewValue and actual DOM value. IIRC browsers that support date won't set invalid date strings as values - though that could be the case for throwing actually.

@frfancha
Copy link

Set $viewValue to '' to match DOM, why not, but then why changing the model?

@Narretz
Copy link
Contributor

Narretz commented Nov 11, 2014

"changing the model" is the bug

----- Ursprüngliche Nachricht -----
Von: "frfancha" notifications@github.com
Gesendet: ‎11.‎11.‎2014 19:46
An: "angular/angular.js" angular.js@noreply.github.com
Cc: "Narretz" mjstaffa@googlemail.com
Betreff: Re: [angular.js] input[type=date] borks model on binding ifngRequired is falsey (#9996)

Set $viewValue to '' to match DOM, why not, but then why changing the model?

Reply to this email directly or view it on GitHub.=

@frfancha
Copy link

Ok.

@EverPresent
Copy link
Author

Mmmm... Ok, but the formatter in the stringBasedInputType function returns the original value if the value evaluates to isEmpty (which is used by input[type] text, email, and url).

function stringBasedInputType(ctrl) {
  ctrl.$formatters.push(function(value) {
    return ctrl.$isEmpty(value) ? value : value.toString();
  });
}

As does the one in the numberInputType function (used by input[type] number).

  ctrl.$formatters.push(function(value) {
    if (!ctrl.$isEmpty(value)) {
       ... omitted for brevity ...
    }
    return value;
  });

The only other built-in formatters that I found in the angular code were for checkbox and ngList which are both kind of special cases anyway.

Is matching the DOM value something that is needed? If so, it would seem to me that these other formatters for those input types mentioned should be modified to return an empty string as well. If not, then returning the original value would seem to be an adequate fix. Thoughts?

@Narretz
Copy link
Contributor

Narretz commented Nov 12, 2014

@EverPresent this sounds like a good idea, actually. What makes me wonder though: when the model gets formatted to undefined instead of '', it should still get converted to null, because it is still empty. I'll have to test this again.

@EverPresent
Copy link
Author

I'm playing around with this and thought I would post a little more info for your consumption.

When the formatter receives undefined and formats it into an empty string. The parser immediately turns around and formats that empty string into null. The rest of the angular framework sees that the value of that field is set to null, and modifies the model accordingly.

The crux of this bug, I think, is occurring when the parser converts the empty string into a null value. The rest of the angular framework is just taking that value, and doing what it is supposed to do. If the parser always converts an empty string into null, then the angular framework will always modify the underlying model (even prior to user action as we note in the above plunk) because null is a legitimate value for a bound property. If the parser always converts an empty string into undefined then the angular framework will delete the property from the model object (not necessarily what we want to happen either).

Both null and undefined evaluate to true in isEmpty. I think the formatter must return the original value because there is a big difference between these values. If the formatter returns undefined then the parser is never called and no model-changing actions occur.

@frfancha
Copy link

When the view has been changed by the model (=formatters have run), the parsers should not be called at all, the model must stay what it was before calling the formatters and there is no point in calling the parsers.
The other way around is not true: when a model have been produced after a view change by the user (on blur or on each key stroke, or delayed, according to ngModelOptions), the model must be parsed to be nicely displayed, but onBlur only!
Type 4/4/20 in date field => model is set to April, 4th 2020, but view is still 4/4/20
The user can:
-Leave the field: only then view is "formatted" and becomes, perhaps, 2020-04-04
-OR: continue to type 14 => model is April, 4th, 2014, view is still 4/4/14. Only then user leaves the field => then view is "formatted" and becomes 2014-04-04 (or whatever date settings have been selected).

Attention: this is the description of how it should behave according to me, not at all an analysis of the current behavior of AngularJS.

Fred

@EverPresent
Copy link
Author

I'm not entirely sure what the current behavior of angular is in regards to your comments. But I think you would get better feedback from the entire body of core developers if you posted those ideas on the dev mailing list or opened a separate issue to discuss proposed changes.

Narretz added a commit to Narretz/angular.js that referenced this issue Nov 13, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Narretz added a commit to Narretz/angular.js that referenced this issue Nov 13, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Narretz added a commit to Narretz/angular.js that referenced this issue Nov 13, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
@Narretz
Copy link
Contributor

Narretz commented Nov 13, 2014

@frfancha
Let's try to separate bugs from expected behavior (whether sensible or not).

Bug:

  • Parsers running after the model is set and has been formatted

Expected:

  • Parsers run after the viewValue is commited; this can be delayed by time or only executed on certain events, such as blur.
  • Formatters only run when the model is set from the scope. They don't run after the viewValue has been commited, parsed or set as model on the scope.
  • That means there is currently no sanctioned way of "masking" user input while / after they are entering it. Most people get around that limitation by doing that inside parsers.

Narretz added a commit to Narretz/angular.js that referenced this issue Nov 15, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Narretz added a commit to Narretz/angular.js that referenced this issue Nov 17, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Closes: angular#10048
Narretz added a commit to Narretz/angular.js that referenced this issue Nov 17, 2014
Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Closes: angular#10048
@Narretz
Copy link
Contributor

Narretz commented Nov 25, 2014

This is fixed in 1.3.4.
We are currently evaluating how we can make ngModel / forms better, and this includes better support for formatting, too.

@Narretz Narretz closed this as completed Nov 25, 2014
@frfancha
Copy link

@Narretz
Thanks for the fix.

@weagle08
Copy link

This bug still seems to be happening in 1.3.8 & 1.3.10

@Narretz
Copy link
Contributor

Narretz commented Jan 23, 2015

@weagle08 I tried the plunker and it shows the same result for the latest snapshot and the fixed version that was included. If you are referring to a different error, please open a new issue. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.