fix(input): improve html5 validation support #7937

caitp · 2014-06-22T19:40:12Z

This CL improves mocking support for HTML5 validation, and ensures that it
works correctly along with debounced commission of view values.

Closes #7936

caitp · 2014-06-22T19:44:40Z

src/ng/directive/input.js

    var viewValue = ctrl.$viewValue;

    $timeout.cancel(pendingDebounce);
-    if (ctrl.$$lastCommittedViewValue === viewValue) {
+    if (!revalidate && ctrl.$$lastCommittedViewValue === viewValue) {


CC @petebacondarwin / @shahata --- It's possible for the old value and new value to be the same here, because HTML5 will report controlls suffering from bad input's value as the empty string --- but we should revalidate anyways just so the native HTML validators do their business.

However, it may not be desirable to update the "last committed value" in that case, what do you think?

What does it mean for the native validators to do their business?
Is the question, should we set the last committed value to the empty string if the HTML5 validation fails?

We occasionally need to revalidate, even if the actual value hasn't changed. But I guess by that point, the last committed value is probably the empty string anyways. So maybe it's not worth worrying about

We should probably have some unit tests that demonstrate the various scenarios - at least for posterity.

If I understand correctly, this is important only in case the html5 validators report a different validity error the second time this is invoked and we want to update ctrl.$error with the latest validation problem. If this is the case, maybe we should consider removing the revalidate parameter and compare the validity state here with the previous committed one. I don't like it very much, but I think it is a bit better then passing revalidate all this way. Another option is to run the validators no matter what in case the string is empty.

Actually, passing the revalidate is a problem any way, since it will not work in case ng-model-options="{updateOn: 'blur'}". In this case, this function will be invoked by the blur trigger handler, which will not pass the revalidate argument...

It's not every time, only when prevValue==newValue && prevValue=='' && (prevValidity.badInput || newValidity.badInput) or something like this. This made me realize that there is also the case where a value was invalid and now it is just empty - this case should also be revalidated and currently it is not covered imo. Anyway, even if you don't diff ValidityState, what do you think about the second point I mentioned? The revalidate thing will not work correctly in case of updateOn: 'blur'...

no, the ngModelOptions stuff added a ton of complexity to this machinery that didn't exist previously, and there were no tests against it breaking the html5 validation stuff because it's not really possible to test without without the hacks here (those hacks probably should have existed initially, but hey).

So what we need to do is figure out a way for them to work well together in tandum.

Computing a diff between ValidityState objects is problematic, because A) ValidityState keys are not always enumerable (Firefox) which makes a diffing algorithm difficult and makes us have to keep track of possible ValidityState flags which are not necessarily never changing, maintenance hell, and B) it's just a stupid amount of work that isn't really needed anywhere.

You're right that this is broken in the case of invalid -> empty, so that's fixable, but the gunk with ngModelOptions is a lot harder because that piece of machinery is overly complex and not built with constraint validation in mind, which is why you guys are CC'd here

I agree diffing the ValidityState is not pretty/cheap and I really don't like it too, I'm just trying to think of a way to do this that will work in all cases. What about the option to run all validators anyway in case the string is empty?

I'm fine with that, at least in the case where we're using html5 validators

petebacondarwin · 2014-06-22T20:38:25Z

src/ng/directive/input.js

@@ -1093,7 +1107,7 @@ function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
    }
  });

-  addNativeHtml5Validators(ctrl, 'number', element);
+  addNativeHtml5Validators(ctrl, 'number', numberBadFlags, null, ctrl.$$validityState);


The ignoreFlags parameter doesn't seem to be used at all. If we set it to [] here rather than null then we could remove the if(flags) line in the testFlags() function above.

It's needed for other types, eventually we will want to do this for required validation too (at the bare minimum), and we don't want to invalidate required if we have badInput

(This is because of the really, really stupid constraint validation API, it makes zero sense, but it's what we've got =()

But if we insist that these parameters are always arrays then we don't need the check right? Is there anywhere that a null is going to be passed where we can't insist that it is an empty array instead?

The question is whether you prefer testing if flags is null, or if you prefer creating an extra global object --- my preference is on the former here

This CL improves mocking support for HTML5 validation, and ensures that it works correctly along with debounced commission of view values.

petebacondarwin · 2014-06-24T11:36:23Z

LGTM

caitp · 2014-06-24T12:34:21Z

alright, checking this in today, since it's pretty broken in 1.2.x too --- if it needs to be cleaned up I think that can be done before the release, but this should be ok

This CL improves mocking support for HTML5 validation, fixes the behaviour which invokes validators. Previously, an input would only be revalidated if either its value changed, or if it was the empty string but did not suffer from bad input --- now, it will be revalidated if either the value has changed, or the value is the empty string, there is a ValidityState for the element, and that ValidityState is being tested by one of the validators in the pipeline. Closes #7937 Closes #7957

shahata · 2014-06-24T17:06:55Z

@caitp I thought we agreed on changing the revalidate thing since it will not work for {updateOn: 'blur'}. don't you think this should be resolved? or am I missing something?

caitp · 2014-06-24T17:09:05Z

nobody understands how updateOn works other than the two of you, so figure something out to make that work. ngModelOptions should have never shipped without working with this if there had been tests in place before --- now there are tests, now we know we can make it work.

shahata · 2014-06-24T17:11:07Z

okay, so would you like me to send a PR that fixes this?

caitp · 2014-06-24T17:12:22Z

yes, along with the e2e tests you wrote already =)

shahata · 2014-06-24T17:12:33Z

np

shahata · 2014-06-24T22:47:15Z

@caitp I've submitted the fix we discussed to html5 validation with ngModelOptions: #7976
Too tired currently to add the e2e tests, will probably add it to the the PR or a new PR sometime tomorrow...

This CL improves mocking support for HTML5 validation, and ensures that it works correctly along with debounced commission of view values. Closes angular#7936 Closes angular#7937

caitp reviewed Jun 22, 2014
View reviewed changes

caitp added cla: yes and removed cla: no labels Jun 22, 2014

petebacondarwin reviewed Jun 22, 2014
View reviewed changes

fix(input): improve html5 validation support

34f0cbf

This CL improves mocking support for HTML5 validation, and ensures that it works correctly along with debounced commission of view values.

caitp added component: forms labels Jun 23, 2014

caitp added this to the 1.3.0-beta.14 milestone Jun 23, 2014

caitp closed this in 1f6a5a1 Jun 24, 2014

theurere mentioned this pull request Aug 26, 2014

Update angular to 1.2.23 strukturag/spreed-webrtc#98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): improve html5 validation support #7937

fix(input): improve html5 validation support #7937

caitp commented Jun 22, 2014

caitp Jun 22, 2014

petebacondarwin Jun 22, 2014

caitp Jun 22, 2014

petebacondarwin Jun 22, 2014

shahata Jun 22, 2014

shahata Jun 22, 2014

caitp Jun 22, 2014

shahata Jun 22, 2014

caitp Jun 22, 2014

shahata Jun 22, 2014

petebacondarwin Jun 22, 2014

caitp Jun 22, 2014

caitp Jun 22, 2014

petebacondarwin Jun 22, 2014

caitp Jun 22, 2014

petebacondarwin commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

shahata commented Jun 24, 2014

fix(input): improve html5 validation support #7937

fix(input): improve html5 validation support #7937

Conversation

caitp commented Jun 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

caitp commented Jun 24, 2014

shahata commented Jun 24, 2014

shahata commented Jun 24, 2014