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

ng-if around a single radio button removes the entire ngModelController from the formController #15009

Closed
tsemer opened this issue Aug 9, 2016 · 18 comments · Fixed by angular-indonesia/angular.js#123

Comments

@tsemer
Copy link

tsemer commented Aug 9, 2016

Currently, when wrapping a single radio button input inside a ng-if, and when the ng-if expression evaluates to "false", the ngModelController is removed entirely from the formController.

Plunker:
https://plnkr.co/edit/6KwTDlydGdC1VHtLoszg?p=preview

A workaround is to use ng-show, but sometimes ng-if is preferable, and should be able to work as well. The current situation also baffles developers as it is non-trivial.

Angular version: 1.5.7

@gkalpak
Copy link
Member

gkalpak commented Aug 9, 2016

It is not obvious in your example (here is a slightly better illustration), but while there are 3 "child" NgModelControllers only one of them is registered on the parent form (under myForm.child); namely the 2nd one (more on that later).

This happens, because each control will assign its model controller on the parent form (if one exists), on a property that matches the value of the control's name attribute (here: "child"). If there are multiple controls with the same [name], then the last one to register itself will overwrite the others. This also means that the parent form only has a reference to the last [name="child"] controller that registers itself.

As expected, since myForm.child references the 2nd "child" control, when it is removed the child property is also removed (deleted) from myForm.

But why is the 2nd child overwriting the other two?

Normally, it would be the 3rd "child" control overwriting the other two, because the last control to register wins. Yet, because of the ngIf that will asynchronously compile and link the 2nd "child" control, it is the 2nd control that registers itself last and overwrites the already registered 1st and 3rd controls.

Conclusion(s):

  1. Angular doesn't support having multiple inputs with the same name (in the same form).
  2. It is possible to add support for this, but this shouldn't be needed in an Angular app. What is your usecase?
  3. Because Angular does not expect controls with the same name, we are not doing a very good job at overwriting previous controls. So, while overwritten controls are not publicly accessible, their validation states still affect the FormController.

Possible Actions:

  1. Investigate whether there is a reasonable and common usecase for supporting "synonymous" controls within an Angular form.
  2. Do a better job at overwriting existing controls. If nothing else, we could $log.debug() a warning that a control with the same name does already exist. Other things we can do, that would make the behavior more straight-forward imo (but involve breaking changes):
    • Properly remove the replaced control (so that it is no longer part of the form).
    • Prevent overwriting an existing control.

@gkalpak
Copy link
Member

gkalpak commented Aug 9, 2016

tl;dr
This is expected behavior (until proven otherwise 😃).
If there is enough justification, it could turn out to be bug or requested feature.

@gkalpak gkalpak modified the milestones: Purgatory, Backlog Aug 9, 2016
@tsemer
Copy link
Author

tsemer commented Aug 10, 2016

Angulr doesn't support having multiple inputs with the same name (in the same form).

But in the special case of radio buttons they must have the same name, since they are bound to one model, and hence, to one model controller, correct?

I've tried now to give them different names. The result is two different radio button groups: one containing child1 and child3, and one containing child2. And of course there are three references to the same model controller on the form, which is sub-optimal.

https://plnkr.co/edit/MNrze43iWbwGUtajWSA3?p=preview

I think different names would bring up even more issues than it would solve. And perhaps more importantly, doesn't angular want to keep to the W3C HTML spec where radio "name" attribute is used as a key for the radio button group?

@gkalpak
Copy link
Member

gkalpak commented Aug 10, 2016

But in the special case of radio buttons they must have the same name, since they are bound to one model, and hence, to one model controller, correct?

Nope. Each element gets its own NgModelController instance (even if all instances bind to the same model value).

I've tried now to give them different names. The result is two different radio button groups: one containing child1 and child3, and one containing child2.

Nope. In reality, there are 3 different radio button groups for the 3 "child" radios (as far as browser understands). But in Angular you don't care about that, because it is the ngModel that determines whether a radio is selected or not. Angular is not concerned with groups (it doesn't need to).

The reason it seems like there are two "child" groups, is related to how Scope works and is not relevant. Make sure to follow the dot rule for models: The expressions you bind to must contain at least one dot (e.g.: ng-model="child" <-- BAD | ng-model="data.child" <-- GOOD).

