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

Bug when using ngModel, $asyncValidators and SELECT elements. #8929

Closed
duncangrist opened this issue Sep 4, 2014 · 20 comments
Closed

Bug when using ngModel, $asyncValidators and SELECT elements. #8929

duncangrist opened this issue Sep 4, 2014 · 20 comments

Comments

@duncangrist
Copy link

Tested and reproduced with version: 1.3.0-rc.0

If you use the relatively new $asyncValidators feature with SELECT elements bound to an ngModel the SELECT options become unselectable.

Please see the following plunk:
http://plnkr.co/edit/wlBKH2

This seems to be because $asyncValidators set the $modelValue to undefined while the async validators are running, but never set it back again to the actual value once all the async validators pass.

@gonzaloruizdevilla
Copy link
Contributor

either PR #8861 or PR #8892 will solve this

@shahata
Copy link
Contributor

shahata commented Sep 4, 2014

No, I don't think so. This is is something else. This happens because select invokes render() right after $setViewValue() which sets an empty item as selected (since $modelValue is undefined). It can be easily solved by invoking ctrl.$render() when the async validator resolves, but I'm not sure if this is the best way to solve it.

@petebacondarwin WDYT? (this is somewhat remotely related to cdc7db3f)

@petebacondarwin
Copy link
Contributor

@shahata - without looking deeply into this, calling render() when the validator resolves (i.e. we are no longer pending) may work but perhaps we could go the other way and not call render() directly after $setViewValue() if we are in a $pending state?

@gonzaloruizdevilla
Copy link
Contributor

I'm not sure this is the way to go, and it is not resolving the issue in all the cases. When you change a value in the select, the async validators are triggered three times instead of only once.

UPDATE: in the plnkr, the changes in the select triggers its own async validation, then it triggers input async validation and then the select async validation is triggered a second time. Actually, I find it very difficult to determine what the expected behavior should be when both the input and the select have async validations on the same model value.

http://plnkr.co/edit/EDuXlZopgARZ93kYSvGy?p=preview
(this plnkr is a fork of @shahata 's )

@shahata
Copy link
Contributor

shahata commented Sep 4, 2014

@petebacondarwin hmm, but if render() isn't called then I guess there would be some bad consequences. After all, it's invoked there for a reason... From what I see, the reason render() is invoked there is in order to add/remove the "?" option when it becomes selected/unselected.

Truth is I'm a bit confused by this render() function. Why does it have to use $modelValue and not $viewValue? I think just changing everything to $viewValue will solve this issue and will not break anything, but there is a good chance I'm missing something... I'm sending a PR to test this.

@duncangrist
Copy link
Author

Glad this seems to be getting traction - when/if you do get a satisfactory fix, be sure to double check that selecting 0 (zero) works as when I quickly hacked my angular copy to fix this issue, I found that another bug stopped the selection of zero.
I narrowed that bug down to NgModelController.$$setPending, resolve function - where it calls var value = ctrl.$viewValue || ''; and then uses that subsequent value when calling ctrl.$$updateValidModelValue(value);. This behaviour is causing 0 to be converted to an empty string which is then stored against the model.

Let me know if there's anything else I can do.

@shahata
Copy link
Contributor

shahata commented Sep 4, 2014

@maximumduncan you can play around with this to see if your issue is resolved: http://plnkr.co/edit/3CDLMe?p=preview
regarding the "0 becomes empty string thing", you'd better open a different issue for that, but the easy workaround is to use a string array instead of number array: ['0','1','2',...]

@duncangrist
Copy link
Author

@shahata - yes, that does seem to resolve the original bug thanks.

However, I disagree on the viability of your workaround for the "0 becomes empty string thing" because normally angular has no trouble when using zeros in selects. In your Plunk, if you remove the async-validate attribute from the select, and convert the strings back to ints, you'll see that zero behaves just like all the other numbers. It appears that it's specifically being broken by the $asyncValidators feature.

@shahata
Copy link
Contributor

shahata commented Sep 5, 2014

Yes, I agree that this should be reported as a different issue. The workaround is just that - a way to work around the issue.

@duncangrist
Copy link
Author

Fair enough - I was going to raise it as an issue, but I hesitated because it's not actually a reproducible issue until this Issue's bug is resolved. What would you suggest is the correct play here?

@Narretz Narretz added this to the 1.3.0-rc.2 milestone Sep 10, 2014
@tbosch tbosch modified the milestones: 1.3.0-rc.2, 1.3.0-rc.3 Sep 15, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 16, 2014

@caitp Could you review this?

@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.3, 1.3.0-rc.4 Sep 22, 2014
@IgorMinar
Copy link
Contributor

This behavior is wrong for two reasons.

1/ after validation the right value should be set
2/ during validation we should not cause flicker by setting the value to nothing

It's interesting that the text input doesn't have these issues at all as you can see in the username field of the @matsko's demo app: http://yearofmoo-angularjs-forms.herokuapp.com/#/ I'd expect the username to be also temporarily set to the old value or empty string while the validation is happening (which would be wrong behavior but at least consistent with the select behavior)

And the 0 bug is also real. I don't know it's cause yet.

@IgorMinar
Copy link
Contributor

#8937 seems like the right fix

with that fix the 0 bug is also fixed.

@jeffbcross jeffbcross assigned jeffbcross and unassigned caitp Sep 30, 2014
@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.4, 1.3.0-rc.5 Oct 2, 2014
@shahata shahata closed this as completed in f717416 Oct 3, 2014
jeffbcross pushed a commit to jeffbcross/angular.js that referenced this issue Oct 7, 2014
Closes angular#8929

Conflicts:
	src/ng/directive/select.js
@duncangrist
Copy link
Author

I've confirmed that both issues reported in here are fixed my end when using 1.3.0-rc.5. Thanks a lot guys :)

@jfarid27
Copy link

jfarid27 commented Apr 4, 2016

I'd love to reopen this. I'm able to recreate ng-model breaking in Angular 1.4.8 when using async validators, this time using just input text elements. Again the issue is when asyncValidators are added, changes to ng-model aren't ran when the viewValue changes.

Version: 1.4.8
Example: https://jsfiddle.net/ks0khrbc/

@jfarid27
Copy link

jfarid27 commented Apr 4, 2016

I also went further and added asyncValidators by creating a new directive that accesses a required ngModel controller, as suggested by a coworker since that's the exact example defined in the documentation for forms. It surprisingly still breaks ng-model actually.

https://jsfiddle.net/3seaL8qu/

@petebacondarwin
Copy link
Contributor

@jfarid27 - I don't understand your use case. In both examples you return a promise that is never resolved, so of course the value never becomes valid, and so the model is not updated.

@jfarid27
Copy link

jfarid27 commented Apr 4, 2016

@petebacondarwin yeah it seems to be the case. It was a little unintuitive but I wanted an async validation that does not immediately make requests to resolve and exposes the deferred objects for a separate service to resolve. Maybe there's a case for this, but it seems like more work to implement and a bad idea.

@petebacondarwin
Copy link
Contributor

You should be able to do this. You can create a service that does the work and returns a promise to the async validator. When the promise is resolved by the service the validator will resolve too

@jfarid27
Copy link

jfarid27 commented Apr 5, 2016

@petebacondarwin yes but I'd also love to see model view changes. I'm trying to do a "bulk" validation in one API call after a user fills out all their form elements then clicks a button. The moment I add an $asyncValidator, there are no longer any $modelView updates. I figured I would try to use the $asyncValidators, but it seems to serve only the specific purpose of validating model immediately and not allowing any updates to the $modelView until the promise is resolved.

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

Successfully merging a pull request may close this issue.

10 participants