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

Don't add top-level events for uncontrolled inputs #1968

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

Fixes #1964.

Test Plan: jest. Also verified that the ballmer-peak example still works and works when changed to use a textarea or select, but that if the input is changed to an uncontrolled component with no onChange, that Chrome doesn't list the event handlers bound in the Elements > Event Listeners panel.

Fixes facebook#1964.

Test Plan: jest. Also verified that the ballmer-peak example still works and works when changed to use a textarea or select, but that if the input is changed to an uncontrolled component with no onChange, that Chrome doesn't list the event handlers bound in the Elements > Event Listeners panel.
@sophiebits
Copy link
Collaborator Author

This certainly makes the code more complex, not sure if it's worth it. (Refactoring ReactDOMSelect to not always read in handleChange probably seems good regardless though…)

@syranide
Copy link
Contributor

syranide commented Aug 1, 2014

(Kind of unrelated)

Personally (and perhaps I'm alone in this) I would like for us to have explicit paths for all the different "unique" input types, rather than the weird mess that it is right now.

Also, what on earth is going on in componentDidMount and compnentWillMount?

returnValue = onChange.call(this, event);

Why does that call the onChange handler with this? That doesn't appear to make any sense to me.

@sophiebits
Copy link
Collaborator Author

Not sure what you mean, that call is in _handleChange only?

@syranide
Copy link
Contributor

syranide commented Aug 3, 2014

@spicyj That was just unrelated thoughts about the implementation of ReactDOMInput, but I realized onChange.call(this, ...) is because the LinkValueUtils handler expects this to be the input... that doesn't seem very sanitary (if someone would provide a non-bound handler).

Anyway, for posterity, I'm guessing similar action should be taken to avoid getNode side-effects for ReactDOMImg (perhaps ReactDOMForm too?) as these cause immediate evaluation of all nodes up to the target, http://www.w3.org/TR/DOM-Level-3-Events/#event-type-load says onload doesn't bubble.

@sophiebits
Copy link
Collaborator Author

This complicates the code a bit and I haven't heard that this actually helps, so closing.

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

Successfully merging this pull request may close these issues.

Don't add top-level events for uncontrolled inputs
4 participants