Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent submit from propagating. #1336

Merged
merged 7 commits into from
Jul 11, 2019

Conversation

fsteger
Copy link
Contributor

@fsteger fsteger commented Jun 26, 2019

Reasons for making this change

When forms are nested in the react DOM, child submit events will otherwise propagate to the parent form. This can result in unexpected behavior, such as stale data on the parent form if submission of the child triggers an update to the parent's data.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@epicfaace
Copy link
Member

Trying to understand your use case -- forms should not be nested anyway, so maybe there is another solution to this?

@juliankigwana
Copy link
Contributor

juliankigwana commented Jun 30, 2019

I have a use case:

Take the following schema property nameIdValueMap which is an Array and each child has its own child properties. I would like to utilise the power of the RJSF library and the components I have build around it to render these child properties in a sub-form, however when I do so we have the propagation issue as well as invalid html output having nested forms.

"nameIdValueMap": {
    "title": "NameID Value Map",
    "type": "array",
    "items": {
        "type": "object",
        "properties": {
            "key": {
                "title": "Key",
                "type": "string"
            },
            "value": {
                "title": "Value",
                "type": "string"
            },
            "binary": {
                "title": "Binary",
                "type": "boolean"
            }
        }
    }
}

Screen Shot 2019-06-28 at 15 50 31

There is an added issue with the fact that the form tag is hard coded into the Form.js and that nesting forms like this will produce invalid html and a nasty error in the console. I would recommend then that in addition to this change it would need a way to override the form tag with something else. This should allow us to render complex nested schemas inside of arrays.

@epicfaace
Copy link
Member

epicfaace commented Jun 30, 2019

Yes, I agree, being able to override the default tag created so it is not a <form> would help, and I would definitely accept a PR that does that.

If that happens, though, is there any additional need for preventing submit form from propagating? (given that we won't need to have nested <form> tags)

@fsteger
Copy link
Contributor Author

fsteger commented Jul 1, 2019

For my use case, the forms were not nested in the html dom; only the react dom.

I was using a custom ArrayFieldTemplate that adds in a control for each row that opens a modal in a react portal. The modal has a form where the user can update some aspects of the row, such as entering a new position. On submitting the form, what I was seeing was the Form.js onSubmit getting called twice-- once targeting the modal form (expected), and again targeting the parent form (unexpected). This was creating an odd race condition where the child form's submit would update the form data (such as changing the row's position); and then the parent form's onSubmit would run with the stale data still in its state, essentially reverting the change.

@epicfaace
Copy link
Member

I see, makes sense, thanks!

@epicfaace
Copy link
Member

It seems like it's not a good idea to stop event propagation, though. https://css-tricks.com/dangers-stopping-event-propagation/

Since the event we're trying to prevent (parent form submission) is also controlled by us in rjsf, could we perhaps alternatively do this by adding some logic in onSubmit to see whether the form that initiated the event is the same as the current form?

@epicfaace
Copy link
Member

Thanks for the change. All right -- seems like the logic is sufficiently complicated that it needs a test. Could you add a test with a Form-in-Form scenario?

const arraySubmit = arrayForm.querySelector(".array-form-submit");

arraySubmit.click();
sinon.assert.calledOnce(onSubmit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can get more specificity on what exactly called onSubmit -- can you create two functions, outerOnSubmit and innerOnSubmit, then test to see if outerOnSubmit is never called and innerOnSubmit is called?

src/components/Form.js Show resolved Hide resolved
@epicfaace
Copy link
Member

Looks good -- thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants