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

fix(aria/ngModel): do not overwrite the default $isEmpty() method for checkboxes #14625

Closed
wants to merge 2 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented May 17, 2016

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 overwrites NgModelController.$isEmpty() for custom checkboxes (only considering false as empty), despite the fact that custom checkboxes don't necessarily have custom parsers setting falsy values to false. (E.g. the checkbox is considered "checked" even if value is undefined (because undefined !== 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

Other information:
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 #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 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:

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:

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:

.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;
      };
    }
  };
})

@gkalpak
Copy link
Member Author

gkalpak commented May 17, 2016

I wonder if it would make more sense to provide a custom $isEmpty method to checkboxes, that interprets all falsy values as "empty"...

@Narretz
Copy link
Contributor

Narretz commented Jul 28, 2016

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 ...

@gkalpak
Copy link
Member Author

gkalpak commented Jul 29, 2016

I was referring to custom checkboxes (I think 😃), which don't necessarily have a checked property. For input[checkbox] we are already doing the right thing (basing emptiness on the value of the checked property).

But, I am still not sure if it is a good idea to provide a custom $isEmpty function for custom checkboxes. Or at least we should document it properly, because it is happening under the hood and we don't mention it anywhere.

expect(element.attr('aria-checked')).toBe('false');
});

it('should rely on the `$isEmpty()` method', function() {
Copy link
Contributor

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.

@Narretz
Copy link
Contributor

Narretz commented Aug 3, 2016

You are right, the real checkbox uses checked. I'm fine with removing the custom isEmpty.

@gkalpak
Copy link
Member Author

gkalpak commented Aug 5, 2016

@Narretz, is this a LGTM? 😛
(I fixed the test description btw.)

@Narretz
Copy link
Contributor

Narretz commented Aug 5, 2016

Yes, LGTM :D

gkalpak and others added 2 commits August 8, 2016 16:49
…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;
      };
    }
  };
})
```
@gkalpak gkalpak force-pushed the fix-ngAria-ngModel-require branch from 13bbc79 to 9390da8 Compare August 8, 2016 13:51
@gkalpak gkalpak closed this in 975a617 Aug 8, 2016
@gkalpak gkalpak deleted the fix-ngAria-ngModel-require branch August 8, 2016 14:32
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.

ngAria ngModel directive breaks when used with terminal priority 1<p<201 directive
3 participants