fix(input): listen on "change" for radio and checkbox #14685

Narretz · 2016-05-27T09:33:18Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug fix

What is the current behavior? (You can also link to an open issue here)
radio and checkbox listen on "click" which can cause unforeseen behavior

What is the new behavior (if this is a feature change)?
radio and checkbox listen on "change"

Does this PR introduce a breaking change?
Yes, for certain unit-test behaviors

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~[ ] Docs have been added / updated (for bug fixes / features)~~

Other information:

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 need to be attached to the document to propagate events
correctly. This should only be of concern in unit-tests that compile input elements and
trigger click events on them. This is because we now listen to the change event which
gets automatically triggered by browsers when a checkbox or radio is clicked. However,
this may fail in some browsers when the elements are not attached to the document.

Before:

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

      browserTrigger(inputElm[0], 'click');
      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:

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

      browserTrigger(inputElm[0], 'click');
      expect($rootScope.checkbox).toBe(true);
    });

gkalpak · 2016-05-27T10:30:47Z

test/helpers/testabilityPatch.js

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

+        $rootElement.append(helper.formElm);
+        jqLite($document[0].body).append($rootElement);


OOC, why is this needed ?

OK, I see why it is needed. Is it necessary on all browsers or some of them ? Which ones ?

Some browsers don't trigger the "change" event on "click" when the input isn't attached to the document. This may be a bug, but it also makes sense. This is why the BC should only appear in (unit )tests.

If we use browserTrigger, it is needed in Chrome. It works in Firefox. Haven't tested other browsers. If you use the native element.click() fn, you need it in both FF and Chrome

We recently changed browserTrigger to manually raise events if the node is not connected to a document - c8bfbfc - would that solve this problem?

Possibly, I'd have to test. It's not so much an issue, rather a slight annoyance. The PR still isn't grren because an ngTouch test fails on Safari iOS - related to the clickbusting code in ngClick. I don't have an iOs device, so I can't test this.

gkalpak · 2016-05-27T10:53:50Z

Generally, LGTM.

I am a little worried of what might unforseeably break as a result of this change. For example, usecases depending on the relative order of event callbacks (e.g. click listeners registered after our listener and expecting the model to have been updated).

We should mention it in the commit message imo.
There might be other cases that I missed.

Talking about BC messages, mentioning browserTrigger would be confusing (since it Angular internal stuff). I would just replace it with a comment, e.g.: // Somehow trigger a `click`event on the element.

Narretz · 2016-05-27T11:00:02Z

You are right, this could break some other cases.
And I always thought browserTrigger was external ... comes from working on core so much!

Narretz · 2016-05-27T16:05:07Z

I've fixed the test and updated the commit message / BC note

gkalpak · 2016-05-29T19:23:01Z

Something is still failing on mobile Safari 😞

Narretz · 2016-05-31T11:55:34Z

@gkalpak Yeah I noticed that. Is probably gonna be a pain in the ass to fix.

mgol · 2016-11-21T15:32:17Z

@Narretz This just hit me in a production-running app that also used FastClick. FastClick morphs click handlers into touchstart ones with some hacks around it and in 50% of cases this broke the radios on the site in iOS 10.

Narretz · 2016-11-23T12:41:04Z

Intereesting. Maybe this will pass in ios10? Because so far ios8 is blocking this ...

mgol · 2016-11-23T13:11:26Z

Angular 2 supports iOS 7-10 so I'd rather not drop iOS 8 support in ng1; we
should try to cover the same browsers IMO...

On Wed, 23 Nov 2016 at 13:42, Martin Staffa notifications@github.com
wrote:

Intereesting. Maybe this will pass in ios? Because so far ios8 is blocking
this ...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#14685 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABrUnpE3732pd9SIA17qFVudd4SmGI5Rks5rBDSggaJpZM4IoVMX
.

Narretz · 2017-10-06T13:09:38Z

@petebacondarwin @gkalpak I've updated this (BC) PR for 1.7.0. Can you give it another look? Or probably a new look since it's from last year. ;)
Specifically I updated the commit message to include more info about possible Breaking Changes and info about which browsers are affected by the event propagation issue.

gkalpak

LGTM 👍

I am not thrilled about the fact that clicking on an detached element does not trigger a change event (but this is a browser/spec issue, not a problem with this PR 😞). I have briefly thought about changing browserTrigger() to also emit a change event if an detached checkbox/radiobutton's checked property is changed as a result of click event, but it felt too automagical 😃

A couple of commit message nitpicks:

Mention "instead of click" in the commit message subject (to make it obvious for those readng through the changelog - it now sounds more like change is listened to in addition to click).
you have don't have to attach --> you don't have to attach
Put injectables and function names in `...` (e.g. $rootElement, $document, triggerHandler()).

gkalpak · 2017-10-09T08:00:15Z

test/BinderSpec.js

    element = $compile(
      '<div>' +
        '<input type="radio" ng-model="sex" value="female">' +
        '<input type="radio" ng-model="sex" value="male">' +
      '</div>')($rootScope);
+
+    jqLite($document[0].body).append($rootElement.append(element));


Maybe leave a comment here why this is needed.

gkalpak · 2017-10-09T08:05:17Z

test/ng/directive/ngRepeatSpec.js

@@ -365,6 +365,8 @@ describe('ngRepeat', function() {
            '</li>' +
          '</ul>')(scope);

+      jqLite($document[0].body).append($rootElement.append(element));


I think a comment would be nice here too.

…d 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 angular#4516 Closes angular#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('<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.

Narretz · 2017-10-11T14:06:40Z

I Opened an issue on Chromium for future reference: https://bugs.chromium.org/p/chromium/issues/detail?id=773680

Narretz added type: bug component: forms frequency: moderate severity: inconvenient labels May 27, 2016

Narretz added this to the 1.5.x milestone May 27, 2016

googlebot added the cla: yes label May 27, 2016

gkalpak reviewed May 27, 2016
View reviewed changes

Narretz force-pushed the fix-radio-checkbox branch from ee23d70 to 512c031 Compare May 27, 2016 15:59

Narretz mentioned this pull request Feb 14, 2017

$viewChangeListeners not called with <input type="radio"> #15696

Closed

Narretz modified the milestones: 1.5.x, 1.7.0 Mar 8, 2017

Narretz added the needs: breaking change label Mar 24, 2017

Narretz force-pushed the fix-radio-checkbox branch 2 times, most recently from 3c2089a to 0c9ca91 Compare March 31, 2017 14:52

Narretz force-pushed the fix-radio-checkbox branch 3 times, most recently from ece7f22 to c9c7d7c Compare October 6, 2017 12:32

gkalpak approved these changes Oct 9, 2017

View reviewed changes

Narretz force-pushed the fix-radio-checkbox branch from c9c7d7c to f460088 Compare October 11, 2017 13:34

Narretz merged commit 656c8fa into angular:master Oct 13, 2017

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): listen on "change" for radio and checkbox #14685

fix(input): listen on "change" for radio and checkbox #14685

Narretz commented May 27, 2016

gkalpak May 27, 2016

gkalpak May 27, 2016

Narretz May 27, 2016

Narretz May 27, 2016

petebacondarwin Jun 10, 2016

Narretz Jun 10, 2016

gkalpak commented May 27, 2016 •

edited

Loading

Narretz commented May 27, 2016

Narretz commented May 27, 2016

gkalpak commented May 29, 2016

Narretz commented May 31, 2016

mgol commented Nov 21, 2016

Narretz commented Nov 23, 2016 •

edited

Loading

mgol commented Nov 23, 2016

Narretz commented Oct 6, 2017

gkalpak left a comment

gkalpak Oct 9, 2017

gkalpak Oct 9, 2017

Narretz commented Oct 11, 2017

fix(input): listen on "change" for radio and checkbox #14685

fix(input): listen on "change" for radio and checkbox #14685

Conversation

Narretz commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented May 27, 2016 • edited Loading

Narretz commented May 27, 2016

Narretz commented May 27, 2016

gkalpak commented May 29, 2016

Narretz commented May 31, 2016

mgol commented Nov 21, 2016

Narretz commented Nov 23, 2016 • edited Loading

mgol commented Nov 23, 2016

Narretz commented Oct 6, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Oct 11, 2017

gkalpak commented May 27, 2016 •

edited

Loading

Narretz commented Nov 23, 2016 •

edited

Loading