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

Allow input elements to opt out of selection restoration #1439

Closed

Conversation

nathansobo
Copy link

Refs #1350

This PR is mainly to start a conversation. I'm not sure if this is the ideal implementation, and tests need to be added regardless. But I wanted to get some feedback before investing further time.

I'm building a new editor view for Atom with React, and selection restoration is costing ~2ms when handling text insertions. I'd like to selectively disable it for the hidden input element that receives input events on behalf of the editor.

With selection restoration:
screenshot_2014-04-23_09_11_00

Without selection restoration:
screenshot 2014-04-23 09 14 27

Ideally I'd like to check a property on the component corresponding to the input node, like unsafeSkipSelectionRestoration, but I couldn't figure out how to map the raw DOM node back to the react component that originated it. Is there any global mapping of react id's to component instances?

If the 'data-react-skip-selection-restoration' attribute is true, we
won't attempt to restore the selection during reconciliation. This
can improve performance when input elements are focused if the user is
sure they won't be removed and reattached.
@zpao
Copy link
Member

zpao commented Apr 23, 2014

Thanks for starting the discussion @nathansobo. There is a mapping somewhere but before we go down that road, let's make sure we want to do it that way.

cc @salier @joshduck @yungsters - I think we probably just shouldn't be attempting to restore selection in type="hidden" elements. What do you think?

@nathansobo
Copy link
Author

@zpao When I say "hidden", what I mean is that I have the opacity set to 0, not that it's a password field. Since I'm implementing a text editor, I'm just using the input element to receive events but doing all the rendering in HTML with React.

@nathansobo
Copy link
Author

I'm probably going to need to publish an Atom-specific fork of React until we can resolve this in the official framework. That's no big deal, but I thought I'd let you know. Enjoying the framework!

@jordwalke
Copy link
Contributor

👍

@petehunt
Copy link
Contributor

@zpao we should probably try to solve this so @nathansobo doesn't need to fork. I wonder if we can create a customized React.DOM.textarea that blacklists itself from ReactInputSelection (I don't understand this stuff that well so I may be off base here).

@sophiebits
Copy link
Collaborator

We might be able to do a variant of @syranide's #870 to prevent detaching inputs from the DOM unnecessarily, which I believe alleviates the need for this transaction wrapper.

@petehunt
Copy link
Contributor

Hmm, maybe the best way would be to get rid of this transaction wrapper and instead only do the selection restoration when we detect that a text input node will be moved? I wonder if we can efficiently detect that with ChildrenUpdates?

@syranide
Copy link
Contributor

@petehunt Interestingly enough you can, and as @spicyj suggest, I'm pretty sure you could piggyback on my #870 if you're ok with it. Simply traversing up the parents for the active input during each processUpdates and detecting when any of those parents are moved, should be super fast and robust. Additionally, unless there's a conflict where an iframe and input switch places, we could simply treat the input as "immovable" for that update (but perhaps it only complicates the logic for an uncommon case).

So you'd only need to get and set the selection when you know it's going to move/has moved.

@sebmarkbage
Copy link
Collaborator

Adding a revision needed flag since this is probably not the exact solution we'll land, but this seems like it still needs a fix.

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.

7 participants