Fix: set $submitted on child forms when form is submitted by Narretz · Pull Request #15778 · angular/angular.js

Narretz · 2017-03-06T12:20:22Z

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 · 2017-03-06T12:20:24Z

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 · 2017-03-06T12:20:25Z

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 · 2017-03-23T08:37:27Z

-    this.$$animate.addClass(this.$$element, SUBMITTED_CLASS);
-    this.$submitted = true;
-    this.$$parentForm.$setSubmitted();
+  $setSubmitted: function(onlySetOnChildren) {


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?

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

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?

oh right!
+1 on the new param name

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

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.

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 · 2017-03-23T09:28:27Z

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

mgol · 2017-03-23T09:52:06Z

-    this.$$animate.addClass(this.$$element, SUBMITTED_CLASS);
-    this.$submitted = true;
-    this.$$parentForm.$setSubmitted();
+  $setSubmitted: function(onlySetOnChildren) {


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 · 2017-03-23T10:18:38Z

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 · 2017-03-23T11:56:07Z

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

aeslinger0 · 2017-08-16T15:23:29Z

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 · 2017-08-16T15:34:49Z

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

Narretz · 2017-08-17T09:16:14Z

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 · 2017-11-07T17:10:05Z

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 · 2017-12-07T13:45:05Z

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:

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)
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 · 2017-12-07T16:11:36Z

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

Narretz · 2017-12-07T18:19:21Z

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 · 2017-12-11T08:18:05Z

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

gkalpak · 2017-12-11T09:02:02Z

    this.$submitted = true;
-    this.$$parentForm.$setSubmitted();
+    forEach(this.$$controls, function(control) {
+      if (control.$$setSubmitted && !control.$submitted) {


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?

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)

…tted Closes angular#10071 BREAKING CHANGE: Forms will now set $submitted on child forms when they are submitted. For example: ``` <form name="parentform" ng-submit="$ctrl.submit()"> <ng-form name="childform"> <input type="text" name="input" ng-model="my.model" /> </ng-form> <input type="submit" /> </form> Submitting this form will set $submitted on "parentform" and "childform". Previously, it was only set on "parentform". This change was introduced because mixing form and ngForm does not create logically separate forms, but rather something like input groups. Therefore, child forms should inherit the submission state from their parent form.

Narretz added component: forms frequency: moderate severity: confusing type: bug labels Mar 6, 2017

Narretz added this to the 1.6.x milestone Mar 6, 2017

googlebot added the cla: no label Mar 6, 2017

Narretz mentioned this pull request Mar 6, 2017

fix(form): set $submitted to true on child forms when parent is submitted #11023

Closed

Narretz added the needs: review label Mar 6, 2017

Narretz modified the milestones: 1.6.x, 1.6.4 Mar 8, 2017

petebacondarwin suggested changes Mar 23, 2017

View reviewed changes

mgol suggested changes Mar 23, 2017

View reviewed changes

Narretz modified the milestones: 1.6.4, 1.7.0 Mar 23, 2017

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

andremw mentioned this pull request Aug 24, 2017

$submitted is not set on sub ng-form #10071

Closed

Narretz self-assigned this Nov 7, 2017

Narretz force-pushed the feat-form-submitted branch from beb33a4 to 7787d00 Compare November 7, 2017 16:49

petebacondarwin approved these changes Dec 8, 2017

View reviewed changes

gkalpak reviewed Dec 11, 2017

View reviewed changes

Narretz force-pushed the feat-form-submitted branch 2 times, most recently from 118a547 to 6bf01af Compare December 12, 2017 11:48

Narretz added 2 commits December 12, 2017 14:12

docs(ngForm): clarify usage and limitations

e98bb35

Narretz force-pushed the feat-form-submitted branch from 44a6659 to e98bb35 Compare December 12, 2017 13:13

Narretz merged commit f6e60c1 into angular:master Dec 13, 2017

Narretz deleted the feat-form-submitted branch December 13, 2017 18:23

Conversation

Narretz commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Mar 6, 2017

Uh oh!

googlebot commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Mar 23, 2017

Uh oh!

Narretz commented Mar 23, 2017

Uh oh!

aeslinger0 commented Aug 16, 2017

Uh oh!

petebacondarwin commented Aug 16, 2017

Uh oh!

Narretz commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narretz commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narretz commented Dec 7, 2017

Uh oh!

gkalpak commented Dec 7, 2017

Uh oh!

Narretz commented Dec 7, 2017

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Narretz commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Narretz commented Mar 6, 2017 •

edited

Loading

Narretz Mar 23, 2017 •

edited

Loading

Narretz commented Aug 17, 2017 •

edited

Loading

Narretz commented Nov 7, 2017 •

edited

Loading