From f460088a6409f2f2155f7db5c9d3d89597f46656 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 27 May 2016 11:19:06 +0200 Subject: [PATCH] fix(input): listen on "change" for radio and checkbox ngModels instead of "click" 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 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('')($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('')($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. --- src/ng/directive/input.js | 4 ++-- test/BinderSpec.js | 7 ++++++- test/helpers/testabilityPatch.js | 7 ++++++- test/ng/directive/booleanAttrsSpec.js | 7 ++++++- test/ng/directive/inputSpec.js | 22 ++++++++++++++++++++-- test/ng/directive/ngRepeatSpec.js | 8 ++++++-- 6 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 7497a190e00e..f1444c87c325 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -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; @@ -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; diff --git a/test/BinderSpec.js b/test/BinderSpec.js index 7aa574a2f7f0..59345759b274 100644 --- a/test/BinderSpec.js +++ b/test/BinderSpec.js @@ -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( '
' + '' + '' + '
')($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]); diff --git a/test/helpers/testabilityPatch.js b/test/helpers/testabilityPatch.js index c397af35e6e6..64544e586444 100644 --- a/test/helpers/testabilityPatch.js +++ b/test/helpers/testabilityPatch.js @@ -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) { @@ -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(); diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js index a5fb941a3d98..ac6cbdcbfd04 100644 --- a/test/ng/directive/booleanAttrsSpec.js +++ b/test/ng/directive/booleanAttrsSpec.js @@ -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('')($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); diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index b29fba489fe5..260e11d1d4b7 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -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( '' + '' + @@ -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'); }); @@ -4092,6 +4093,23 @@ describe('input', function() { }); + they('should update the model on $prop event', ['click', 'change'], function(event) { + var inputElm = helper.compileInput(''); + + 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(''); diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index c26d1a923fb1..0db0a73a8bcd 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -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( @@ -365,6 +365,10 @@ describe('ngRepeat', function() { '' + '')(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); @@ -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() {