Fix: set $submitted on child forms when form is submitted

[Narretz]

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

What is the current behavior? (You can also link to an open issue here)
When submitting a form, $submitted is not set on child forms

Does this PR introduce a breaking change?
I don't think so.
No one in #10071 said that his might be breaking.
AngularJS doesn't have nested form isolation, and ngForm is only for grouping sub-forms. I think that it's expected that a submission event will affect all sub-forms.
Probably: #11023 (comment)

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)

Follow up to #11023

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[petebacondarwin]

The onlySetOnChildren param name is misleading as it might imply that we don't set it on this :-)
How about something about not setting it on the parent?

[petebacondarwin]

Is this param even necessary? I don't see it used anywhere?
Perhaps it would be best to leave it out for this PR?

[Narretz]

It's used in line 278 - we need it because if we always set it on both parent and children we can go into an infinite loop.
You're right with the parameter name. How about doNotSetOnParent?

[petebacondarwin]

oh right!
+1 on the new param name

[mgol]

This is another boolean trap in a public API, isn't it? :/

[mgol]

We have quite a lot of boolean parameters in our providers config methods but then they're not so bad as the convention is shared across the library and in all cases it just enables or disables the setting indicated by the method name.

Here there's no obvious relation between the parameter and the method name; it's impossible to know what's happening just by looking at the code. Naming the variable properly only helps us. Note, though, that negative parameters are confusing to people as when you set it to false you have double negation and it's been scientifically proven that it's harder for people to process.

If this is going to extent the public API, I think we should use an object with a parameter so that it's named (and then rather setOnParent than doNotSetOnParent). It's way easier to do it in ES6 but it's still possible in ES5.

[petebacondarwin]

The alternative is to create a new private function. Perhaps something like this:

$setSubmitted: function() {
  var rootForm = this;
  while(rootForm.$$parentForm) { rootForm = rootForm.$$parentForm; }
  rootForm.$$setSubmitted();
}

$$setSubmitted() {
  this.$$animate.addClass(this.$$element, SUBMITTED_CLASS);
  this.$submitted = true;
  forEach(this.$$controls, function(control) {
    if (control.$$setSubmitted && !control.$submitted) {
      control.$setSubmitted(false);
    }
  });
}

[Narretz]

@petebacondarwin wdyt about the breaking-changeness of the PR? @gkalpak had some reservations in the previous PR: #11023 (comment)

[mgol]

We have quite a lot of boolean parameters in our providers config methods but then they're not so bad as the convention is shared across the library and in all cases it just enables or disables the setting indicated by the method name.

Here there's no obvious relation between the parameter and the method name; it's impossible to know what's happening just by looking at the code. Naming the variable properly only helps us. Note, though, that negative parameters are confusing to people as when you set it to false you have double negation and it's been scientifically proven that it's harder for people to process.

If this is going to extent the public API, I think we should use an object with a parameter so that it's named (and then rather setOnParent than doNotSetOnParent). It's way easier to do it in ES6 but it's still possible in ES5.

[petebacondarwin]

Regarding the BC - I guess that it is a breaking change. And this also raises the importance of being able to "detach" or "isolate" subforms from their parent. In which case you could block the child/parent form from being submitted if it was not "attached" to the form on which the setSubmit was called.

[Narretz]

Ok, then let's put into 1.7.0 and rethink the API.

[aeslinger0]

I'm not sure why this needs to be a breaking change. I'm probably missing something. If the goal is to set child forms to submitted when the parent form is submitted, couldn't you add an attribute to ngForm such as inherit-submit="true" and use that as a flag to conditionally apply the new functionality? Since it'd be an optional attribute that results in the current behavior when omitted, I don't think it'd be breaking. Plus, it'd give you the ability to control form-by-form which subforms inherit the submit action.

[petebacondarwin]

@aeslinger0 that is an interesting idea. What do you think @Narretz ?

[Narretz]

Hm, I have to think about it, but in general the form API isn't very consistent as is, and I don't think an option that affects a single part of the API is very elegant.

[Narretz]

I've implemented @petebacondarwin 's approach to setting submitted.
Wrt the need for having a way to detach the form from its parent, here are a few notes on the consequences that would follow #10193 (comment)

Detaching the form is easy (just call $removeControl on the parent form), but the behavior that the detached form should have is not.

[Narretz]

Here's a re-cap on what I think would be needed to support "detached" forms, i.e. a form that does not inherit or propagate state from / to its parent form.

Requirements:

1. A way to detach / re-attach a form. This can be done with $removeControl on linking, and could be wrapped in a directive that watches an expression to toggle this behavior. (When detaching / re-attaching later, the state would only be propagated after the form changes again)

2. To specify / implement the behavior of a detached form with regard to form submission.

Problems:
2a) the submit rules for forms (see https://code.angularjs.org/1.6.7/docs/api/ng/directive/form#submitting-a-form-and-preventing-the-default-action) also only apply to form elements. i.e. if you have ngForm with only one text input, pressing enter in the text input should trigger a submit event.

2b) only the form element fires a "submit" event when you press a submit button etc., ngForm does not. This means, if you want to submit detached forms individually, you would have to wrap forms in ngForm. That however would mean, you cannot submit the whole form.
Also, if you have form -> ngForm, and you press a submit button in the detached ngForm, the parent form will still receive the submit event.

Solution:
Imo can both 1) and 2) be implemented in user land - in fact, there are already libs that do 1) (although not perfect). For 2), a) is also already partly available (https://github.com/MarkPieszak/Angular1-ngForm-ngSubmit-fixes).
b) is more difficult, but should be possible by firing synthetic submit event on ngForm and overriding the form
submission handling with a function that checks if the submit event was triggered by an element that sits inside a detached ngForm (in which case it would not be fired on this form).

We should also update the docs with the info that in core ngForm, when used standalone, does not support form-specific submit handling.

Finally, I would introduce this PR as the default, without an option - if you have nested forms that are not detached, then I find it reasonable that the parent form sets $submitted on child forms.

[gkalpak]

I still don't like the idea of detached forms fwiw 😁

[Narretz]

That's why I would move this whole topic to user-land. I agree that the semantics are unclear, it can become very complex very quickly, we don't know what kind of behavior devs actually want.
From my perspective, we only need to decide if this PR is breaking or not, or if we should make it configurable.

[petebacondarwin]

This PR looks good to me as it stands. Let's punt on detached forms for the moment.

[Narretz]

Ok, so then I will squash the commits, add a BC notice and merge only into master.

[gkalpak]

Why the !control.$submitted check? Under what circumstances can a sub-form be $submitted (while the parent form is not) and does that guarantee that all sub-forms of the sub-form are also $submitted?

[Narretz]

Technically, the form could have been removed with "removeControl", then have been changed to submitted via "setSubmitted" and re-attached to the parent form with "addControl". Unlikely, but possible. I don't think it's worth optimzing this, though. I think I took the implementation from this comment: #15778 (comment)