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

Use setImmediate to defer value restoration #1759

Merged
merged 2 commits into from
Jul 30, 2014
Merged

Conversation

sophiebits
Copy link
Collaborator

Depends on #1758.

Fixes #1698.

Previously, controlled components would update too soon when using something like ReactLayeredComponentMixin (i.e., before the layer's updates could propagate from the parent), causing the cursor to jump even when always updating the new model value to match the DOM state. With this change, we defer the update until after all nested updates have had a chance to finish, which prevents the cursor from misbehaving.

Also cleaned up the logic around updating a bit -- the .value and .checked updates in ReactDOMInput weren't being relied on at all so I removed them and opted for a simple forceUpdate instead. I also got rid of _isChanging which hasn't been necessary since the introduction of update batching.

Test Plan: Tested the example in http://jsfiddle.net/Bobris/ZZtXn/2/ and verified that the cursor didn't jump. Changed the code to filter out numbers and verified that the field prevents typing numbers (attempting to do so still causes the cursor to jump to the end). Also verified that controlled and uncontrolled radio buttons, textareas, and select boxes work.

@Bobris
Copy link

Bobris commented Jun 29, 2014

Wow, thanks a lot!

Unfortunately I cannot build it on my Windows machine, so I am hoping for some early release:

>grunt build --force
Running "delete-build-modules" task

Running "jsx:normal" (jsx) task
Warning: This socket is closed. Used --force, continuing.

Running "version-check" task

Running "browserify:basic" (browserify) task
Fatal error: Uncaught, unspecified "error" event.

@sophiebits
Copy link
Collaborator Author

Here's a build of this version, hopefully it works for you:

https://s3.amazonaws.com/uploads.hipchat.com/6574/26709/yXvD7Jm5JM0NBjs/react-pull-1759.zip

@Bobris
Copy link

Bobris commented Jun 29, 2014

You are just amazing. I wasn't even able to boot up Linux in VM :-). I will let you know how it will work. Thanks again.

@Bobris
Copy link

Bobris commented Jun 29, 2014

I can confirm, it works beautifully.

@zpao
Copy link
Member

zpao commented Jul 1, 2014

cc @sebmarkbage

@Tvaroh
Copy link

Tvaroh commented Jul 17, 2014

@spicyj Ben, when this fix will be officially released? Does it go into 0.11?

@sophiebits
Copy link
Collaborator Author

@Tvaroh No, it will not be in 0.11. It might be in 0.12 or it might not be. I haven't even tested to see if this fixes the rAF batching problem. rAF batching is not ready for use in production.

@Tvaroh
Copy link

Tvaroh commented Jul 17, 2014

@spicyj I tried the build above and it doesn't fix the problem. I optionally provide RAF support in my state management layer on top of React called Morearty.

@Tvaroh
Copy link

Tvaroh commented Jul 17, 2014

For clarity - it doesn't use customized batching strategy. It just re-renders from the top in RAF (optionally). Doing this in RAF causes cursor to move at the line's end.

@sophiebits
Copy link
Collaborator Author

@Tvaroh Sorry, I see. Controlled components won't work as-is for you as they assume you'll update the state value immediately. My best suggestion is to make your own wrapper around <input/> like Om does.

@Tvaroh
Copy link

Tvaroh commented Jul 17, 2014

Okay, thanks for the help, Ben.

@zpao zpao mentioned this pull request Jul 21, 2014
Callbacks passed to this setImmediate function are called at the end of the current update cycle, which is guaranteed to be asynchronous but in the same event loop (with the default batching strategy).

This is useful for new-style refs (facebook#1373, facebook#1554) and for fixing facebook#1698.

Test Plan: jest
Depends on facebook#1758.

Fixes facebook#1698.

Previously, controlled components would update too soon when using something like ReactLayeredComponentMixin (i.e., before the layer's updates could propagate from the parent), causing the cursor to jump even when always updating the new model value to match the DOM state. With this change, we defer the update until after all nested updates have had a chance to finish, which prevents the cursor from misbehaving.

Also cleaned up the logic around updating a bit -- the .value and .checked updates in ReactDOMInput weren't being relied on at all so I removed them and opted for a simple forceUpdate instead. I also got rid of _isChanging which hasn't been necessary since the introduction of update batching.

Test Plan: Tested the example in http://jsfiddle.net/Bobris/ZZtXn/2/ and verified that the cursor didn't jump. Changed the code to filter out numbers and verified that the field prevents typing numbers (attempting to do so still causes the cursor to jump to the end). Also verified that controlled and uncontrolled radio buttons, textareas, and select boxes work.
Tvaroh referenced this pull request in moreartyjs/moreartyjs Jul 28, 2014
@slorber
Copy link
Contributor

slorber commented Jul 29, 2014

@spicyj @Tvaroh pointed me to this pull request.

I use a custom JS framework inspired by Om and use ReactLink with an Atom/cursor implementation:

                    <input type="text" valueLink={this.linkCursor(this.formUsernameCursor())} required="true"/>

My app (React/Phonegap) used to be rendered from the very bottom, I don't have any component with any local state.

I tried to use rAF / setTimeout to render my app from the top and it didn't work well. But actually it did work well in my browser! Once deployed in my Android phone, they were very unexpected behaviors.

On my phone:

  • The caret always goes at the end
  • The backspace button does not work (does not remove chars)
  • If I recently typed a new char, the backspace button does work (!!!) (recently ~= 1sec)

On some other phones:

  • The caret always goes at the end
  • The backspace button does not work (does not remove chars)
  • On some unpredictable case, the backspace button does work and on some others it duplicates the text content of the input (!!!)

So, I just wanted to make you aware of these issues I had and advise you to test this on a React/Phonegap app as well :)

@sophiebits
Copy link
Collaborator Author

Using controlled components with requestAnimationFrame batching is not currently supported and this PR doesn't attempt to fix it.

zpao added a commit that referenced this pull request Jul 30, 2014
Use setImmediate to defer value restoration
@zpao zpao merged commit 590e505 into facebook:master Jul 30, 2014
@slorber
Copy link
Contributor

slorber commented Jul 30, 2014

yes @spicyj , it's just to inform you of this behavior, if one day you want to support rAF in controlled components :) just don't know where to write this as there doesn't seem to be any issue open for that

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.

Controlled input cursor jumps when used with layers
5 participants