angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

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

Closed lovasoa closed 6 years ago

lovasoa commented 7 years ago

Bug report

How to reproduce

  1. Open the following jsfiddle: http://jsfiddle.net/ADukg/10031
  2. Click the button
  3. Tick the second radio checkbox
  4. Tick the first radio checkbox

What is the current behavior?

An alert appears ONLY after step 3.

What is the expected behavior?

An alert should appear after step 3 and 4. The functions specified in $viewChangeListeners should be called every time the view value changes, as stated in the documentation

Which versions of AngularJS, and which browser / OS are affected by this issue?

The fiddle demonstrates the bug with the latest snapshot of angular. Tested in both firefox and chrome.

Other information (e.g. stacktraces, related issues, suggestions how to fix)

The issue was reported here on stackoverflow: http://stackoverflow.com/questions/34703156/only-last-radio-button-triggers-viewchangelistener

edenferreira commented 7 years ago

I see angular 1.0.1 on your jsFiddle. I've updated to 1.6.1 and appears to work http://jsfiddle.net/ADukg/10030/

lovasoa commented 7 years ago

@edenferreira I updated the fiddle. The bug is still present. In older versions of angular, step 4 worked and step 3 didn't, in newer versions, it's the opposite, but the bug is always present.

gkalpak commented 7 years ago

@lovasoa, the problem is that controls are registered on their parent form by their name. Having the same name on one than one controls, the last one registered (usually the last to appear in DOM) overwrites all the others. This is what is happening here. Note that (unlike "traditional" HTML forms), readio buttons do not need the same name on AngularJS forms. You can either give them unique names or omit the name property altogether and let AngularJS assign unique names automatically.

Since each control has its own instance of NgModeController, you need to hook into each one's $viewValueListeners. Alternatively, you can use the ngChange directive, which does exctly that.

Closing since everything works as expected, but feel free to continue the discussion below.

lovasoa commented 7 years ago

This is still at least very confusing. The documentation states that $viewValueListeners is an Array of functions to execute whenever the view value has changed.. Here, the view value changes, and the function isn't called.

The documentation also states that

This can be used in place of additional $watches against the model value.

Here, the result is clearly different from what we would get by placing a $watch on the model value.

gkalpak commented 7 years ago

Actually, you are right: Something is wrong. Even if the $viewChangeListener is attached to the last radio button, it does not fire when the value changes (by selecting the other radio button).

Narretz commented 7 years ago

I think the problem is with the way inputs are added to their parent form controller. They are added by name, which means since the radio inputs have the same name, what you have in formCtrl.bool is the second radios ngModelController . However, each radio still has its own ngModelController, so in your example you are only adding the viewChangeListener to the second radio. Radio inputs aren't really working to their best right now, but unfortunately we haven't figured out how / attempted to solve this. Previous issues: https://github.com/angular/angular.js/issues/15009 https://github.com/angular/angular.js/issues/8506 https://github.com/angular/angular.js/issues/7647

Specifically we should have named radio groups that behave like a group, but I wonder if we can do that without a breaking change.

gkalpak commented 7 years ago

That's what I thoguht it was at first, but this is different actually. Even if you change the names, the problem is that the $viewChangeListeners are not fired on radio buttons when the value changes because of a click on another button.

Basically, the radioInputType has a listener for the click event, so when you click on it $viewChangeListeners are executed. But when the value changes because you clicked on another radio, then we fail to detct the change (or fail to detect that we need to fire $viewChangeListeners because of this change).

I haven't looked into it yet, but the current behavior is indeed weird/inconsistent 😞

Narretz commented 7 years ago

Oh, that's interesting. I assume then we need to switch from listening on click to listening on change - would be breaking, though: https://github.com/angular/angular.js/blob/71f437c15a606b542536e70c392808e84d982078/src/ng/directive/input.js#L1819

Same issue might exist for check boxes?

Or we could listen on blur, too?

Narretz commented 7 years ago

Look what I found: https://github.com/angular/angular.js/pull/14685 There's a problem with ios8 here ...

gkalpak commented 7 years ago

I think the change event doesn't fire when clicking on another control (at least in Chrome) - similar to how ngChange works. And checkboxes don't have the same issue afaict. Actually, I am not exactly sure there is an issue after all :grin:

As expected, the $viewChangeListeners are fired when the value of the field changes as a result of user intraction with the respective control. For example, when you type something into an textfield or when you click on a checkbox (the value changes as a result of the interaction with the control and the $viewChangeListeners are triggered).

The radio buttons are kind of special in that you can't change their state (unselect) and thus update the bound model directly via interaction with the control itself. Imagine the following radios:

<input type="radio" name="a" value="foo" ng-model="modelValue" />
<input type="radio" name="b" value="bar" ng-model="modelValue" />

You can set modelValue to "foo" by clicking on radio a (and the $viewChangeListeners will fire as expected). But then you can't change the model value through the control itself. You have to click (interact) with radio b. So, technically, the change in radio a's state comes from the model not the view: click radio b --> modelValue set to "bar" --> radio a unselected

In that regard (and since ngChange is actually tied to the $viewChangeListeners) things seem to work as expected.

Note, that the above radio button scenario is equivalent to having two textfields bound to the same model value (so typing into one will update the other). In this case too, ngChange will only fire on the field you are typing, not on the other field, whose value is programmatically update (from the model to the view).

It might be confusing that ngChange is neither bound to the change event (although that would not make any difference in this case) nor to programmatic changes to $modelValue, but it is like this for a reason and not likely to change pun intended.

Quoting the docs:

[ngChange] will not be evaluated:

  • ...
  • if the model is changed programmatically and not by a change to the input value

Having said all that, I would like to change my official view to "works as expected" 💼

lovasoa commented 7 years ago

I am going to repeat myself, but the docs say that

$viewValueListeners is an Array of functions to execute whenever the view value has changed.

and

This can be used in place of additional $watches against the model value.

And you are saying that

As expected, the $viewChangeListeners are fired when the value of the field changes as a result of user intraction with the respective control.

Reading the docs, the behavior you describe is not the one that is expected.

lovasoa commented 7 years ago

And working around this by using different names for the same radio buttons group is far from being an ideal work-around, as angular won't change the name attributes, and thus we end up with sementically invalid HTML.

gkalpak commented 7 years ago

$viewValueListeners is an Array of functions to execute whenever the view value has changed. [...] This can be used in place of additional $watches against the model value.

And you are saying that the $viewChangeListeners are fired when the value of the field changes as a result of user intraction with the respective control. Reading the docs, the behavior you describe is not the one that is expected.

I am afraid that is what exactly I am saying :smiley: The docs need to be updated. $viewChangeListeners are indeed meant to be fired when the value changes as a result of user interaction with the control (aka the view). To be more precise, they are fired when the modelValue changes as a result of calling $comminViewValue() (which is typically called from $setViewValue(), which is typically called when the value is changed in the view).

And (afaict) $viewChangeListeners can be used in place of additional watchers only for changes that originate from the view (or one of the aforementioned methods). The reasoning being that for programmatic changes to the modelValue directly, you don't need a watcher, because you are making the change, so you can react on it directly. (Admittedly, this is not always straight forward 😕)

I'll let @Narretz confirm that I have not missed something and then we can update the docs :grin:

lovasoa commented 7 years ago

And (afaict) $viewChangeListeners can be used in place of additional watchers only for changes that originate from the view (or one of the aforementioned methods).

But in the case of the input buttons, the change is indeed coming from the view. I am very curious of how you are going to update the docs without making $viewChangeListeners look completely useless and unpredictable.

In my opinion, the behavior described here is clearly a bug.

lovasoa commented 7 years ago

The reasoning being that for programmatic changes to the modelValue directly, you don't need a watcher, because you are making the change, so you can react on it directly.

In the case of radio buttons, I am not making any programmatic change. I am curious to know any legitimate use case of $viewChangeListeners with the current behavior, in particular with radio buttons.

gkalpak commented 7 years ago

As explained in previous comment, in Angular when you click radio B it updates the model value (change from view -> model) and fires B's $viewChangeListeners and then because radio A is bound to the same model, radio A's state changes to unchecked (change model -> view) and no $viewChangeListeners are fired. Also (as already mentioned) this is consistent with how the native change event works. If you bind change to all radios of a radio-group, the event handlers are only triggered when the radio is clicked (not when it gets unchecked because you clicked another radio).

If you need to be notified for changes in the bound model, you can either add ng-change to all radios (similar to what you would do for native change event) or wrap them in a parent element and add a directive that attach a change event listener to capture the bubbling change event (similar to what you would do for native change event) or add a watcher (🤢) or go wild and create a custom radiogroup component that handles all that more gracefully.

gkalpak commented 6 years ago

Based on the discussion above (and since the docs have been updated to better reflect what $viewChangeListeners are), I am going to close this as works as expected.