-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(aria/ngModel): do not overwrite the default $isEmpty()
method for checkboxes
#14625
Conversation
I wonder if it would make more sense to provide a custom |
d84a420
to
d0f14fc
Compare
We could imo check emptiness based on the checked property, no? This would change the semantics (as the model value is currently validated), but I think it makes sense, for checkboxes specifically. I mean they are empty when unchecked almost by definition ... |
I was referring to custom checkboxes (I think 😃), which don't necessarily have a But, I am still not sure if it is a good idea to provide a custom |
expect(element.attr('aria-checked')).toBe('false'); | ||
}); | ||
|
||
it('should rely on the `$isEmpty()` method', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rely on it in which case? This could be more verbose.
You are right, the real checkbox uses checked. I'm fine with removing the custom isEmpty. |
@Narretz, is this a LGTM? 😛 |
Yes, LGTM :D |
…or checkboxes Previously, `ngAria` would overwrite the default `ngModelController.$isEmpty()` method for custom `checkbox`-shaped controls (e.g. `role="checkbox"` or `role="menuitemcheckbox"`), using the same implementation as `input[checkbox]` (i.e. `value === false`). While this makes sense for `input[checkbox]` which also defines a custom parser, it doesn't make sense for a custom `checkbox` out-of-the-box. For example, an unintialized value (`undefined`) would make the checkbox appear as "checked". If the user wants to provide a custom parser (e.g. setting falsy values to `false`), then they should also provide a custom `$isEmpty()` method. As a side effect, this commit solves issue angular#14621. (We could have solved it in different ways.) Fixes angular#14621 BREAKING CHANGE: Custom `checkbox`-shaped controls (e.g. checkboxes, menuitemcheckboxes), no longer have a custom `$isEmpty()` method on their `NgModelController` that checks for `value === false`. Unless overwritten, the default `$isEmpty()` method will be used, which treats `undefined`, `null`, `NaN` and `''` as "empty". **Note:** The `$isEmpty()` method is used to determine if the checkbox is checked ("not empty" means "checked") and thus it can indirectly affect other things, such as the control's validity with respect to the `required` validator (e.g. "empty" + "required" --> "invalid"). Before: ```js var template = '<my-checkbox role="checkbox" ng-model="value"></my-checkbox>'; var customCheckbox = $compile(template)(scope); var ctrl = customCheckbox.controller('ngModel'); scope.$apply('value = false'); console.log(ctrl.$isEmpty()); //--> true scope.$apply('value = true'); console.log(ctrl.$isEmpty()); //--> false scope.$apply('value = undefined'/* or null or NaN or '' */); console.log(ctrl.$isEmpty()); //--> false ``` After: ```js var template = '<my-checkbox role="checkbox" ng-model="value"></my-checkbox>'; var customCheckbox = $compile(template)(scope); var ctrl = customCheckbox.controller('ngModel'); scope.$apply('value = false'); console.log(ctrl.$isEmpty()); //--> false scope.$apply('value = true'); console.log(ctrl.$isEmpty()); //--> false scope.$apply('value = undefined'/* or null or NaN or '' */); console.log(ctrl.$isEmpty()); //--> true ``` -- If you want to have a custom `$isEmpty()` method, you need to overwrite the default. For example: ```js .directive('myCheckbox', function myCheckboxDirective() { return { require: 'ngModel', link: function myCheckboxPostLink(scope, elem, attrs, ngModelCtrl) { ngModelCtrl.$isEmpty = function myCheckboxIsEmpty(value) { return !value; // Any falsy value means "empty" // Or to restore the previous behavior: // return value === false; }; } }; }) ```
…ethod for checkboxes
13bbc79
to
9390da8
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix(es)
What is the current behavior? (You can also link to an open issue here)
ngAria
overwritesNgModelController.$isEmpty()
for custom checkboxes (only consideringfalse
as empty), despite the fact that custom checkboxes don't necessarily have custom parsers setting falsy values tofalse
. (E.g. the checkbox is considered "checked" even if value isundefined
(becauseundefined !== false
.)Also,
ngModel
breaks when manually compiling from a terminal directive with lower priorty (see #14621).Does this PR introduce a breaking change?
Yes!
Please check if the PR fulfills these requirements
Docs have been added / updated (for bug fixes / features)Other information:
Previously,
ngAria
would overwrite the defaultngModelController.$isEmpty()
method for customcheckbox
-shaped controls (e.g.role="checkbox"
orrole="menuitemcheckbox"
), using the sameimplementation as
input[checkbox]
(i.e.value === false
). While this makes sense forinput[checkbox]
which also defines a custom parser, it doesn't make sense for a customcheckbox
out-of-the-box. For example, an unintialized value (
undefined
) would make the checkbox appear as"checked".
If the user wants to provide a custom parser (e.g. setting falsy values to
false
), then theyshould also provide a custom
$isEmpty()
method.As a side effect, this commit solves issue #14621. (We could have solved it in different ways.)
Fixes #14621
BREAKING CHANGE:
Custom
checkbox
-shaped controls (e.g. checkboxes, menuitemcheckboxes), no longer have a custom$isEmpty()
method on theirNgModelController
that checks forvalue === false
. Unlessoverwritten, the default
$isEmpty()
method will be used, which treatsundefined
,null
,NaN
and
''
as "empty".Note: The
$isEmpty()
method is used to determine if the checkbox is checked ("not empty" means"checked") and thus it can indirectly affect other things, such as the control's validity
with respect to the
required
validator (e.g. "empty" + "required" --> "invalid").Before:
After:
If you want to have a custom
$isEmpty()
method, you need to overwrite the default. For example: