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 #1964

Open
nathansobo opened this Issue Jul 30, 2014 · 9 comments

Comments

Projects
None yet
4 participants
@nathansobo
Copy link

nathansobo commented Jul 30, 2014

The document-wide handlers for native events that perform synthetic event dispatch execute in 0.2-0.7 milliseconds on my machine. I realize this doesn't seem like much time, but we're really trying to shave off any non-essential source of latency for typing and cursor movement in Atom, and every little bit helps. Disabling synthetic events on keydown and textinput is saving about 1ms of latency for a keystroke.

In our fork, I've added the ability to add a reactSkipEventDispatch property to the native event to opt out of synthetic dispatch, but I'd be interested in a more official mechanism for opting out of this feature for certain event types. Even better, perhaps React could maintain a cache of what event types are actually being listened for and bail out as soon as possible if handling an event.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented Jul 30, 2014

React should only attach top-level handlers for events that you're using in your components – are you seeing otherwise?

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 31, 2014

@nathansobo I'm curious whether it is the simulated dispatcher (i.e, capturing, bubbling, etc) or the initialization of the synthetic event that is most costly for you.

I assume it is the simulated dispatcher, which currently does a lot of ugly string hacking to do its thing (chenglou is working on that I believe). While the SyntheticKeyboardEvent has a lot of polyfills, they're all very simple and I don't imagine that they're the issue (but perhaps they are... because browsers).

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 31, 2014

Also, you can easily opt-out of React's event dispatcher by just not using it and calling addEventListener from inside componentDidMount instead. You pay a slight up-front cost though.

PS. I imagine that since you're operating in a single modern and fast environment, React's entire event dispatcher is quite redundant for you, you could probably just replace it internally with immediate calls to addEventListener.

@nathansobo

This comment has been minimized.

Copy link
Author

nathansobo commented Jul 31, 2014

I'm as sure as I can be that we're not attaching event handlers for keydown and textInput events via React properties anywhere in our code. Are they perhaps attached by the input DOM component?

Here's a flame chart of a keystroke, with keydown and textInput events being processed by React.
screenshot_2014-07-31_14_37_26
screenshot_2014-07-31_14_37_39
screenshot_2014-07-31_14_38_01

Upon consideration we should probably just stopPropagation of these events and they'll never reach the handler. But for some reason they are indeed being handled in a non-trivial amount of time.

I'm quite intrigued by your idea of just using native APIs directly and skipping the global handling. I'm assuming that would require some more logic in our fork to interpret the event handler properties of DOM components?

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 31, 2014

@nathansobo Ah, that explains it. Yeah, React.DOM.input (overloaded by ReactDOMInput) always attaches an onChange handler (does not behave like HTML onchange), which is implemented by ChangeEventPlugin and listens to quite a number of events. If you're using controlled inputs it's obviously required, but it seems like it should be possible to discard it for uncontrolled inputs, but I'm not 100% sure without further examination of the code.

So if you're using uncontrolled inputs, it's probably something we should have a look at to see if it can avoided. If you're using controlled inputs, your best bet is probably to implement your own controlled inputs on-top of uncontrolled inputs (again, we should have a look at if the change handler can be avoided for them). However, you can remove ReactDOMInput from ReactDefaultInjection right now, and you get the raw input component in React.DOM.input instead, which you could use to implement controlled inputs using native events if you want.

As for using native APIs directly, I'm not super familiar with the event system code in React. But apart from onChange (which has a different HTML5 equivalent I believe), all the events represent the equivalent HTML5 events. So simply discarding/ignoring the entire synthetic React event system and updating ReactDOMComponent as necessary (look for putListener, deleteListener, etc, perhaps you could just update those instead...) to add native event listeners directly should be a piece of cake (and not a hack). Again, you pay a higher up-front cost for each event listener though, but that may be preferable in your case.

(You probably want to remove ReactDOMInput, etc, regardless of if you decide to switch ReactDOM to native events or not)

@nathansobo

This comment has been minimized.

Copy link
Author

nathansobo commented Jul 31, 2014

Thanks for your thoughtful explanation here. Yeah, this input definitely uncontrolled. It's not even visible, but just exists to receive input which we render ourselves in the editor. For now, stopping propagation of keydown and textInput events seems to eliminate the expensive handling from the flame graph.

I'm hesitant to remove controlled inputs entirely in case users want to use them elsewhere. For now we're exporting our copy of React to users until the multiple-instance issue is addressed, so I'd like inputs to work as expected for them.

Not sure disabling the synthetic event system is warranted yet, but if we see it costing us time elsewhere I'll definitely take a crack at it.

I'm going to close this now. Thanks for the conversation!

@nathansobo nathansobo closed this Jul 31, 2014

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented Jul 31, 2014

Pretty sure we don't need to attach the event handlers for uncontrolled components. I'll check and let you know.

@sophiebits sophiebits reopened this Jul 31, 2014

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 31, 2014

@nathansobo If you're stopping propagation of textinput and keydown then you have not removed controlled inputs, you've broken them instead. (EDIT: Or perhaps that's not true now that I read it again, as it seems you're preventing the events further up the tree) You can very easily reimplement controlled inputs with the raw input component, if that's something you want to expose for users (but don't want to replace the synthetic event system entirely)... so no real harm done.

That being said, the synthetic event system in React exists for two reasons AFAIK:

  1. Normalize browser inconsistencies (lots of them), especially old browsers.
  2. Drop up-front O(n) addEventListener cost, for slightly more expensive event handling.

1 should be irrelevant for you, and 2 is probably not as important for you as you're in a faster modern environment. Also, dropping frames is far more noticeable (as you say) than a popup taking a few frames longer to open in worst-case. Just food for thought.

I definitely see Atom becoming my one-and-only editor in the future, so I'm just happy to help. :)

sophiebits added a commit to sophiebits/react that referenced this issue Aug 1, 2014

Don't add top-level events for uncontrolled inputs
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

This comment has been minimized.

Copy link
Collaborator

sophiebits commented Aug 1, 2014

Just made #1968 with a possible fix, though I'm unsure how well I like it.

chenglou added a commit to chenglou/react that referenced this issue Aug 20, 2014

[RFC] Per React container event listening/dispatching
Work in progress. `enterleave` plugin (and maybe `analyticsPlugin`) is broken because it relied on the old behavior. But wanted to put this out here for suggestions. There are also some comments that need to be changed, I'll do it when the code is finalized.

If we attach the event listening/dispatching at container level, it'll benefit the case of `<Editor/><Plugin1/>` (both are container roots), since `Plugin1` won't disturb `Editor`.

We also detach those listeners now. There wasn't really a need in the past.

Fixes facebook#2043
Should help with facebook#1964

@gaearon gaearon changed the title Option to disable React's handling of synthetic events? Don't add top-level events for uncontrolled inputs Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.