And of course there are three references to the same model controller on the form, which is sub-optimal.

Nope. There are three references to three different NgModelController instances.

I think different names would bring up even more issues than it would solve.

I don't think so. In fact, Angular will create a unique name for each input[radio] that doesn't have a name attribute.

And perhaps more importantly, doesn't angular want to keep to the W3C HTML spec where radio "name" attribute is used as a key for the radio button group?

I don't think there is any such thing as "radio button group" in HTML. According to the spec, the name attribute of input[radio] elements specifies:

The name part of the name/value pair associated with this element for the purposes of form submission.

In Angular, form submission is totally different, so we don't need the name, because ngModel specifies where to set the value.


So, like I said in my previous comment, I don't see where having same names for different controls in an Angular form would be useful.

Btw, you can create a custom directive that does decouple the name attribute from the name under which the control is registered with its parent form. A very naive approach is shown here.

@gkalpak gkalpak self-assigned this Aug 10, 2016
@Narretz
Copy link
Contributor

Narretz commented Aug 10, 2016

I haven't read everything, but in Angular you still need to give every radio button that belongs to a single choice the same name. Because the checking behavior depends on the browser: if radio (a) gets checked, then radio (b) gets unchecked if both have the same name.

So if one radio has ngIf around it, is added later than all other radios with the same name. That means it'll overwrite any previous ngModelControllers with the same name on the parent form controller. If you remove it via ngIf, it is removed from the parent form, but since we do not special case input radio, we do not re-add any of the still existing radios with the same name.

@tsemer
Copy link
Author

tsemer commented Aug 11, 2016

Thank you both for your answers, even though they contradict each other. I agree with Narrets regarding the name (keeping in line with HTML, nowhere else have I seen that angular requires different names for radio buttons), even though this is not the point of this thread.

The point being: If you guys consider this not a bug, could either of you provide a plunker where there is a working example of an ng-if around a radio button? I haven't found a way to make this work. It works for all other controls. If it cannot be done then I would say it is either an agreed specific limitation for radio buttons and should be mentioned in angular docs, or it is a bug. Would you agree?

@Narretz
Copy link
Contributor

Narretz commented Aug 11, 2016

I first wanted to get some consensus on the issue here, before I re-categorize this issue. So let's wait and see what @gkalpak thinks about the new info.

@gkalpak
Copy link
Member

gkalpak commented Aug 12, 2016

I haven't read everything, but in Angular you still need to give every radio button that belongs to a single choice the same name. Because the checking behavior depends on the browser: if radio (a) gets checked, then radio (b) gets unchecked if both have the same name.

