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

Support invalid event within Form elements #5152

Closed
kidwm opened this issue Oct 13, 2015 · 5 comments
Closed

Support invalid event within Form elements #5152

kidwm opened this issue Oct 13, 2015 · 5 comments

Comments

@kidwm
Copy link

kidwm commented Oct 13, 2015

The invalid event is fired when a submittable element has been checked and doesn't satisfy its constraints.

Ref: https://developer.mozilla.org/en-US/docs/Web/Events/invalid

@tomduncalf
Copy link
Contributor

I'm taking a look at this as a first bug - from the docs and some testing, the invalid event seems to be fired at the form element level rather than the form itself.

That is to say:

<form oninvalid="console.log('invalid form')">
  <input type="text" required />
  <button type="submit"/>
</form>

will not log invalid form on submit, but:

<form>
  <input type="text" required oninvalid="console.log('invalid input')" />
  <button type="submit"/>
</form>

will log invalid input on submit.

Does this match the expected behaviour, and if so do we want the onInvalid handler to be on the individual React.DOM form elements (i.e. <input onInvalid={...}> so it is called when that element is invalid), or do we want it to bubble up to the parent <form> (i.e. <form onInvalid={...}> so it is called whenever at least one element in that form is invalid)?

@zpao
Copy link
Member

zpao commented Oct 14, 2015

We should match the DOM and fire it on the element (not the form). We already have support for these events which don't bubble. Most of our events are added at the document level and then delegated but the ones which don't bubble need to be added to specific elements on creation. Here's how we're doing it for forms and the submit & reset events: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponent.js#L403-L416

Thanks for taking this on! Feel free to get a PR up before it's completely ready if you have questions / need help.

@tomduncalf
Copy link
Contributor

Thanks Paul, good to know I was looking in the right place! Will update when I have had time to create a PR or if I need any help :)

Cheers
Tom

On 14 Oct 2015, at 20:23, Paul O’Shannessy notifications@github.com wrote:

We should match the DOM and fire it on the element (not the form). We already have support for these events which don't bubble. Most of our events are added at the document level and then delegated but the ones which don't bubble need to be added to specific elements on creation. Here's how we're doing it for forms and the submit & reset events: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponent.js#L403-L416

Thanks for taking this on! Feel free to get a PR up before it's completely ready if you have questions / need help.


Reply to this email directly or view it on GitHub.

@kidwm kidwm changed the title Support invalid event on Form tag Support invalid event within Form elements Oct 15, 2015
tomduncalf added a commit to tomduncalf/react that referenced this issue Oct 15, 2015
tomduncalf added a commit to tomduncalf/react that referenced this issue Oct 15, 2015
tomduncalf added a commit to tomduncalf/react that referenced this issue Oct 15, 2015
@tomduncalf
Copy link
Contributor

Hey,

I've put together a PR at #5187 - it would be great to get your feedback on it.

A couple of specific questions I had:

  1. How should I handle automated testing of this? There's a lot of test code to look at and I'm unfamiliar with the React codebase, so I'm unsure if/how this should be tested.
  2. Does the splitting out of the _setupTrapBubbledEventsLocal logic make sense? Or do you think it would be better to have a separate switch statement for this bit, preceeding the other switch in mountComponent, matching all the elements which need this setup, and then keep the other switch as is (minus the first case)

Any feedback appreciated, this does seem to work but I'm totally new to the React code so not sure if the way I've gone about it is suitable or not!

Cheers,
Tom

@kidwm
Copy link
Author

kidwm commented Oct 16, 2015

@tomduncalf Awesome!

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

No branches or pull requests

4 participants