when ng-maxlength is invalid, 'require' validation is also invalid

[homanchou]

On a pristine form, I have an input field

<form name="myForm">
    <input name="username" ng-model="user.name" required ng-maxlength="5">
</form>

Given that user.name was pre-populated with a string exceeding 5 characters, in angular 1.1.5 the modelCtrl errors were:

form.username.$error = {"required":false,"maxlength":true}

That is expected because only the maxlength is violated, so 'required' was not invalid.

But after angular > 1.2.0 this behavior is now invalid for both:

form.username.$error = {"required":true,"maxlength":true}

This is a bit unexpected. Why should 'required' have an error when the model has some value... unless we're now saying that the value being required must be valid across all other validation requirements in total?

You can test it here by swapping out the angular version:
http://plnkr.co/edit/uUaC9tgHEozBkUReRXBk?p=preview

[caitp]

I haven't been able to sleep, so I've been trying to find the commit which broke this:

../input_bisect_spec.js:

'use strict';

describe('#6304', function() {
  var element;

  afterEach(function() {
    dealoc(element);
  });

  iit('should not set ngRequired error', inject(function($compile, $rootScope) {
    $rootScope.user = {
      name: "a very long value"
    };
    element = $compile('<form name="form"><input type="text" required ng-model="user.data" name="uname" ng-maxlength="5" /></form>')($rootScope);
    $rootScope.$digest();

    expect($rootScope.form.uname.$error.required).toBeFalsy();
  }));
});

../test.sh:

rm -rf node_modules
rm -rf bower_components
rm -rf components
npm install
grunt package
mv test/ng/directive/inputSpec.js test/ng/directive/inputSpec_.js
cp ../input_bisect_spec.js test/ng/directive/inputSpec.js
grunt test:jqlite
code=$?
mv test/ng/directive/inputSpec_.js test/ng/directive/inputSpec.js
exit $code

Bisecting with:

git bisect start v1.2.13 v1.1.4
git bisect run ../test.sh

But interestingly, the unit test fails ($error.required === true) for every tried commit, including the 1.1.5 commits, even though this isn't the behaviour seen in the repro.

I'm probably doing something wrong that isn't obvious while exhausted, so maybe I'll do better tomorrow. If you want to have a go at finding the CL responsible for the change in behaviour, that would be awesome.

[tbosch]

You mean with angular 1.1.5 you have "required":false, right?

If you clear the input and enter the 6 characters yourself, you will get required:false, but not on the initial load. So this really seems to be a bug!

[caitp]

@tbosch it does, but I can't seem to get this to happen in a unit test. It's a bit curious. Do you see anything wrong in the test I wrote up there?

[Narretz]

It's probably caused by 31f190d or one of the other compilation fixes (maybe 4357da8)

In 1.2.0-rc.2, the link functions run input -> required, in 1.2.0-rc.3 they run required -> input

[tbosch]

@caitp Sorry for the late answer. The html to be compiled contains ng-model="user.data" but you use user.name in the JS code. Should be ng-model="user.name".

Here is the corrected test case that also includes the manual user interaction:

'use strict';

describe('#6304', function() {
  var element;

  afterEach(function() {
    dealoc(element);
  });

  iit('should not set ngRequired error', inject(function($compile, $rootScope) {
    $rootScope.user = {
      name: "a very long value"
    };
    element = $compile('<form name="form"><input type="text" required ng-model="user.name" name="uname" ng-maxlength="5" /></form>')($rootScope);
    $rootScope.$digest();

    expect($rootScope.form.uname.$error.required).toBeFalsy();

    // simulate user interaction
    var input = element.children();
    input.val('another very long value');
    browserTrigger(input, 'change');

    expect($rootScope.form.uname.$error.required).toBeFalsy();
  }));
});

[tbosch]

While debugging this, I know why the problem exists:

1. Initially the <input> is empty
2. When setting a value that is too long in the scope Angular creates a validation error and does not set the value into the <input>

By this, the value is still not present in the real DOM element, and therefore Angular says that it fails the required validation. So the problem lies in those 3 lines: https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L1423-L1425
In there, the required validator is called always with the view value, which is empty initially as the max length validation did not pass.

[caitp]

haha, well it was like 4am when I wrote that stuff, that's my excuse. Good catch @tbosch

[tbosch]

Forget my last comment, the real problem is in this line: https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L435

There the max length validator returns undefined if it's not valid. As the required validator is called later on it thinks that the value is empty and therefore reports a validation error on it's own.

However, the validators just work according to our docs! They say if a validator fails it should return undefined. Someone from angular-ui complained about this before, don't remember the issue (we closed it as invalid I think).

[Narretz]

@tbosch The actual error was introduced in 1.2.0-rc.3. My money is still on postLink priority run change.
http://plnkr.co/edit/AMAW5vmRU28ulMeDon6p?p=preview

That you cannot collect all validation errors from an input because the first that is invalid returns undefined is a bigger problem that should probably be addressed.

[homanchou]

@tbosch Yes, the validations are inconsistent based on whether the input was initialized with existing data vs if you type into the input manually. Along the same vein, the view value of the input is inconsistent based on this as well. In my example if I have ng-maxlength=5, and existing model value of string length > 5, the input field simply appears empty (this is incredibly misleading and unexpected that on a pristine form, we can't show the offending long string for the user to fix it). Why should a prepopulated input be invisible vs when manually typing into the input, when I exceed the 5 character limit, it's not like the input string suddenly disappears on me on the 6th character (at least not in the input, it does stop binding to the scope). If this is caused by validators returning undefined then I agree this is a problem that needs to be addressed. Personally I think it would be much nicer if the data that's presented in the inputs, whether typed or initialized by ajax, is exactly what is bound to the model: what you see is what you get, and never becomes undefined. To me that follows the principle of least surprise.

[homanchou]

Here is another oddity.

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

If I have <input type="number"> but my model is a string like { age: '23' }, on form load the validation correctly detects that '23' is not a number:

form.age.$error = {"number":true}

If I then type in a number 23, as expected, I get:

form.age.$error = {"number":false}

However if I then clear the input and type a string again, 'abc', the number validation doesn't appear to kick back in but stays as:

form.age.$error = {"number":false}

[matsko]

@homanchou this PR should fix that: #7377