fix(ngAria): handle non-string model values

[arbus]

Fix for #10212. Compare the string value of the model
so that aria-checked will be properly applied to
type=radio in the case where the model is a number

Closes #10212

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[pkozlowski-opensource]

@arbus While this particular fix addresses the issue #10212 I don't think it is general enough. If I understand the idea of the fix correctly, it tries to compare value displayed in the DOM with the stringified model value. But if I'm not mistaken those can be easily different, especially if you've got custom parsers / formatters.

I'm not up to date with all the recent changes to the NgModelController but couldn't we check the checked property of native radios / check-boxes instead?

I'm definitively not an expert here but the current fix (and the initial code) looks very fishy for me...
@Narretz @shahata you guys are more familiar with the NgModelController code, maybe you've got a better idea of solving this one?

[shahata]

I agree with @pkozlowski-opensource - we shouldn't duplicate logic from ngModel in here, we can just check the checked attribute.

Also, this fix will not work in case newVal = 0 :)

[googlebot]

CLAs look good, thanks!

[arbus]

@shahata The reason for duplicating some of the logic is to accommodate custom radio implementations which don't set the checked attribute.

Thanks for the catch on newVal = 0, fixed now!

[pkozlowski-opensource]

@arbus I was assuming that this logic is there for custom (non-native) radio impls. But I think that we can't go down this path - the current logic is very fragile and I can think of number of scenarios where it would break. So basically the current logic / proposed fix works only for the very specific / simplest conditions and is not good enough in a general case.

If facilities in the current NgModelController are not enough we should extend NgModelController instead of duplicating its logic. @Narretz I guess this is something to take into account when working on the forms / inputs re-design.

[Narretz]

Couldn't we watch the $viewValue in the case of radioInputs?

This is what we do in the directive to check checkedness: https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L1312

[caitp]

Lots of things don't have toString methods --- Object.create(null), { toString: null }, null, to name a few. Safer bet is just '' + newVal

[caitp]

Actually, '' + newval will still throw in the Object.create(null) case, although that is a bit of a corner case.

Maybe val && val.toString ? val.toString() : val would work better.

[caitp]

suggestions: add a helper function like this:

function ToString(O) {
  return (O == null || !O.toString) ? O : O.toString();
}

probably the best bet.

[shahata]

or just do var boolVal = newVal == attr.value like ngModelController does...

[pkozlowski-opensource]

Regardless of the above discussion, don't you guys feel like the underlying / idea of the fix is very fragile??

[Narretz]

$render of inputTypeRadio checks if $viewValue === attr.value, not the with the modelValue. I checked, that works. We could also expose the check as ctrl.$isEmpty(). Then we could do:

var boolVal = !ngModel.$isEmpty(ngModel.$viewValue);

Still a bit silly that we need to pass the $viewValue, since we now again always use $isEmpty with the $viewValue everywhere (or should). We could change $isEmpty in 1.4 to not take an argument, so we'd have a canonical way of getting the empty state for ngAria and other plugins.

[Narretz]

I have created a follow-up PR that uses the same checks as ngModel. Would be nice if someone could review it, thanks! #11007