@Narretz, no it is not necessary in Angular, because ngModel will handle the checking/unchecking (we don't need you, mr. browser 😛)

If you remove it via ngIf, it is removed from the parent form, but since we do not special case input radio, we do not re-add any of the still existing radios with the same name.

We definitely shouldn't do that (re-add existing NgModelControllers that were overwritten).

nowhere else have I seen that angular requires different names for radio buttons

Typically, it doesn't (it will work just fine with identical names). BUT, since NgModelController uses its name attribute to register with its parent FormController (as documented), identical names will lead in the overwritten controller's not being accessible on the parent FormController. (If you are OK with that, Angular doesn't mind what you name your inputs 😃)

If you guys consider this not a bug, could either of you provide a plunker where there is a working example of an ng-if around a radio button? I haven't found a way to make this work.

Everything works as expected if you give your radios different names (or no names at all).

It works for all other controls.

It only works if you give your controls different names. The exact same applies to radios.
So, for all types off controls:

  • Different names --> No probem
  • Same names --> Only the last accessible on parent FormController

@gkalpak
Copy link
Member

gkalpak commented Aug 12, 2016

That said, I wouldn't mind having a built-in ngName directive (whose value would be used over that of name - if present). But I don't think it is necessary either.

Maybe we should add a note to the docs, because it is indeed probable that people new to Angular will try to put the same names on their radios.

@Narretz
Copy link
Contributor

Narretz commented Aug 12, 2016

You are right, you don't have to use the same name. I remember I has some issues with this a long time ago, so I thought you can't do individual names.

But I think it's very probable that devs will give the radios the same name, because that's how you do it without angular - and it works in most cases, so there's no indication not to do it.

So maybe add something to the docs, saying that you don't have to use the same name, and you should use unique names when your radios are dynamically added / removed.

@tsemer
Copy link
Author

tsemer commented Aug 13, 2016

Thanks gkalpak for the detailed explanation. The ng-if is indeed no issue with different names.

I completely understand the technicality behind your answer regarding names. Do you mind If we try to step back from technicality and see a bigger picture for a moment? I was under the impression that Angular tries very hard to build on top of HTML and not to change its core usage (is that correct? I hope so..). The overall value is huge - more intuitive framework, less need for documentation because the code documents itself, not just for framework code but also for usage code. If the official instruction is to use different names for radio buttons of the same [name-value] nature, then this is 'breaking' a very common usage of HTML forms. Is keeping HTML in tact not a core value for Angular? Was this just my assumption?

(I understand that radio buttons can still work the HTML way, but you officially advise against it, and it causes my original bug, so I hope you see that this is not a satisfying answer in this case.)

A small additional question that comes to mind is, when using ng-messages for a radio button, are developers supposed to point to any arbitrary name that is bound to that model? I guess this would work, but again seems not very intuitive and would also look somewhat peculiar (which, in turn, may cause more maintenance overhead).

I very much hope my comment does not seem antagonising or petty, it is meant to be neither.

@tsemer
Copy link
Author

tsemer commented Aug 13, 2016

By the way:

I don't think there is any such thing as "radio button group" in HTML.

There is, actually:
https://www.w3.org/TR/html5/forms.html#radio-button-group

And I guess my tl;dr post version is "and boy oh boy how nice it would be if Angular would consider implementing it similarly, even though technically they don't have to." 😃

@Narretz
Copy link
Contributor

Narretz commented Aug 28, 2016

@tsemer we aren't actually supporting a radio group correctly, as our radio binding extends over the boundaries of forms. (It's restricted by scope boundaries).
So, to actually support radios by spec we'd have to make some non-trivial changes.

@tsemer
Copy link
Author

tsemer commented Aug 28, 2016

Thanks @Narretz. That sounds a bit scary, do I understand correctly that within the same scope, two forms containing radio buttons with the same bound model would behave like they are on a single form?

Do you consider it something that you wish to fix, for any milestone?

@Narretz
Copy link
Contributor

Narretz commented Sep 2, 2016

@tsemer It was raised once before iirc, but it's not something that actually affects many people in practice.

@petebacondarwin
Copy link
Contributor

@Narretz wrote:

So maybe add something to the docs, saying that you don't have to use the same name, and you should use unique names when your radios are dynamically added / removed.

I agree. At this stage in the life of AngularJS there is little benefit in implementing such a considerable breaking change.

@petebacondarwin
Copy link
Contributor

Let's leave this open as a marker for adding docs about this.

@petebacondarwin petebacondarwin removed this from the Purgatory milestone Feb 26, 2018
@gkalpak
Copy link
Member

gkalpak commented Mar 6, 2018

I have created #16478 to document the current situation.

Note that — even if should not be necessary in AngularJS-controlled forms (as explained above) — implementing an "ngName" directive yourself (i.e. a directive that allows you to use the name [name] on multiple radios and still have their NgModelControllers available on the parent FormController), would be as simple as:

.directive('ngName', () => ({
  link: {pre: (s, e, attrs) => attrs.$observe('ngName', name => attrs.$set('name', name))},
  priority: 2,
  restrict: 'A',
}))

Then, use it like this:

<form name="myForm">
  <label>
    <input type="radio" name="same-name" ng-name="radio1" value="foo" ng-model="data.choice" />
    Foo
  </label>
  <label>
    <input type="radio" name="same-name" ng-name="radio2" value="bar" ng-model="data.choice" />
    Bar
  </label>
</form>
<pre>1st NgModelController: {{ myForm.radio1 | json }}</pre>
<pre>2nd NgModelController: {{ myForm.radio2 | json }}</pre>

Demo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.