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

ng-required validation not working properly with true/false radio values. #2594

Closed
michaelmoussa opened this issue May 6, 2013 · 9 comments

Comments

@michaelmoussa
Copy link

See Fiddle.

I have a use case where radio input data is stored server-side as booleans - true for "yes", false for "no".

If I use value="true" and value="false" on my form, the radio control corresponding to the default value is not selected when the page loads (validation, however, works as expected).

If I use ng-value="true" and ng-value="false" instead, the default value is selected automatically based on the model as expected, but validation doesn't work correctly. The form is only declared valid if "Yes" is selected, but still invalid if "No" is selected.

I can't select "no" by default because the business rules say that the inputs should initially be unselected (presumably so the user has to actually go through the whole form and answer the questions instead of just skipping ahead because an option has already been selected for them).

@kpko
Copy link

kpko commented Jun 28, 2013

I have the same problem here. Also the model value of my second radio button (ng-value="false") is undefined instead of false, if I use ng-required.

<input type="radio" ng-required="true" ng-model="test" ng-value="true" />
<input type="radio" ng-required="true" ng-model="test" ng-value="false" />

@michaelmoussa
Copy link
Author

@kpko I ended up writing my own directive to work around it. Feel free to use it! https://github.com/michaelmoussa/ng-boolean-radio

@pkozlowski-opensource
Copy link
Member

Yes, this is a corner-case. The issue is due to this check:

if (attr.required && (isEmpty(value) || value === false)) {

that was introduced to cater for a check-box use-case:

it('should be required if false', function() {
compileInput('<input type="checkbox" ng:model="value" required />');
browserTrigger(inputElm, 'click');
expect(inputElm[0].checked).toBe(true);
expect(inputElm).toBeValid();
browserTrigger(inputElm, 'click');
expect(inputElm[0].checked).toBe(false);
expect(inputElm).toBeInvalid();
});

I'm afraid that I don't know what is the best course of action here :-(

@pkozlowski-opensource
Copy link
Member

The real question is this: should a false value be considered as fulfilling the required condition....

@michaelmoussa
Copy link
Author

The real question is this: should a false value be considered as fulfilling the required condition....

I believe it should. In a "yes/no" scenario with no default option selected, it makes sense to map a "no" selection to false.

IMO, checking for null would be the best way to determine that the user has not fulfilled a required condition.

@PowerKiKi
Copy link
Contributor

👍 I agree with @michaelmoussa on this. We have a Yes/No <select> mapped to true/false values. If we make it required, the "No" cannot be selected.

In my opinion it's very well known that null is the absence of value, whereas false is an actual value. And that would make sense to consider only null instead of false if a value is required.

Now, for the checkbox use-case mentioned, I guess it may also make sense. Couldn't we make a special case for checkbox only ? Something along the lines of:

if (attr.required && (isEmpty(value) || (attr.type = "checkbox" && value === false))) {

@rngadam
Copy link

rngadam commented Sep 3, 2013

arrrgh, spent way too much time trying to figure out why my form was throwing required. YES, false should be considered as fulfilling the required condition otherwise it makes our model/view extremely confusing.

@michaelmoussa solution works for me. Please, please pretty please use it!

@petebacondarwin
Copy link
Contributor

I agree with @PowerKiKi - we should make the checkbox input a special case but that we shouldn't put the burden of this special case on the required directive.

We should change the normal required directive to check only for empty/null/undefined but then change the checkboxInputType directive add in an extra validator that checks for false.

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Sep 24, 2013
Make the `required` directive accept the model value `false` when it is
not an <input> element or when the <input> element is not a checkbox

Closes angular#3490 angular#2594
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Sep 25, 2013
`checkboxInputType` and `ngList` directives need to have special logic for whether
they are empty or not.  Previously this had been hard coded into their
own directives or the `ngRequired` directive.  This made it difficult to handle
these special cases.

This change factors out the question of whether an input is empty into a method
`$isEmpty` on the `ngModelController`.  The `ngRequired` directive now uses this
method when testing for validity and directives, such as `checkbox` or `ngList`
can override it to apply logic specific to their needs.

Closes angular#3490, angular#3658, angular#2594
vojtajina pushed a commit to vojtajina/angular.js that referenced this issue Oct 7, 2013
`checkboxInputType` and `ngList` directives need to have special logic for whether
they are empty or not.  Previously this had been hard coded into their
own directives or the `ngRequired` directive.  This made it difficult to handle
these special cases.

This change factors out the question of whether an input is empty into a method
`$isEmpty` on the `ngModelController`.  The `ngRequired` directive now uses this
method when testing for validity and directives, such as `checkbox` or `ngList`
can override it to apply logic specific to their needs.

Closes angular#3490, angular#3658, angular#2594
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
`checkboxInputType` and `ngList` directives need to have special logic for whether
they are empty or not.  Previously this had been hard coded into their
own directives or the `ngRequired` directive.  This made it difficult to handle
these special cases.

This change factors out the question of whether an input is empty into a method
`$isEmpty` on the `ngModelController`.  The `ngRequired` directive now uses this
method when testing for validity and directives, such as `checkbox` or `ngList`
can override it to apply logic specific to their needs.

Closes angular#3490, angular#3658, angular#2594
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
`checkboxInputType` and `ngList` directives need to have special logic for whether
they are empty or not.  Previously this had been hard coded into their
own directives or the `ngRequired` directive.  This made it difficult to handle
these special cases.

This change factors out the question of whether an input is empty into a method
`$isEmpty` on the `ngModelController`.  The `ngRequired` directive now uses this
method when testing for validity and directives, such as `checkbox` or `ngList`
can override it to apply logic specific to their needs.

Closes angular#3490, angular#3658, angular#2594
@Narretz
Copy link
Contributor

Narretz commented Jun 24, 2014

I think this was fixed by the commits mentioned above, except for setting the initial checked attribute if the model is set to boolean when the input is compiled, and value is used instead of ng-value. I am going to raise this as a separate issue.

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

No branches or pull requests

7 participants