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

fix(ngModelController): always use last commited view value in validators #10299

Closed
wants to merge 1 commit into from

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Dec 2, 2014

...ed

This fixes issues where a parser calls $setViewValue. This is a common
strategy for manipulating the $viewValue while the user is entering
data into an input field.

When the $viewValue was changed inside the parser, the new viewValue
would be committed and used for validation. The original parser
however would run last and pass the original (outdated) viewValue on
to the validators, which could cause false positives, e.g. for
minlength.

This is a regression since 1.3.0-rc.1, caused by 6046e14#diff-c244afd8def7f268b16ee91a0341c4b2L2055

@googlebot
Copy link

CLAs look good, thanks!

@Narretz
Copy link
Contributor Author

Narretz commented Dec 2, 2014

@shahata Could you take a look at this?

@Narretz Narretz changed the title fix(ngModelController): don't run $validators if modelValue hasn't chang... (wip) fix(ngModelController): don't run $validators if modelValue hasn't chang... Dec 2, 2014
@Narretz Narretz force-pushed the fix-ngModel-parse-view branch from c541938 to ac4e97b Compare December 2, 2014 22:49
@Narretz Narretz changed the title (wip) fix(ngModelController): don't run $validators if modelValue hasn't chang... fix(ngModelController): don't run $validators if modelValue hasn't chang... Dec 2, 2014
@shahata
Copy link
Contributor

shahata commented Dec 3, 2014

@Narretz I'm not sure about this. Validators get both modelValue and viewValue as parameters, so I could argue that even if modelValue didn't change, it should still be called again since the viewValue is not the same.

Also, I'm not sure I understand why a parser would call $setViewValue. I would do something like ctrl.$viewValue = value; ctrl.$render();. Can you show an example of where this behavior is causing trouble?

@Narretz
Copy link
Contributor Author

Narretz commented Dec 3, 2014

One problem is in this issue: #10126
Enter a, in the input. , gets stripped out, but the minlength validator says true which is not correct.

The minlength validator will validate with a viewValue that is out of date, because parser -> (setViewValue -> parser -> validate) -> validate and because $$parseAndValidate caches the $$lastCommittedViewValue.

Setting $viewValue directly does not fix this.

@shahata
Copy link
Contributor

shahata commented Dec 4, 2014

Okay, I get it now, but let's say I have a parser that just removes the last , (without touching $viewValue and $setViewValue) and I also have maxlength="1" - wouldn't your implementation do the wrong thing when typing a and then adding a , (since a and a, have the same model value, validators will not run and won't detect that maxlength is now invalid)

I think that simply passing ctrl.$$lastCommittedViewValue instead of viewValue to $$runValidators will solve the issue in #10126 without causing the problem I described. WDYT?

@Narretz
Copy link
Contributor Author

Narretz commented Dec 4, 2014

You are absolutely right, this breaks validation if a parser only truncates the model value. I even had the last committed view value solution, but wanted to skip the extra validation. I'll fix this up.

@Narretz Narretz force-pushed the fix-ngModel-parse-view branch 2 times, most recently from b7eef7d to 5542010 Compare December 5, 2014 09:33
Narretz added a commit to Narretz/angular.js that referenced this pull request Dec 5, 2014
…dation

This fixes issues where a parser calls $setViewValue. This is a common
strategy for manipulating the $viewValue while the user is entering
data into an input field.

When the $viewValue was changed inside the parser, the new viewValue
would be committed, parsed and used for validation. The original parser
however would run after that and pass the original (outdated) viewValue
on to the validators, which could cause false positives, e.g. for
minlength.

Fixes angular#10126
Fixes angular#10299
@Narretz Narretz changed the title fix(ngModelController): don't run $validators if modelValue hasn't chang... fix(ngModelController): always use last commited view value in validators Dec 5, 2014
@Narretz
Copy link
Contributor Author

Narretz commented Dec 5, 2014

I updated the PR with your suggestion

@shahata
Copy link
Contributor

shahata commented Dec 5, 2014

LGTM

…dation

This fixes issues where a parser calls $setViewValue. This is a common
strategy for manipulating the $viewValue while the user is entering
data into an input field.

When the $viewValue was changed inside the parser, the new viewValue
would be committed, parsed and used for validation. The original parser
however would run after that and pass the original (outdated) viewValue
on to the validators, which could cause false positives, e.g. for
minlength.

Fixes angular#10126
Fixes angular#10299
@Narretz Narretz force-pushed the fix-ngModel-parse-view branch from 5542010 to c4ff166 Compare December 5, 2014 13:31
@Narretz Narretz closed this in 2d6a0a1 Dec 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants