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

Commit

Permalink
fix(input): listen on "change" instead of "click" for radio/checkbox …
Browse files Browse the repository at this point in the history
…ngModels

input[radio] and inout[checkbox] now listen on the change event instead
of the click event. This fixes issue with 3rd party libraries that trigger
a change event on inputs, e.g. Bootstrap 3 custom checkbox / radio button
toggles.
It also makes it easier to prevent specific events that can cause a checkbox / radio
to change, e.g. click events. Previously, this was difficult because the custom click
handler had to be registered before the input directive's click handler.

It is possible that radio and checkbox listened to click because IE8 has
broken support for listening on change, see http://www.quirksmode.org/dom/events/change.html

Closes #4516
Closes #14667
Closes #14685

BREAKING CHANGE:

`input[radio]` and `input[checkbox]` now listen to the "change" event instead of the "click" event.
Most apps should not be affected, as "change" is automatically fired by browsers after "click"
happens.

Two scenarios might need migration:

- Custom click events:

Before this change, custom click event listeners on radio / checkbox would be called after the
input element and `ngModel` had been updated, unless they were specifically registered before
the built-in click handlers.
After this change, they are called before the input is updated, and can call event.preventDefault()
to prevent the input from updating.

If an app uses a click event listener that expects ngModel to be updated when it is called, it now
needs to register a change event listener instead.

- Triggering click events:

Conventional trigger functions:

The change event might not be fired when the input element is not attached to the document. This
can happen in **tests** that compile input elements and
trigger click events on them. Depending on the browser (Chrome and Safari) and the trigger method,
the change event will not be fired when the input isn't attached to the document.

Before:

```js
    it('should update the model', inject(function($compile, $rootScope) {
      var inputElm = $compile('<input type="checkbox" ng-model="checkbox" />')($rootScope);

      inputElm[0].click(); // Or different trigger mechanisms, such as jQuery.trigger()
      expect($rootScope.checkbox).toBe(true);
    });
```

With this patch, `$rootScope.checkbox` might not be true, because the click event
hasn't triggered the change event. To make the test, work append the inputElm to the app's
`$rootElement`, and the `$rootElement` to the `$document`.

After:

```js
    it('should update the model', inject(function($compile, $rootScope, $rootElement, $document) {
      var inputElm = $compile('<input type="checkbox" ng-model="checkbox" />')($rootScope);

      $rootElement.append(inputElm);
      $document.append($rootElement);

      inputElm[0].click(); // Or different trigger mechanisms, such as jQuery.trigger()
      expect($rootScope.checkbox).toBe(true);
    });
```

`triggerHandler()`:

If you are using this jQuery / jqLite function on the input elements, you don't have to attach
the elements to the document, but instead change the triggered event to "change". This is because
`triggerHandler(event)` only triggers the exact event when it has been added by jQuery / jqLite.
  • Loading branch information
Narretz authored Oct 13, 2017
1 parent 5462373 commit 656c8fa
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 9 deletions.
4 changes: 2 additions & 2 deletions src/ng/directive/input.js
Original file line number Diff line number Diff line change
Expand Up @@ -1820,7 +1820,7 @@ function radioInputType(scope, element, attr, ctrl) {
}
};

element.on('click', listener);
element.on('change', listener);

ctrl.$render = function() {
var value = attr.value;
Expand Down Expand Up @@ -1854,7 +1854,7 @@ function checkboxInputType(scope, element, attr, ctrl, $sniffer, $browser, $filt
ctrl.$setViewValue(element[0].checked, ev && ev.type);
};

element.on('click', listener);
element.on('change', listener);

ctrl.$render = function() {
element[0].checked = ctrl.$viewValue;
Expand Down
7 changes: 6 additions & 1 deletion test/BinderSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,12 +401,17 @@ describe('Binder', function() {
expect(optionC.text()).toEqual('C');
}));

it('ItShouldSelectTheCorrectRadioBox', inject(function($rootScope, $compile) {
it('ItShouldSelectTheCorrectRadioBox', inject(function($rootScope, $compile, $rootElement, $document) {
element = $compile(
'<div>' +
'<input type="radio" ng-model="sex" value="female">' +
'<input type="radio" ng-model="sex" value="male">' +
'</div>')($rootScope);

// Append the app to the document so that "click" on a radio/checkbox triggers "change"
// Support: Chrome, Safari 8, 9
jqLite($document[0].body).append($rootElement.append(element));

var female = jqLite(element[0].childNodes[0]);
var male = jqLite(element[0].childNodes[1]);

Expand Down
7 changes: 6 additions & 1 deletion test/helpers/testabilityPatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ function generateInputCompilerHelper(helper) {
};
});
});
inject(function($compile, $rootScope, $sniffer) {
inject(function($compile, $rootScope, $sniffer, $document, $rootElement) {

helper.compileInput = function(inputHtml, mockValidity, scope) {

Expand All @@ -341,6 +341,11 @@ function generateInputCompilerHelper(helper) {
// Compile the lot and return the input element
$compile(helper.formElm)(scope);

$rootElement.append(helper.formElm);
// Append the app to the document so that "click" on a radio/checkbox triggers "change"
// Support: Chrome, Safari 8, 9
jqLite($document[0].body).append($rootElement);

spyOn(scope.form, '$addControl').and.callThrough();
spyOn(scope.form, '$$renameControl').and.callThrough();

Expand Down
7 changes: 6 additions & 1 deletion test/ng/directive/booleanAttrsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,15 @@ describe('boolean attr directives', function() {
}));


it('should not bind checked when ngModel is present', inject(function($rootScope, $compile) {
it('should not bind checked when ngModel is present', inject(function($rootScope, $compile, $document, $rootElement) {
// test for https://github.com/angular/angular.js/issues/10662
element = $compile('<input type="checkbox" ng-model="value" ng-false-value="\'false\'" ' +
'ng-true-value="\'true\'" ng-checked="value" />')($rootScope);

// Append the app to the document so that "click" triggers "change"
// Support: Chrome, Safari 8, 9
jqLite($document[0].body).append($rootElement.append(element));

$rootScope.value = 'true';
$rootScope.$digest();
expect(element[0].checked).toBe(true);
Expand Down
22 changes: 20 additions & 2 deletions test/ng/directive/inputSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3974,7 +3974,7 @@ describe('input', function() {

describe('radio', function() {

it('should update the model', function() {
they('should update the model on $prop event', ['click', 'change'], function(event) {
var inputElm = helper.compileInput(
'<input type="radio" ng-model="color" value="white" />' +
'<input type="radio" ng-model="color" value="red" />' +
Expand All @@ -3990,7 +3990,8 @@ describe('input', function() {
expect(inputElm[1].checked).toBe(true);
expect(inputElm[2].checked).toBe(false);

browserTrigger(inputElm[2], 'click');
if (event === 'change') inputElm[2].checked = true;
browserTrigger(inputElm[2], event);
expect($rootScope.color).toBe('blue');
});

Expand Down Expand Up @@ -4092,6 +4093,23 @@ describe('input', function() {
});


they('should update the model on $prop event', ['click', 'change'], function(event) {
var inputElm = helper.compileInput('<input type="checkbox" ng-model="checkbox" />');

expect(inputElm[0].checked).toBe(false);

$rootScope.$apply('checkbox = true');
expect(inputElm[0].checked).toBe(true);

$rootScope.$apply('checkbox = false');
expect(inputElm[0].checked).toBe(false);

if (event === 'change') inputElm[0].checked = true;
browserTrigger(inputElm[0], event);
expect($rootScope.checkbox).toBe(true);
});


it('should format booleans', function() {
var inputElm = helper.compileInput('<input type="checkbox" ng-model="name" />');

Expand Down
8 changes: 6 additions & 2 deletions test/ng/directive/ngRepeatSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ describe('ngRepeat', function() {
});


it('should iterate over object with changing primitive property values', function() {
it('should iterate over object with changing primitive property values', inject(function($rootElement, $document) {
// test for issue #933

element = $compile(
Expand All @@ -365,6 +365,10 @@ describe('ngRepeat', function() {
'</li>' +
'</ul>')(scope);

// Append the app to the document so that "click" on a radio/checkbox triggers "change"
// Support: Chrome, Safari 8, 9
jqLite($document[0].body).append($rootElement.append(element));

scope.items = {misko: true, shyam: true, zhenbo:true};
scope.$digest();
expect(element.find('li').length).toEqual(3);
Expand Down Expand Up @@ -395,7 +399,7 @@ describe('ngRepeat', function() {
expect(element.find('input')[0].checked).toBe(false);
expect(element.find('input')[1].checked).toBe(true);
expect(element.find('input')[2].checked).toBe(true);
});
}));
});

describe('alias as', function() {
Expand Down

0 comments on commit 656c8fa

Please sign in to comment.