Different behavior for required and ng-required for initial values

[adharris]

I'm noticing different behavior when using required vs ng-required when combined with ng-change. When the initial model value is any empty value besides undefined (i.e. null or ""), ng-changed is fired when using ng-required="true", but not when using the basic required.

It also seems that this behavior has changed from 1.3.0 rc0 and rc1. In rc0, the change handler is never called.

The behavior for ng-required in rc1 makes sense to me; validation is run for the initial value, null and "" do not pass validation so the model value is set to undefined, and sense undefined does not equal the initial value ng-change is evaluated. But shouldn't the two variants behave the same?

Here is a plunker demonstrating the differences: http://plnkr.co/edit/e1J1VnUSpR8xVhdKIXVY?p=preview

I'm not sure, but this might be related to #8949 and/or #9001

[Narretz]

This is probably because ngRequired observes the required attr and fires the validation a second time.
Actually, I don't think the behavior of ngRequired in rc.1 is correct. The validation calls the parsers, which detect an undefined viewValue and therefore changes the model value to undefined (without calling any parsers), which triggers ngChange. But as @caitp pointed out in #9044 (comment) angular should not actually parse empty input values.

[caitp]

to expand on that, I don't think we should parse any values unless setViewValue was called, which should not happen when calling $validate() in isolation (imo)

[Narretz]

I agree. At a minimum, running parsers should be extracted from $validate. But I haven't checked how tight the coupling overall still is.

[shahata]

For the record, re-parsing on $validate was introduced in tbosch@1ec8bde
The goal was to get rid of $$invalidModelValue. If we do not want to re-parse, it means we need to add $parsedModelValue or something similar as discussed in #9016 (comment)

[Narretz]

This "fails" even with 92f05e5 (don't parse if viewValue is undefined)
This is because there's an unnecessary validation after the model is set to the view for the first time:

When the ngModelController is created, it checks the model via $watch, formats it, sets its viewValue, and calls $$runValidators. This is okay.
The problem is that all directives / validators that use an observer call $validate very often, too often I'd say.
-> before the ngModelCtrl is initialized: these are ignored because the $modelValue isn't set yet => Okay
-> after the modelWatch is called: these go through, and since the modelValue is invalid, they set the model to undefined. Since this observer comes directly after the first model -> value, it is unnecessary, since the viewValue / modelValue cannot have changed yet.

One solution is to not call $validate inside observers when the attr value hasn't changed. It's actually possible to that $validate is called far more often that it needs to be. The problem here is that this is needed for all observers, so everyone who writes a validator needs to be aware of this boilerplate.

[IgorMinar]

@tbosch can we have a quick chat about this some time today?

[caitp]

@Narretz yes, I've thought about this, unfortunately this is hard.

I think we do want to run the validators on demand not sure we can defer validation the same way we defer adding/removing classes) --- however, we don't want to incorrectly report errors that aren't really errors. I'm not sure how (if?) we're still seeing incorrect parse errors (you seem to be saying we are), it would be helpful if you can provide a repro so that it's easier to investigate

[Narretz]

@caitp This specific issue is actually not about parse errors, I think your fix got those (at least those I know).
This is about inputs that a have an invalid initial value & use a validator that is called by observe. In this plunker, I have put some console.log inside the angular.js file:

http://plnkr.co/edit/NItvaqntVXGPlgYvFbNI?p=preview

1. observe inside the directive is called, but no parse / validation is called because the modelCtrl isn't ready yet.
2. the modelCtrl detects a change in the watch and after setting the view calls $$runValidators (which let's us keep initially invalid models)
3. the observer is fired again, this time it calls parseAndValidate. Because the value is invalid, the model is changed and therefore the viewChangeListeners are called.

[btford]

@IgorMinar @tbosch what was the result of this conversation?

[tbosch]

@btford All our points are already mentioned in the previous commnents.

[Narretz]

What I don't understand (and previously didn't pay any attention to) is that required only fires the observer once, but ngRequired twice. I can for the love of it not understand why that is.

e: it's not the cause of the issue, but it's interesting nevertheless.
The actual cause lies somewhere here, 9ad7d74, probably something about the removal of $$invalidModelValue

[onehourlate]

Just to add my two cents : this change even breaks the most basic angular-only behavior with checkbox input types.

If an input checkbox has an initial model of undefined or null, and it has both ng-required and ng-change, the ng-change callback will fire on page load, which is definitely not supposed to happen.

cf http://plnkr.co/edit/5gqfJvtxwHpvSHoPDBfA?p=preview (the $scope.counter equals to 1 after page load, when it should be 0)

[Narretz]

@btford I'm taking this over, this is part of a bigger problem with ngModel

[Narretz]

This is fixed in 1.3.4 :)